Bug 1102 - IPv4 header doesn't handle correctly the fragment offset field
IPv4 header doesn't handle correctly the fragment offset field
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: internet
ns-3-dev
All All
: P5 normal
Assigned To: ns-bugs
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-17 21:18 EDT by Tommaso Pecorella
Modified: 2011-06-23 16:39 EDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tommaso Pecorella 2011-04-17 21:18:59 EDT
There is a division and a shift that are (to me) kinda flawed.

The end result is that, if you try to use it, the number you've entered in the code is changed in the received packet. Bad.

According to the RFCs the fragment offset is carried in the packet as a 8-bytes group (i.e., 1 means 8 bytes offset), however entering "1" leads to a zero in the packet.

Right now is a minor bug and kinda unimportant, since fragmentation isn't supported... yet.

Still, a bug is a bug.


http://codereview.appspot.com/4447049
Comment 1 John Abraham 2011-04-17 23:47:03 EDT
Hi,
Can you elaborate more with a code-snippet of how you entered "1". Did you use SetFragmentOffset ?
If you set a fragment offset of 1 using SetFragmentOffset (), the frag-offset that is serialized will to be "0" as you observed, ip fragmentation being zero-indexed with 8 byte chunks.

The author, intended to use SetFragmentOffset to take a non-serialized fragment offset (say 104) , divide it by 8 only during serializing (as 8b boundary normalization is purely a header serialization requirement), rather than set the fragment offset explicitly.

Fragmentation and reassembly may typically deal with the total bytes as a whole such as 104 rather than deal with the header 8b normalization.This is similar to linux kernel implementation of ip_output.c where frag offset normalization is done only just prior to serialization

545                                 offset += skb->len - hlen;
546                                 iph->frag_off = htons(offset>>3);

Does this explain your observation? 

-John
Comment 2 Tommaso Pecorella 2011-04-19 14:03:43 EDT
Hi,

mmm... maybe you're right, but then the bug is elsewhere... in the doxygen !

From the doxygen:
void ns3::Ipv4Header::SetFragmentOffset	(	uint16_t 	offset	)	
Parameters: offset	the ipv4 fragment offset

To me "the ipv4 fragment offset" is the **raw** number (13 bits) of the Fragment Offset field, and it has to be in 8-bytes units.

For the function (as it is) it is the offset in bytes.

If we want to use the second interpretation, then the function works as it is. Otherwise it have to be corrected.

