Bug 1895 - IP header Source Address changed while forwarding RREQ
IP header Source Address changed while forwarding RREQ
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: dsr
ns-3-dev
All All
: P5 critical
Assigned To: Yufei Cheng
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-02 20:39 EDT by Tomasz Seweryn
Modified: 2014-05-02 10:38 EDT (History)
4 users (show)

See Also:


Attachments
Forwarding proper source address for the IP header (240 bytes, patch)
2014-04-02 20:39 EDT, Tomasz Seweryn
Details | Diff
Attachment with proper directions of diff (240 bytes, patch)
2014-04-02 20:45 EDT, Tomasz Seweryn
Details | Diff
Patch for the IP source header check for the rreq message (50.59 KB, application/octet-stream)
2014-04-07 18:44 EDT, Yufei Cheng
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tomasz Seweryn 2014-04-02 20:39:56 EDT
Created attachment 1812 [details]
Forwarding proper source address for the IP header

In dsr-options.cc, DsrOptionRreq::Process method, a DsrRouting::ScheduleInterRequest method is invoked to forward the RREQ packet during route discovery process. Inside DsrRouting::ScheduleInterRequest a DsrRouting::SendRequest method is scheduled with "packet" and "m_mainAddress" parameters. "m_mainAddress", which is a variable having value of current node ip address, is later used in lower layers for filling a source address field of ipv4 header of the forwarded packet. In a result, every time a RREQ packet goes through any intermediate node, the IP header source address field changes. This field should remain unchanged.

Proofs can be found in the following document:
https://www.ietf.org/rfc/rfc4728.txt
1)Page 66, in 8.2.2, first bullet:
"If the Target Address field in the Route Request matches this node's own IP address, then the node SHOULD return a Route Reply to the initiator of this Route Request (the Source Address in the IP header of the packet) (...)"

2)Page 67, first bullet:
"Else, the node MUST examine the route recorded in the Route Request option (the IP Source Address field and the sequence of Address[i] fields) to determine if this node's own IP address already appears in this list of addresses."

Inside DsrOptionRreq::Process method a first "if statement" checks whether currently processed RREQ was sent by the same node, but with current way of forwarding RREQs condition is never "true":
"
  Ipv4Address srcAddress = ipv4Header.GetSource ();
/*
 * \ when the ip source address is equal to the address of our own, this is request packet originated
 * \ by the node itself, discard it
 */
  if (srcAddress == ipv4Address)
    {
      NS_LOG_DEBUG ("Discard the packet");
      return 0;
    }
"
During simulation of manet using dsr module a strange behaviour occurred. Packet-sink and onoff applications were installed on nodes with "Rx" and "Tx" trace sources attached to sinks. Pcap tracing on wifi phy layer was also enabled on nodes. Analysis of data packets in pcap files and output of "Rx", "Tx" trace sources showed, that number of packets received by some nodes was bigger than number of packets sent by corresponding nodes (each node with onoff application had one node with packet-sink application assigned). Even in pcap files from wifi phy layer, after counting data packets with unique ip.id fields, the number of such packets was bigger than the number of packets sent to the node.

After changing the code, as it is proposed in the attachment, the number of data packets with unique ip.id received by node is always <= than number of data packets sent by proper, corresponding node.
Comment 1 Tomasz Seweryn 2014-04-02 20:45:17 EDT
Created attachment 1813 [details]
Attachment with proper directions of diff
Comment 2 Yufei Cheng 2014-04-05 10:43:32 EDT
I think the implementation decision back then was that we cannot modify the IP source address so that we have added fields in dsr header, "source ID" and "destination ID" which are the real source address and destination address for the source routing packet in dsr.

