Bug 1895

Created attachment 1813 [details]
Attachment with proper directions of diff

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

(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.

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.

Created attachment 1815 [details]
Patch for the IP source header check for the rreq message

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?

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.

(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.

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.

(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.

(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.

(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

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?

(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.

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.

changeset:   10774:1f47a73ab755