In either case, the doxygen need a clearer explanation, as if we leave the function as it is (it's perfectly fine indeed), then a dumb user (me in this case) could try to fragment and set the offset to an impossible value, like 1234 (not a multiple of 8 bytes) or to the "right" unit but in 8-bytes units. And end up with rather strange results.

I honestly didn't even think to the bytes unit thing, as during fragmentation I keep the counter in 8-bytes units to make sure of having a "right aligned" fragment, so it was natural to me to pass that to the SetFragmentOffset.

Anyway, a documented bug is a feature. Let's update the doxygen and consider this as a feature.

BTW; the Ipv4Header::Print will show the offset in 8-bytes units, this might as well be confusing... if someone doesn't know what he/she is doing. On the other hand we can safely assume that anyone willing to look at those fields should have some IP header field knowledge, isn't it ?

I'll close the bug and I'll add some doxygen clarification in the upcoming IPv4 fragmentation patch. Right now SetFragmentOffset isn't used by anyone anyway.


Cheers,

T.
Comment 3 Tommaso Pecorella 2011-04-23 07:44:15 EDT
Not *exactly" the same issue, but very close.

In the Ipv4Header the fragment offset is defined as:
  uint16_t m_fragmentOffset : 13;

The two accessors
  void SetFragmentOffset (uint16_t offset);
and
  uint16_t GetFragmentOffset (void) const;
"simply" do an access to that. There are also two asserts to ensure that it is 13-bits long, and they're both wrong as they check for 14 bits. DOH.

HOWEVER Serialize and Deserialize do "think" that m_fragmentOffset is measured in bytes.

*** something wrong somewhere ***

Solution 1: apply my previous patch.

Solution 2: m_fragmentOffset becomes a "plain" uint16_t and we remove the asserts (wrong in any case, should had checked against 0x1fff).

Solution 2 is the less intrusive as it restores the inner logic.
Comment 4 Tom Henderson 2011-06-13 01:21:05 EDT
(In reply to comment #2)
> Hi,
> 
> mmm... maybe you're right, but then the bug is elsewhere... in the doxygen !
> 
> From the doxygen:
> void ns3::Ipv4Header::SetFragmentOffset    (    uint16_t     offset    )    
> Parameters: offset    the ipv4 fragment offset
> 
> To me "the ipv4 fragment offset" is the **raw** number (13 bits) of the
> Fragment Offset field, and it has to be in 8-bytes units.
> 
> For the function (as it is) it is the offset in bytes.


My read of assert code there is that it was intended to have units of 8-byte blocks with a maximal value of 2^13 -1.  As a user, I would expect these methods to have those units.  I would vote for correcting the method but leave the offset semantics being eight-byte blocks.
Comment 5 Tommaso Pecorella 2011-06-13 05:54:49 EDT
(In reply to comment #4)
> (In reply to comment #2)
> > Hi,
> > 
> > mmm... maybe you're right, but then the bug is elsewhere... in the doxygen !
> > 
> > From the doxygen:
> > void ns3::Ipv4Header::SetFragmentOffset    (    uint16_t     offset    )    
> > Parameters: offset    the ipv4 fragment offset
> > 
> > To me "the ipv4 fragment offset" is the **raw** number (13 bits) of the
> > Fragment Offset field, and it has to be in 8-bytes units.
> > 
> > For the function (as it is) it is the offset in bytes.
> 
> 
> My read of assert code there is that it was intended to have units of 8-byte
> blocks with a maximal value of 2^13 -1.  As a user, I would expect these
> methods to have those units.  I would vote for correcting the method but leave
> the offset semantics being eight-byte blocks.

Either way is fine. It just affects my IPv4 fragmentation implementation anyway (or some undocumented and unknown user code, but I don't think anyone ever used those functions).

John ?

This week I'm traveling, but I'll prepare two patches next week, one for the 1st, one for the 2nd case.

Cheers,


Tommaso
Comment 6 John Abraham 2011-06-13 09:03:45 EDT
Hi Tomasso,Tom,
A few things to think about:

which layer will need to use "GetFragmentOffset" and "SetFragmentOffset"?
In my opinion, this is typically used by the logic found in "Fragmentation & Reassembly" module, which is a layer 3 module.

If layer 4 and above (say a callback in some other layer) accesses these functions, they could be considered "hacks". And I think ns-3 is designed for the routine cases rather than provide a way for users to hack normal work-flows.Users can hack ipv4 headers at their own risks.

Therefore, the "Frag/Reass" module need not know about header normalization. That is, we can leave the normalization to the serializer. As far a frag/reass is concerned, it should always have a byte stream.  In that sense, Serializer must continue to think that
m_fragmentOffset is not normalized (this is the case currently, however the ASSERTS cause confusion).

However, Set/GetFragmentOffset which may be used typically by the Frag/Reass module, should deal with non-normalized offsets (which means the ASSERTS need to be removed)

Note as I mentioned earlier,
8 b header normalization happens right when serialization has to happen, and never before that.

http://www.cs.fsu.edu/~baker/devices/lxr/http/source/linux/net/ipv4/ip_output.c

521                         /* Prepare header of the next frame,
522                          * before previous one went down. */
523                         if (frag) {
524                                 frag->ip_summed = CHECKSUM_NONE;
525                                 skb_reset_transport_header(frag);
526                                 __skb_push(frag, hlen);
527                                 skb_reset_network_header(frag);
528                                 memcpy(skb_network_header(frag), iph, hlen);
529                                 iph = ip_hdr(frag);
530                                 iph->tot_len = htons(frag->len);
531                                 ip_copy_metadata(frag, skb);
532                                 if (offset == 0)
533                                         ip_options_fragment(frag);
534                                 offset += skb->len - hlen;
535                                 iph->frag_off = htons(offset>>3); <<<<<<<<<<<<<<<<< 8 b  normalization only right before checksum.
536                                 if (frag->next != NULL)
537                                         iph->frag_off |= htons(IP_MF);
538                                 /* Ready, complete checksum */
539                                 ip_send_check(iph);
540                         }
541 

If the above approach is taken we may need to ASSERT when  (((fragment offset)*8) - 1 + Ipv4 hdr size) > 65535 (Max IPV4 packet size).

This is just my opinion, either way is fine if properly documented. 

Or we can also replace the functions with the unambiguous

GetEightByteBlockFragmentOffset ()
GetByteBlockFragmentOffset ()

-John




(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > Hi,
> > > 
> > > mmm... maybe you're right, but then the bug is elsewhere... in the doxygen !
> > > 
> > > From the doxygen:
> > > void ns3::Ipv4Header::SetFragmentOffset    (    uint16_t     offset    )    
> > > Parameters: offset    the ipv4 fragment offset
> > > 
> > > To me "the ipv4 fragment offset" is the **raw** number (13 bits) of the
> > > Fragment Offset field, and it has to be in 8-bytes units.
> > > 
> > > For the function (as it is) it is the offset in bytes.
> > 
> > 
> > My read of assert code there is that it was intended to have units of 8-byte
> > blocks with a maximal value of 2^13 -1.  As a user, I would expect these
> > methods to have those units.  I would vote for correcting the method but leave
> > the offset semantics being eight-byte blocks.
> 
> Either way is fine. It just affects my IPv4 fragmentation implementation anyway
> (or some undocumented and unknown user code, but I don't think anyone ever used
> those functions).
> 
> John ?
> 
> This week I'm traveling, but I'll prepare two patches next week, one for the
> 1st, one for the 2nd case.
> 
> Cheers,
> 
> 
> Tommaso
Comment 7 Tom Henderson 2011-06-13 09:43:54 EDT
(In reply to comment #6)
> Hi Tomasso,Tom,
> A few things to think about:
> 
> which layer will need to use "GetFragmentOffset" and "SetFragmentOffset"?
> In my opinion, this is typically used by the logic found in "Fragmentation &
> Reassembly" module, which is a layer 3 module.

I don't think we can make assumptions about who is using this API and what are their intentions; it is a public header and I can easily imagine users outside of the stack handling this.  Since the methods explicitly use the name of this header field, I would expect the units to match, unless documented otherwise.

> 535                                 iph->frag_off = htons(offset>>3);

I guess we look at it in different ways.  I equate Ipv4Header::SetFragmentOffset() to the above assignment, which takes the normalized value.  Whether or not the IP stack maintains this offset in bytes or in blocks is outside the scope of this header class.   

> 
> This is just my opinion, either way is fine if properly documented. 

Same here.
Comment 8 John Abraham 2011-06-13 09:51:02 EDT
Also please consider using unambiguous identifiers for instance,

GetEightByteBlockFragmentOffset ()
GetOneByteBlockFragmentOffset ()

if we want this API to be used by "ANY" module and not restricted to Frag/Reass

-John

(In reply to comment #7)
> (In reply to comment #6)
> > Hi Tomasso,Tom,
> > A few things to think about:
> > 
> > which layer will need to use "GetFragmentOffset" and "SetFragmentOffset"?
> > In my opinion, this is typically used by the logic found in "Fragmentation &
> > Reassembly" module, which is a layer 3 module.
> 
> I don't think we can make assumptions about who is using this API and what are
> their intentions; it is a public header and I can easily imagine users outside
> of the stack handling this.  Since the methods explicitly use the name of this
> header field, I would expect the units to match, unless documented otherwise.
> 
> > 535                                 iph->frag_off = htons(offset>>3);
> 
> I guess we look at it in different ways.  I equate
> Ipv4Header::SetFragmentOffset() to the above assignment, which takes the
> normalized value.  Whether or not the IP stack maintains this offset in bytes
> or in blocks is outside the scope of this header class.   
> 
> > 
> > This is just my opinion, either way is fine if properly documented. 
> 
> Same here.
Comment 9 Tom Henderson 2011-06-13 11:35:38 EDT
(In reply to comment #8)
> Also please consider using unambiguous identifiers for instance,
> 
> GetEightByteBlockFragmentOffset ()
> GetOneByteBlockFragmentOffset ()
> 
> if we want this API to be used by "ANY" module and not restricted to Frag/Reass
> 
> -John

I feel that in this case, it would be sufficient to just document what is returned in the doxygen, rather than embed it in the method name.  For the SetFragmentOffset, parameter name might be leveraged such as:

Ipv4Header::SetFragmentOffset (uint16_t offsetBlocks)
or
Ipv4Header::SetFragmentOffset (uint16_t offsetBytes)

again, just my subjective opinion on this point.
Comment 10 Tommaso Pecorella 2011-06-14 10:02:26 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > Also please consider using unambiguous identifiers for instance,
> > 
> > GetEightByteBlockFragmentOffset ()
> > GetOneByteBlockFragmentOffset ()
> > 
> > if we want this API to be used by "ANY" module and not restricted to Frag/Reass
> > 
> > -John
> 
> I feel that in this case, it would be sufficient to just document what is
> returned in the doxygen, rather than embed it in the method name.  For the
> SetFragmentOffset, parameter name might be leveraged such as:
> 
> Ipv4Header::SetFragmentOffset (uint16_t offsetBlocks)
> or
> Ipv4Header::SetFragmentOffset (uint16_t offsetBytes)
> 
> again, just my subjective opinion on this point.

I do agree with Tom, mainly because no matter what we will use, the fragment offset will be sooner or later converted ion blocks. Having two different methods will for sure raise questions from the dumb user (and you know there's always one around).

Let's just stick to one and document it for good.

About what to use, I don't care much about Linux implementation since it is not ment to be used outside its scope. Plus it's clear from the code that Linux choices are also related with the way the segments are handled for reassembly, so a rather implementation-specific choice.

Anyway, I'd stick to the bytes-units in the API, purely because the actual (bugged) code is doing so.

As I said, right now I'm abroad, but I'll try to post a patch later.

Cheers, 
Tommaso
Comment 11 Tommaso Pecorella 2011-06-15 08:14:54 EDT
Check and comment it please.

http://codereview.appspot.com/4608045/

The assert is still there (and I'd leave it there) to avoid the usual dumb user to try to set a fragmentation offset that is not a multiple of 8 bytes.

Rationale is: these functions are *not* meant to be used with arbitrary offsets, otherwise the user would have to modify the way the serialize is working (forcing a different IP behavior), and then they can as well remove the asserts.

Please comment the code directly there.

Again, this is only one of the many ways to fix this, but if we keep talking about them, we'll never fix it.

Cheers,


T.