Bug 1817

Summary: IPv4 Identification field should consider protocol as well.
Product: ns-3 Reporter: Tommaso Pecorella <tommaso.pecorella>
Component: internetAssignee: George Riley <riley>
Status: RESOLVED FIXED    
Severity: normal CC: ns-bugs, tomh
Priority: P5    
Version: ns-3-dev   
Hardware: All   
OS: All   
Bug Depends on: 1808    
Bug Blocks:    
Attachments: patch
new patch
patch + fresh regression pcaps

Description Tommaso Pecorella 2013-12-24 10:41:16 EST
Created attachment 1746 [details]
patch

RFC 6864 clearly states

   >> The IPv4 ID field MUST NOT be used for purposes other than
      fragmentation and reassembly.

The proposed patch fixes that.

A further analysis is needed to enforce the ID uniqueness within the maximum datagram lifetime (MDL).
Comment 1 Tommaso Pecorella 2013-12-24 10:58:02 EST
BTW, this patch will require the regressions tests to be rebuilt.

Also, I have no idea id DSR is affected by this. DsrOptionSR::Process() is using the IP header ID field, but it should be a safe use.
Comment 2 Tom Henderson 2014-01-13 01:41:16 EST
(In reply to Tommaso Pecorella from comment #0)
> Created attachment 1746 [details]
> patch
> 
> RFC 6864 clearly states
> 
>    >> The IPv4 ID field MUST NOT be used for purposes other than
>       fragmentation and reassembly.
> 
> The proposed patch fixes that.

I'm not sure that I agree with this interpretation (that this patch fixes ID assignment).  The statement above pertains to using (not generating) IPv4 ID field (as it using it for things like duplicate packet detection).

RFC 6864 specifies that the ID should be set to a unique value for packets that may be fragmented in the future.  It also says that it doesn't care about what value is assigned to packets that can't be fragmented.  Note that it doesn't complain if the same process is used for both types of packets.

The current code (monotonically increasing sequence number for all) seems to already loosely follow this guidance.  It has two limitations:
- it burns sequence numbers on packets that don't need a unique ID
- it does not track on a per-session (src, dst, protocol) basis, but instead for all datagrams, thereby causing cycling of the 16-bit space more quickly than necessary

However, I don't see how the proposed patch improves upon this; if anything, it makes it harder to track uniqueness properties.

> 
> A further analysis is needed to enforce the ID uniqueness within the maximum
> datagram lifetime (MDL).

It seems that a monotonically increasing number is easier to enforce this than a random number.

Can you produce a failing test case of fragmentation due to IP ID generation problems (that the patch then fixes)?   Otherwise, I might suggest to mark as WONTFIX or else work on an implementation that tracks uniqueness separately across (src,dst,proto) combinations.
Comment 3 Tommaso Pecorella 2014-01-14 16:56:01 EST
After re-reading carefully RFC 6864, I agree.

   >> Sources emitting non-atomic datagrams MUST NOT repeat IPv4 ID
      values within one MDL for a given source address/destination
      address/protocol tuple.

As a result, the patch is invalid, and a more elaborate patch is needed.
Comment 4 Tommaso Pecorella 2014-01-14 17:24:14 EST
Created attachment 1762 [details]
new patch

It turns out that the patch is far easier than expected :)

A map is, by default, returning zero for a non-existent key. I love maps.
Comment 5 Tommaso Pecorella 2014-04-26 17:44:05 EDT
Created attachment 1830 [details]
patch + fresh regression pcaps

This patch includes fresh pcaps for AODV and OLSR. The tests were failing due to the different IP id numbers.
Comment 6 Tommaso Pecorella 2014-04-29 04:08:18 EDT
changeset:   10707:bc08a6c3350b