(In reply to Tomasz Seweryn from comment #1)
> Created attachment 1813 [details]
> Attachment with proper directions of diff
Comment 3 Tomasz Seweryn 2014-04-05 17:31:40 EDT
(In reply to Yufei Cheng from comment #2)
> I think the implementation decision back then was that we cannot modify the
> IP source address so that we have added fields in dsr header, "source ID"
> and "destination ID" which are the real source address and destination
> address for the source routing packet in dsr.
> 
> (In reply to Tomasz Seweryn from comment #1)
> > Created attachment 1813 [details]
> > Attachment with proper directions of diff

Ok, but in current code you modify the IP header source address every time the packet is forwarded by every node. The way of the packet is like this: DsrRouting::ScheduleInterRequest (here destination address is set every time to m_mainAddress) -> DsrRouting::SendRequest -> Ipv4L3Protocol::Send (here every time a new IP header is created with given source and destination by higher DSR layer).

If this works this way, then if node A broadcasts a packet and node B receives it and forwards it, node B sets IP header's source to node B's own address and when the packet gets back now to node A the first "if" statement in DsrOptionRreq::Process method is false, though it should be true, as this is the packet originally sent by the node A.
Comment 4 Yufei Cheng 2014-04-07 00:26:41 EDT
I get what you are saying now, you are absolutely right on this one.  I wish I have used better naming for all the variables.  I'm working on one patch now and should be able to put it up in a couple of days.


(In reply to Tomasz Seweryn from comment #3)
> (In reply to Yufei Cheng from comment #2)
> > I think the implementation decision back then was that we cannot modify the
> > IP source address so that we have added fields in dsr header, "source ID"
> > and "destination ID" which are the real source address and destination
> > address for the source routing packet in dsr.
> > 
> > (In reply to Tomasz Seweryn from comment #1)
> > > Created attachment 1813 [details]
> > > Attachment with proper directions of diff
> 
> Ok, but in current code you modify the IP header source address every time
> the packet is forwarded by every node. The way of the packet is like this:
> DsrRouting::ScheduleInterRequest (here destination address is set every time
> to m_mainAddress) -> DsrRouting::SendRequest -> Ipv4L3Protocol::Send (here
> every time a new IP header is created with given source and destination by
> higher DSR layer).
> 
> If this works this way, then if node A broadcasts a packet and node B
> receives it and forwards it, node B sets IP header's source to node B's own
> address and when the packet gets back now to node A the first "if" statement
> in DsrOptionRreq::Process method is false, though it should be true, as this
> is the packet originally sent by the node A.
Comment 5 Yufei Cheng 2014-04-07 18:44:09 EDT
Created attachment 1815 [details]
Patch for the IP source header check for the rreq message
Comment 6 Tom Henderson 2014-04-13 14:22:30 EDT
Yufei, can you please advise which patch is ready for commit to ns-3-dev at this time, and if the bug can then be closed?
Comment 7 Tomasz Seweryn 2014-04-13 15:26:33 EDT
Does it matter for other layers in the simulator if routing layer changes the IP header source address while forwarding the packet? If not, then my patch is not needed. Yufei's patch also fixes Bug 1872 and adds some functionality, so definitely it should be applied, but what about my fix for Bug 1872? It is shorter than Yufei's and logic stays the same. There is also one variable not used, which is left in Yufei's patch and it makes the code to not to compile.
Comment 8 Tommaso Pecorella 2014-04-13 15:32:21 EDT
(In reply to Tomasz Seweryn from comment #7)
> Does it matter for other layers in the simulator if routing layer changes
> the IP header source address while forwarding the packet? 

Yes and no.

No, provided that the original address is restored before the packet reaches the destination.
Yes, because an intermediate node could want to check some checksums (think to IPSec)

> If not, then my
> patch is not needed. Yufei's patch also fixes Bug 1872 and adds some
> functionality, so definitely it should be applied, but what about my fix for
> Bug 1872? It is shorter than Yufei's and logic stays the same. There is also
> one variable not used, which is left in Yufei's patch and it makes the code
> to not to compile.
Comment 9 Tomasz Seweryn 2014-04-18 06:38:20 EDT
Ok. Yufei, your patch fixes source address checking for DSR protocol but IP header source address is still modified when forwarding RREQ packets. It has no influence on DSR but it can be important for other parts of simulator. At least leaving this case without my fix will make other programmers confused in future, because of different behaviour of the protocol than it should be.
Comment 10 Tommaso Pecorella 2014-04-19 17:38:46 EDT
(In reply to Tomasz Seweryn from comment #9)
> Ok. Yufei, your patch fixes source address checking for DSR protocol but IP
> header source address is still modified when forwarding RREQ packets. It has
> no influence on DSR but it can be important for other parts of simulator. At
> least leaving this case without my fix will make other programmers confused
> in future, because of different behaviour of the protocol than it should be.

I (partially) agree with Tomasz, but I think the point is another one.
ns-3 DSR isn't RFC 4728 DSR.

Yufei's patch fixes one of the issues, but the main one remains. The protocol implemented in ns-3 is close to the RFC, but it isn't the RFC version.
It's out of scope to discuss if the implemented version is "better" or not, it's just not compliant with the standard.

Two simple examples:
/**
* \ingroup dsr
* \brief Dsr fixed size header Format
  \verbatim
   |      0        |      1        |      2        |      3        |
   0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |  Next Header |F|     Reservd    |       Payload Length       |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                            Options                           |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  \endverbatim
*/

/**
* \ingroup dsr
* \brief The modified version of Dsr fixed size header Format
  \verbatim
   |      0        |      1        |      2        |      3        |
   0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |  Next Header |F|  Message Type  |       Payload Length       |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |             Source Id           |            Dest Id         |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                            Options                           |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  \endverbatim
*/

The above one is the standard one. The one below is what's implemented in ns-3.

RREQ:
      Source Address

         MUST be set to the address of the node originating this packet.
         Intermediate nodes that retransmit the packet to propagate the
         Route Request MUST NOT change this field.

In the ns-3 implementation the source address is changed.

They are simply different.

In my opinion there are two things to do.
1) point out in the documentation that DSR is *based on* RFC 4728, but it's not an exact implementation. Let's call it "enhanced", perhaps.
2) open an enhancement bug to allow a "strict" mode and an "enhanced" mode. "Strict" mode should follow the RFC... strictly.
Comment 11 Tom Henderson 2014-04-20 13:30:38 EDT
(In reply to Tommaso Pecorella from comment #10)
> (In reply to Tomasz Seweryn from comment #9)
> > Ok. Yufei, your patch fixes source address checking for DSR protocol but IP
> > header source address is still modified when forwarding RREQ packets. It has
> > no influence on DSR but it can be important for other parts of simulator. At
> > least leaving this case without my fix will make other programmers confused
> > in future, because of different behaviour of the protocol than it should be.
> 
> I (partially) agree with Tomasz, but I think the point is another one.
> ns-3 DSR isn't RFC 4728 DSR.
> 
> Yufei's patch fixes one of the issues, but the main one remains. The
> protocol implemented in ns-3 is close to the RFC, but it isn't the RFC
> version.
> It's out of scope to discuss if the implemented version is "better" or not,
> it's just not compliant with the standard.
> 
> Two simple examples:
> /**
> * \ingroup dsr
> * \brief Dsr fixed size header Format
>   \verbatim
>    |      0        |      1        |      2        |      3        |
>    0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |  Next Header |F|     Reservd    |       Payload Length       |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                            Options                           |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>   \endverbatim
> */
> 
> /**
> * \ingroup dsr
> * \brief The modified version of Dsr fixed size header Format
>   \verbatim
>    |      0        |      1        |      2        |      3        |
>    0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |  Next Header |F|  Message Type  |       Payload Length       |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |             Source Id           |            Dest Id         |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                            Options                           |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>   \endverbatim
> */

Let's not lose this in the tracker (unapplied to the real documentation when this bug is closed).

> 
> The above one is the standard one. The one below is what's implemented in
> ns-3.
> 
> RREQ:
>       Source Address
> 
>          MUST be set to the address of the node originating this packet.
>          Intermediate nodes that retransmit the packet to propagate the
>          Route Request MUST NOT change this field.
> 
> In the ns-3 implementation the source address is changed.
> 
> They are simply different.
> 
> In my opinion there are two things to do.
> 1) point out in the documentation that DSR is *based on* RFC 4728, but it's
> not an exact implementation. Let's call it "enhanced", perhaps.

Agreed; this should be done in the class DsrRouting doxygen header, and also in the .rst documentation in this section (could be clarified further).

This section:
http://www.nsnam.org/docs/release/3.19/models/html/dsr.html#implementation-modification

could be renamed more explicitly to "caveats" or "differences from RFC" and perhaps edited.  Tomasz, can you suggest a patch to this section of the documentation, based on your recent working experience with it?

> 2) open an enhancement bug to allow a "strict" mode and an "enhanced" mode.
> "Strict" mode should follow the RFC... strictly.

Or add an entry here:
http://www.nsnam.org/wiki/Project_Ideas

and point back to this tracker issue.

If there is no one likely to work on this in the near term (i.e. if Tomasz does not plan to do it) I would suggest to close the bug and log this extension on the wiki, to cut down on longstanding open bugs.
Comment 12 Tommaso Pecorella 2014-04-20 13:50:09 EDT
(In reply to Tom Henderson from comment #11)
> (In reply to Tommaso Pecorella from comment #10)
> > 2) open an enhancement bug to allow a "strict" mode and an "enhanced" mode.
> > "Strict" mode should follow the RFC... strictly.
> 
> Or add an entry here:
> http://www.nsnam.org/wiki/Project_Ideas
> 
> and point back to this tracker issue.

I added the entry:
http://www.nsnam.org/wiki/Project_Ideas#DSR_RFC_compliance
Comment 13 Tomasz Seweryn 2014-04-22 19:51:12 EDT
To comment this:
(In reply to Tommaso Pecorella from comment #10)
> 
> RREQ:
>       Source Address
> 
>          MUST be set to the address of the node originating this packet.
>          Intermediate nodes that retransmit the packet to propagate the
>          Route Request MUST NOT change this field.
> 
> In the ns-3 implementation the source address is changed.
> 
I found out today why this IP header Source Address is changed while forwarding RREQ broadcast packets. In DsrRouting::SendErrorRequest method, when there is no route to the error destination in the node route cache, a RREQ with Unreach error appended is sent. When this error is served inside DsrOptionRreq::Process a following statement is checked:

"if ((errorSrc == srcAddress) && (unreachNode == ipv4Address))"

In Yufei's last patch srcAddress is changed to source, but in this place it shouldn't, as Yufei predicted. The code above describes situation when neighbour (unreachNode) of node with errorSrc address receives this Unreach error directly from errorSrc. errorSrc == source will be always true, while errorSrc == srcAddress will not. This is important, because inside this "if" statement code stops flooding network with Unreach error option appended to the RREQ.

> 2) open an enhancement bug to allow a "strict" mode and an "enhanced" mode.
> "Strict" mode should follow the RFC... strictly.

Isn't the current ns-3 DSR code different than RFC because the specifics of the simulator imposed it?
Comment 14 Tommaso Pecorella 2014-04-23 02:18:07 EDT
(In reply to Tomasz Seweryn from comment #13)
> To comment this:
> (In reply to Tommaso Pecorella from comment #10)
> > 
> > RREQ:
> >       Source Address
> > 
> >          MUST be set to the address of the node originating this packet.
> >          Intermediate nodes that retransmit the packet to propagate the
> >          Route Request MUST NOT change this field.
> > 
> > In the ns-3 implementation the source address is changed.
> > 
> I found out today why this IP header Source Address is changed while
> forwarding RREQ broadcast packets. In DsrRouting::SendErrorRequest method,
> when there is no route to the error destination in the node route cache, a
> RREQ with Unreach error appended is sent. When this error is served inside
> DsrOptionRreq::Process a following statement is checked:
> 
> "if ((errorSrc == srcAddress) && (unreachNode == ipv4Address))"
> 
> In Yufei's last patch srcAddress is changed to source, but in this place it
> shouldn't, as Yufei predicted. The code above describes situation when
> neighbour (unreachNode) of node with errorSrc address receives this Unreach
> error directly from errorSrc. errorSrc == source will be always true, while
> errorSrc == srcAddress will not. This is important, because inside this "if"
> statement code stops flooding network with Unreach error option appended to
> the RREQ.

Mmm.... could you suggest a patch ?

> > 2) open an enhancement bug to allow a "strict" mode and an "enhanced" mode.
> > "Strict" mode should follow the RFC... strictly.
> 
> Isn't the current ns-3 DSR code different than RFC because the specifics of
> the simulator imposed it?

Not at all. The simulator doesn't "impose" you anything. It may be difficult to follow the RFC, but it's always possible. Moreover, the difficulties in implementing for ns-3 and for a real system are more or less the same.
E.g., "a forwarding node should process packets with dst different then its own". Same difficulty (you have to use promiscuous sockets).
Bending a specification because it's hard to implement means either:
1) you're lazy or
2) the specification is severely flawed.

In the specific case, I think that the implementation is "enhanced" to provide more functionalities.
Comment 15 Yufei Cheng 2014-04-23 10:46:52 EDT
My original email keeps getting bounced back, don't know why.

Regarding the changing source address for IP header, I agree with you guys that it should not be changed.  I think the problem comes from that when DSR is first being implemented, the callback "IpL4Protocol::DownTargetCallback" has not been implemented, and there is no way to send the packet without changing the IP source address.  The two extra fields "Source Id" and "Dest Id" you mentioned are used to remember the "real" source and destination address.  I should have removed them right after start using "DownTargetCallback".  

I would try my best to come up with a patch that fix this behavior, yet the source address change is too embedded in the whole route discovery operation.  Therefore, my guess for coming up with this patch will be a bit longer.  

In the mean time, the result for DSR is correct and my patch have fixed one more bug that I have recently found; the maintenance buffer has one map that used a struct as a map key, yet the map key comparison part is not working as expected.  

Thanks,
Yufei

(In reply to Tommaso Pecorella from comment #14)
> (In reply to Tomasz Seweryn from comment #13)
> > To comment this:
> > (In reply to Tommaso Pecorella from comment #10)
> > > 
> > > RREQ:
> > >       Source Address
> > > 
> > >          MUST be set to the address of the node originating this packet.
> > >          Intermediate nodes that retransmit the packet to propagate the
> > >          Route Request MUST NOT change this field.
> > > 
> > > In the ns-3 implementation the source address is changed.
> > > 
> > I found out today why this IP header Source Address is changed while
> > forwarding RREQ broadcast packets. In DsrRouting::SendErrorRequest method,
> > when there is no route to the error destination in the node route cache, a
> > RREQ with Unreach error appended is sent. When this error is served inside
> > DsrOptionRreq::Process a following statement is checked:
> > 
> > "if ((errorSrc == srcAddress) && (unreachNode == ipv4Address))"
> > 
> > In Yufei's last patch srcAddress is changed to source, but in this place it
> > shouldn't, as Yufei predicted. The code above describes situation when
> > neighbour (unreachNode) of node with errorSrc address receives this Unreach
> > error directly from errorSrc. errorSrc == source will be always true, while
> > errorSrc == srcAddress will not. This is important, because inside this "if"
> > statement code stops flooding network with Unreach error option appended to
> > the RREQ.
> 
> Mmm.... could you suggest a patch ?
> 
> > > 2) open an enhancement bug to allow a "strict" mode and an "enhanced" mode.
> > > "Strict" mode should follow the RFC... strictly.
> > 
> > Isn't the current ns-3 DSR code different than RFC because the specifics of
> > the simulator imposed it?
> 
> Not at all. The simulator doesn't "impose" you anything. It may be difficult
> to follow the RFC, but it's always possible. Moreover, the difficulties in
> implementing for ns-3 and for a real system are more or less the same.
> E.g., "a forwarding node should process packets with dst different then its
> own". Same difficulty (you have to use promiscuous sockets).
> Bending a specification because it's hard to implement means either:
> 1) you're lazy or
> 2) the specification is severely flawed.
> 
> In the specific case, I think that the implementation is "enhanced" to
> provide more functionalities.
Comment 16 Tommaso Pecorella 2014-05-02 10:38:54 EDT
changeset:   10774:1f47a73ab755