Fragmentation support for IPv4

IMPORTANT
ARP prevents packets longer than 3 fragments to be delivered unless the ARP cache is already valid. This is (so far) working as intended.

[Tommaso Pecorella, 2011-04-21 11:24:02]

The tentative is not poor anymore.

I completed the fragmentation support (assuming I haven't forgot anything) and
it should be ready for evaluation.

Feel free to check it and comment at will.

Cheers,


Tommaso



On 21/apr/2011, at 02.57, tommypec@gmail.com wrote:

> Reviewers: ,
> 
> Description:
> The bug here seems related to this:
> 
> http://www.nsnam.org/bugzilla/show_bug.cgi?id=1072
> 
> 
> 
> Please review this at http://codereview.appspot.com/4440051/
> 
> Affected files:
>  A scratch/udp-echo-fragm.cc
>  M src/internet/model/ipv4-l3-protocol.cc
>  M src/internet/model/ipv4-l3-protocol.h
>  M src/network/model/packet-metadata.cc
> 
> 

[Tom Henderson, 2011-04-22 21:29:49]

My main comment is that it would be nice to have a few unit/regression tests,
and to reconcile with Vedran's implementation.  I noticed that Vedran has some
tests already written.

it would be nice if you and Vedran could review each other's approach and decide
on the best way to handle it, or whether they can be merged somehow.  One small
benefit I see on this approach vs. Vedran's is that Vedran's changed the API
slightly (SendOut vs. SendRealOut)

http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc
File scratch/udp-echo-fragm.cc (right):

http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc#new...
scratch/udp-echo-fragm.cc:26: // - Tracing of queues and packet receptions to
file "udp-echo.tr"
It would be nice to turn this into a simple unit test instead of an example. 
e.g. nodes n0 <-> n1 interconnected by SimpleNetDevice with MTU of 1500, and
send IPv4 packet sizes of e.g. 0, 1000, 1500, 1501, 3000, 3001, 65535.

Then, test fragmentation reordering and loss of certain fragments, and timeout
behavior.

http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc#new...
scratch/udp-echo-fragm.cc:117: // workaround: make sure the ARP cache is valid
by sending first a small packet.
Or, bump up the PendingQueueSize in the ArpCache from its default of 3.

http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc#new...
scratch/udp-echo-fragm.cc:120: // Honestly, don't try.
That is only for IPv6 when Jumbo Payload is implemented.  Plus, UDP and TCP must
be changed to support it. However, I would expect that we ought to support
(test) a 65535-byte IPv4 and IPv6 datagram.

http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
File src/internet/model/ipv4-l3-protocol.cc (right):

http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
src/internet/model/ipv4-l3-protocol.cc:738: uint32_t fragmentSize =
outInterface->GetDevice ()->GetMtu () & ~uint32_t (0x7);
suggest a comment on the above cryptic line

http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
src/internet/model/ipv4-l3-protocol.cc:1153: // a much more complex handling is
necessary in case there are options.
can you check for options here, and assert if present? (and request in the
comment for someone to provide a patch to support it)

playing with IPv4 options is not outside the realm of possibility for a research
simulator, so we shouldn't lull the user into believing that it is supported.

http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
src/internet/model/ipv4-l3-protocol.cc:1320: // XXX
is this an XXX, or is it just an observation that this implementation does not
overwrite or otherwise look for fragmentation attacks?

[Tommaso Pecorella, 2011-04-22 22:31:57]

On 2011/04/22 21:29:49, Tom Henderson wrote:
> My main comment is that it would be nice to have a few unit/regression tests,
> and to reconcile with Vedran's implementation.  I noticed that Vedran has some
> tests already written.

You're right about unit testing. About Vedran's implementation I'm in contact
with him and he checked my implementation. We developed separately (my fault, I
noticed Vedran's one when I had mine almost complete) but I'll take a deeper
look at hi to check if there is some stuff to reconcile.

> it would be nice if you and Vedran could review each other's approach and
decide
> on the best way to handle it, or whether they can be merged somehow.  One
small
> benefit I see on this approach vs. Vedran's is that Vedran's changed the API
> slightly (SendOut vs. SendRealOut)

Vedran already checked this, I asked him to become a reviewer for this. I
wouldn't mind either if we adopt Vedran's one, the main goal is to have
fragmentation support, not who's the author.

> http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc
> File scratch/udp-echo-fragm.cc (right):
> 
>
http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc#new...
> scratch/udp-echo-fragm.cc:26: // - Tracing of queues and packet receptions to
> file "udp-echo.tr"
> It would be nice to turn this into a simple unit test instead of an example. 
> [...]

Will do.
 
>
http://codereview.appspot.com/4440051/diff/6002/scratch/udp-echo-fragm.cc#new...
> scratch/udp-echo-fragm.cc:117: // workaround: make sure the ARP cache is valid
> by sending first a small packet.
> Or, bump up the PendingQueueSize in the ArpCache from its default of 3.

Nice tip for the unit test.

>
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
> File src/internet/model/ipv4-l3-protocol.cc (right):
> 
>
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
> src/internet/model/ipv4-l3-protocol.cc:738: uint32_t fragmentSize =
> outInterface->GetDevice ()->GetMtu () & ~uint32_t (0x7);
> suggest a comment on the above cryptic line

Yep. Fragments must be 8-bytes aligned except for the last one.
 
>
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
> src/internet/model/ipv4-l3-protocol.cc:1153: // a much more complex handling
is
> necessary in case there are options.
> can you check for options here, and assert if present? (and request in the
> comment for someone to provide a patch to support it)
> 
> playing with IPv4 options is not outside the realm of possibility for a
research
> simulator, so we shouldn't lull the user into believing that it is supported.

Mmm... right. Ns-3 actually don't have IPv4 option headers (so I can't test any
support either), but it MUST be clear that if anything will be implemented, this
code have to be changed as well.

Maybe I could add a comment about this in the Ipv4Header documentation ?


>
http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
> src/internet/model/ipv4-l3-protocol.cc:1320: // XXX
> is this an XXX, or is it just an observation that this implementation does not
> overwrite or otherwise look for fragmentation attacks?

XXX is a refuso. The comment is about a mismatch between this implementation and
fragmentation attack vulnerable IP stacks (as is most of IP stacks). The
difference arises from the actual implementation, as in kernels it's used a
circular buffer where the newer fragment overwrite the oldest one.
On the other hand implementing that in ns-3 is not necessary imho and it would
make the implementation way more complex. The packet would had to be reunited on
the fly when a fragment arrives and the newer fragment data would had to
overwrite the packet's content. Hellish and unnecessary.
Again, a matter of documenting the fact that this implementation does not allow
those kind of tricks.
Maybe more a matter of doxygen than anything else.

Cheers,

T.

[Tommaso Pecorella, 2011-04-23 00:52:31]

http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
File src/internet/model/ipv4-l3-protocol.cc (right):

http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
src/internet/model/ipv4-l3-protocol.cc:738: uint32_t fragmentSize =
outInterface->GetDevice ()->GetMtu () & ~uint32_t (0x7);
On 2011/04/22 21:29:49, Tom Henderson wrote:
> suggest a comment on the above cryptic line

Removed the line, unnecessary after some changes.

Done.

http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
src/internet/model/ipv4-l3-protocol.cc:1153: // a much more complex handling is
necessary in case there are options.
On 2011/04/22 21:29:49, Tom Henderson wrote:
> can you check for options here, and assert if present? (and request in the
> comment for someone to provide a patch to support it)

Can't do any assert as Options are not in ns3 (can't check if UDP or TCP header
are present as the user might want to use raw IP).

> playing with IPv4 options is not outside the realm of possibility for a
research
> simulator, so we shouldn't lull the user into believing that it is supported.

A comment in the doxygen has been made.

http://codereview.appspot.com/4440051/diff/6002/src/internet/model/ipv4-l3-pr...
src/internet/model/ipv4-l3-protocol.cc:1320: // XXX
On 2011/04/22 21:29:49, Tom Henderson wrote:
> is this an XXX, or is it just an observation that this implementation does not
> overwrite or otherwise look for fragmentation attacks?

Worked out

[Tommaso Pecorella, 2011-04-24 14:16:05]

New code (ironed out in some parts) and new bugs (found and killed ofc).

I had to patch ipv4-header (again) due to a bug. This time is not a matter of
API misinterpretation, it's a real one. I love zero-day bugs hiding for
centuries in never-used code bits.

The attached patch is just one way to fix it, a deeper discussion is in:
http://www.nsnam.org/bugzilla/show_bug.cgi?id=1102
The patch is included here because anything longer than 8000 bytes will go boom
without it.

About the unit test case... I know it's a bit complex, but I haven't found any
decent way to drop fragments other than to re-define a channel.

Anyway, the test "just" send 5 packets with increased size (1000, 2000, 5000,
10000 and 65000), then it checks if they're received correctly in the following
cases:
1) normal channel
2) reordering channel (fragments out-of-order)

Then the test is repeated dropping some fragments. In this case the test checks
if the ICMP::TIME_EXCEEDED is received. Note that the last test works only if
the first 8 bytes of the original packet are received (i.e., the first
fragment), otherwise no ICMP is sent. I didn't check THAT, but... hell, should
be enough for a test, isn't it ?

Last comment: receiving an ICMP is all but obvious, it took me 2 hours to figure
out how to do it (it was rather simple at the end, but way counterintuitive).
Plus it seems that the ICMP data are stripped out, and that's not that good.
Didn't check deeper into this tho.
It could be a nice idea to define an IcmpSocket (dunno if Linux kernel does
that) or, at least, to mention in the UdpSocket documentation how to do it.

Cheers,

T.

[Tommaso Pecorella, 2011-04-29 20:05:29]

I found something I forgot.

When a packet is dropped due to timeout on fragments no trace is fired. That's
bad.

Now, IPv6 fires (incorrectly btw) a generic "m_dropTrace (packet);" (incorrectly
because it's fired for each fragment received but the last, even if it's a
perfectly valid fragment an no timeout is exceeded. IPv6 fragmentation requires
some fine-tuning.

For IPv4 I'd like to have some cleaner solution, so I'd fire a m_dropTrace only
if the fragments are discarded due to timeout.
Problems:
1) How many: one call (it's a single packet anyway) or one per fragment ?

[insert your opinion here]

2) Reason.

In the header the following drop reasons are defined:

  enum DropReason
  {
    DROP_TTL_EXPIRED = 1,   /**< Packet TTL has expired */
    DROP_NO_ROUTE,   /**< No route to host */
    DROP_BAD_CHECKSUM,   /**< Bad checksum */
    DROP_INTERFACE_DOWN,   /**< Interface is down so can not send packet */
    DROP_ROUTE_ERROR,   /**< Route error */
  };

None of them seems the right one. I'd add one but I'll have to modify
IPv4flowprobe as well.

Objections on defining a new DROP_FRAGMENT_TIMEOUT and add it to flowprobe as
well ?

Cheers,

T.

[tomh_tomh.org, 2011-05-02 04:24:23]

On 04/29/2011 01:05 PM, tommypec@gmail.com wrote:
> I found something I forgot.
>
> When a packet is dropped due to timeout on fragments no trace is fired.
> That's bad.
>
> Now, IPv6 fires (incorrectly btw) a generic "m_dropTrace (packet);"
> (incorrectly because it's fired for each fragment received but the last,
> even if it's a perfectly valid fragment an no timeout is exceeded. IPv6
> fragmentation requires some fine-tuning.

Please file this bug.

>
> For IPv4 I'd like to have some cleaner solution, so I'd fire a
> m_dropTrace only if the fragments are discarded due to timeout.
> Problems:
> 1) How many: one call (it's a single packet anyway) or one per fragment
> ?
>

I suggest one call per packet.

>
> 2) Reason.
>
> In the header the following drop reasons are defined:
>
> enum DropReason
> {
> DROP_TTL_EXPIRED = 1, /**< Packet TTL has expired */
> DROP_NO_ROUTE, /**< No route to host */
> DROP_BAD_CHECKSUM, /**< Bad checksum */
> DROP_INTERFACE_DOWN, /**< Interface is down so can not send packet
> */
> DROP_ROUTE_ERROR, /**< Route error */
> };
>
> None of them seems the right one. I'd add one but I'll have to modify
> IPv4flowprobe as well.
>
> Objections on defining a new DROP_FRAGMENT_TIMEOUT and add it to
> flowprobe as well ?
>

None from me.

- Tom

[Tommaso Pecorella, 2011-05-02 10:15:48]

Yo Tom,


On 02/mag/2011, at 06.24, Tom Henderson wrote:

> On 04/29/2011 01:05 PM, tommypec@gmail.com wrote:
>> I found something I forgot.
>> 
>> When a packet is dropped due to timeout on fragments no trace is fired.
>> That's bad.
>> 
>> Now, IPv6 fires (incorrectly btw) a generic "m_dropTrace (packet);"
>> (incorrectly because it's fired for each fragment received but the last,
>> even if it's a perfectly valid fragment an no timeout is exceeded. IPv6
>> fragmentation requires some fine-tuning.
> 
> Please file this bug.

Again ?

https://www.nsnam.org/bugzilla/show_bug.cgi?id=1100

Cheers,


T.

[Tom Henderson, 2011-05-03 04:31:51]

On 2011/05/02 10:15:48, Tommaso Pecorella wrote:
> Yo Tom,
> 
> 
> On 02/mag/2011, at 06.24, Tom Henderson wrote:
> 
> > On 04/29/2011 01:05 PM, mailto:tommypec@gmail.com wrote:
> >> I found something I forgot.
> >> 
> >> When a packet is dropped due to timeout on fragments no trace is fired.
> >> That's bad.
> >> 
> >> Now, IPv6 fires (incorrectly btw) a generic "m_dropTrace (packet);"
> >> (incorrectly because it's fired for each fragment received but the last,
> >> even if it's a perfectly valid fragment an no timeout is exceeded. IPv6
> >> fragmentation requires some fine-tuning.
> > 
> > Please file this bug.
> 
> Again ?
> 
> https://www.nsnam.org/bugzilla/show_bug.cgi?id=1100
> 
> Cheers,
> 

I should have said fix, not file :)

Sorry about that,
Tom

[Tommaso Pecorella, 2011-05-03 23:16:09]

I know I shouldn't mix things together, but right now I'm a bit wasted from work
so I didn't split the patches as they should.

Todo still:
1) split the patches in two, IPv4 and IPv6.
2) make a test case for IPv6 fragmentation.
3) document that some obscure cases of IPv6 fragmentation errors are not
handled.

Footnote: why I did all those changes to IPv6. It's the only decent way to
handle when and why a packet should be discarded. Plus it should fix the memory
leak I found. I have to triple check with valgrind tho. Too sleepy to do it now.

[Tommaso Pecorella, 2011-05-05 11:39:44]

Updated the patch with IPv6 fragmentation fixes

[Tom Henderson, 2011-06-13 05:08:45]

sorry for the delay in reviewing.  Main comments:

- I am not sure that your proposed change to IPv4 header is what the author
intended (despite the bug report 1102)-- will leave comments there

- not sure of the need to introduce ErrorChannel/NetDevice

- not sure of the plan for the file in scratch/

What additional tests should be done?  IPv6 fragmentation?  Test the ARP cache
interaction?  Even if we decide to push with just these tests, it would be
helpful to document what aspects of fragmentation handling are not being tested.

http://codereview.appspot.com/4440051/diff/26001/scratch/udp-echo-fragm.cc
File scratch/udp-echo-fragm.cc (right):

http://codereview.appspot.com/4440051/diff/26001/scratch/udp-echo-fragm.cc#ne...
scratch/udp-echo-fragm.cc:26: // - Tracing of queues and packet receptions to
file "udp-echo.tr"
Not sure whether this file is intended to be merged.  If it is, it needs some
cleanup and work on the comments.

http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-head...
File src/internet/model/ipv4-header.cc (right):

http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-head...
src/internet/model/ipv4-header.cc:122: NS_ASSERT (offset | 0x7);
This change in assert condition is based on changing the semantics of offset...

http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-head...
File src/internet/model/ipv4-header.h (right):

http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-head...
src/internet/model/ipv4-header.h:75: * \param offset the ipv4 fragment offset
measured in bytes from the start.
it seems to me that the original intention of the API was to keep the units in
8-byte blocks but here you are changing it to bytes.  

As a user, I would find this to be surprising.  I would expect that I would pass
in a maximally 13-bit value here specifying it according to the semantics of the
field in the Ipv4 header.

http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-head...
src/internet/model/ipv4-header.h:115: * \returns the offset of this fragment
measured in bytes from the start.
While I agree that your clarification here based on your code changes, I think
users may expect this to be in the actual units of fragmentation offset.

http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-head...
src/internet/model/ipv4-header.h:164: uint16_t m_fragmentOffset;
Again, it seems to me here that you are changing the original intent of this
code.

http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-l3-p...
File src/internet/model/ipv4-l3-protocol.cc (right):

http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.cc:52: TypeId
lots of whitespace/style changes in this file.  what arguments to check-style.py
were used (and what version)?

http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.cc:1138: 
these new methods are missing NS_LOG_FUNCTION

http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-l3-p...
File src/internet/model/ipv4-l3-protocol.h (right):

http://codereview.appspot.com/4440051/diff/26001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.h:363: * \brief If other fragments will be
sent.
"True if other fragmenets will be sent"

http://codereview.appspot.com/4440051/diff/26001/src/internet/test/error-chan...
File src/internet/test/error-channel.cc (right):

http://codereview.appspot.com/4440051/diff/26001/src/internet/test/error-chan...
src/internet/test/error-channel.cc:18: * Author: Mathieu Lacage
<mathieu.lacage@sophia.inria.fr>
author statement correct here?  This is just cloned from simple-channel but
maybe add your own author line also since it was entirely rewritten.

http://codereview.appspot.com/4440051/diff/26001/src/internet/test/error-chan...
src/internet/test/error-channel.cc:30: 
In general, I am not sure about the need to add this set of new classes
(ErrorChannel/NetDevice) when we have SimpleNetDevice/Channel to which one can
add ErrorModels.

This is just local to a test directory but I am left wondering why you just
didn't reuse SimpleChannel.

http://codereview.appspot.com/4440051/diff/26001/src/internet/test/error-chan...
File src/internet/test/error-channel.h (right):

http://codereview.appspot.com/4440051/diff/26001/src/internet/test/error-chan...
src/internet/test/error-channel.h:52: 
Doxygen on the following two methods would be useful.

http://codereview.appspot.com/4440051/diff/26001/src/internet/test/ipv4-fragm...
File src/internet/test/ipv4-fragmentation-test.cc (right):

http://codereview.appspot.com/4440051/diff/26001/src/internet/test/ipv4-fragm...
src/internet/test/ipv4-fragmentation-test.cc:117: : TestCase ("Verify the IPv4
layer 3 protocol fragmentation")
"and reassembly"

http://codereview.appspot.com/4440051/diff/26001/src/internet/test/ipv4-fragm...
src/internet/test/ipv4-fragmentation-test.cc:359: // Each other fragment will
arrive out-of-order.
Can you explain a bit more about what you expect to happen in this test case?

http://codereview.appspot.com/4440051/diff/26001/src/internet/test/ipv4-fragm...
src/internet/test/ipv4-fragmentation-test.cc:388: // Server -> Client : errors
disabled (we want to have back the ICMP)
similarly, can you specify for future readers why an ICMP is expected here?

http://codereview.appspot.com/4440051/diff/26001/src/internet/test/ipv4-fragm...
src/internet/test/ipv4-fragmentation-test.cc:409: NS_TEST_EXPECT_MSG_EQ
((recvSize == 0), true, "Sever got a packet, something wrong");
Server

[Tommaso Pecorella, 2011-06-13 10:14:46]

On 2011/06/13 05:08:45, Tom Henderson wrote:
> sorry for the delay in reviewing.  Main comments:
> 
> - I am not sure that your proposed change to IPv4 header is what the author
> intended (despite the bug report 1102)-- will leave comments there
> 
> - not sure of the need to introduce ErrorChannel/NetDevice
> 
> - not sure of the plan for the file in scratch/
> 
> What additional tests should be done?  IPv6 fragmentation?  Test the ARP cache
> interaction?  Even if we decide to push with just these tests, it would be
> helpful to document what aspects of fragmentation handling are not being
tested.

Hi Tom,

no problem for the delay, I'm in a rush between two meetings abroad, so I'll be
quick.

- IPv4header changes (bug report 1102): I think we'll fix it separately there,
this code will "simply" follow.
- ErrorChannel/NetDevice: SimpleChannel doesn't allow packet reordering, which
was necessary to test the out-of-order fragments reassembly. Those channels and
models are just being used into tests, so I didn't document them fully. I'll do.
- scratch/ file: not intended to be included in the patch. I'll kill it.
- Tests: IPv4 ones are all tested. As for IPv6 is concerned, I didn't modify the
actual tests, so I should check them. If I remember fine they don't check:
  * out-of-order reassembly
  * timeout and ICMP error message
I was keen on implementing also IPv6 fragmentation tests, but I need a way to
receive ICMP error messages in the Application code. Since UDPv6 is missing I
have some issues in hooking the ICMPv6 receive, also 'cuz ICPMv6 is handled in a
way different way than v4 ones.

The ICMP error message is mandatory according to the RFCs, I'll point out in the
doxygen and comments.

Hopefully I'll clean out everything and I'll post a new patch next week.

Cheers,

Tommaso

[Tommaso Pecorella, 2011-06-24 00:44:29]

This is only to cleanup the comments and remove the unnecessary files (scratch
and IPv4header.cc/.h).

I have to check if I can scrap out also the ErrorNetDevice, maybe yes. The other
2 (channel and model) are necessary as the "Simple" one doesn't do what we need
for the test.

About the last "bit", I am eager to have an hint. How can I "trap" an ICMPv6
message ? I mean, I have to have a function called when a particular ICMPv6 type
is received, but beat me in the head if I have found a way to do that. v4 have a
callback, v6 seems not.
Am I dumb or I missed something ? Or I found another bug ?

Cheers,

Tommaso

[Vedran Miletić, 2011-06-24 15:51:05]

Sorry for the delay on reviewing.

Implementation looks good to me. Compared to Mathieu's yans implementation I
adapted for ns-3, this one uses STL more and looks much cleaner.

API change that Tom mentioned can also be implemented here simply by renaming
SendRealOut to SendOut.

A couple of comments on the code are below.

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
File src/internet/model/ipv4-l3-protocol.cc (right):

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.cc:62: .AddAttribute ("WaitReplyTimeout",
Why is it named WaitReply?

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.cc:1207: uint32_t idProto = uint32_t
(ipHeader.GetIdentification ()) << 16 & uint32_t (ipHeader.GetProtocol ());
Wouldn't it be better to use std::pair here (both this and previous line)?

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.cc:1385:
Ipv4L3Protocol::HandleFragmentsTimeout (std::pair<uint64_t, uint32_t> key,
Ipv4Header & ipHeader, uint32_t iif)
I don't understand when does this get called.

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
File src/internet/model/ipv4-l3-protocol.h (right):

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.h:370: std::list<std::pair<Ptr<Packet>,
uint16_t> > m_fragments;
This is really good.

[Tommaso Pecorella, 2011-06-24 19:49:00]

On 2011/06/24 15:51:05, Vedran Miletić wrote:
> Sorry for the delay on reviewing.

No problem at all, we was discussing about the IPv4header :)

> Implementation looks good to me. Compared to Mathieu's yans implementation I
> adapted for ns-3, this one uses STL more and looks much cleaner.

Thanks :)

> API change that Tom mentioned can also be implemented here simply by renaming
> SendRealOut to SendOut.

Can you point me to that discussion? I missed it.
 
> A couple of comments on the code are below.

Replies below.

Cheers,

Tommaso

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
File src/internet/model/ipv4-l3-protocol.cc (right):

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.cc:62: .AddAttribute ("WaitReplyTimeout",
On 2011/06/24 15:51:05, Vedran Miletić wrote:
> Why is it named WaitReply?

Honestly, I can't remember. The only twisted explanation might be: It's a
Timeout. You must wait for it. In case it expires you have to send a Reply (an
ICMP error indeed).
Either I copy-pasted from somewhere else and I forgot to rename it :)

Probably I was very tired when I named it. I'll change to something more
meaningful. Actually less meaningless...

FragmentExpirationTimeout seems good enough.

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.cc:1207: uint32_t idProto = uint32_t
(ipHeader.GetIdentification ()) << 16 & uint32_t (ipHeader.GetProtocol ());
On 2011/06/24 15:51:05, Vedran Miletić wrote:
> Wouldn't it be better to use std::pair here (both this and previous line)?

Could be, but the key is a Pair already and those are the elements of the key. I
didn't want to "over"-use Pairs, as those are perfectly hold by uint64 and
uint32.

Using Pair for those as well could slow down the key comparison later, as
comparing two uints is way more efficient than recursively comparing pairs.

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.cc:1385:
Ipv4L3Protocol::HandleFragmentsTimeout (std::pair<uint64_t, uint32_t> key,
Ipv4Header & ipHeader, uint32_t iif)
In Ipv4L3Protocol::ProcessFragment:
      m_fragmentsTimers[key] = Simulator::Schedule (m_waitFragmentsTimeout,
&Ipv4L3Protocol::HandleFragmentsTimeout, this, key, ipHeader, iif);

Basically when you receive the first fragment you schedule an event that will
fire this function. If you receive all the fragments before that, you cancel the
scheduled event.

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
File src/internet/model/ipv4-l3-protocol.h (right):

http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.h:370: std::list<std::pair<Ptr<Packet>,
uint16_t> > m_fragments;
On 2011/06/24 15:51:05, Vedran Miletić wrote:
> This is really good.

Thanks :)

[Vedran Miletić, 2011-06-25 08:21:53]

On 2011/06/24 19:49:00, Tommaso Pecorella wrote:
> > API change that Tom mentioned can also be implemented here simply by
renaming
> > SendRealOut to SendOut.
> 
> Can you point me to that discussion? I missed it.
>  

See Tom's first reply to this.

>
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
> File src/internet/model/ipv4-l3-protocol.cc (right):
> 
>
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
> src/internet/model/ipv4-l3-protocol.cc:62: .AddAttribute ("WaitReplyTimeout",
> On 2011/06/24 15:51:05, Vedran Miletić wrote:
> > Why is it named WaitReply?
> 
> Honestly, I can't remember. The only twisted explanation might be: It's a
> Timeout. You must wait for it. In case it expires you have to send a Reply (an
> ICMP error indeed).
> Either I copy-pasted from somewhere else and I forgot to rename it :)
> 
> Probably I was very tired when I named it. I'll change to something more
> meaningful. Actually less meaningless...
> 
> FragmentExpirationTimeout seems good enough.
> 

Good.

>
http://codereview.appspot.com/4440051/diff/38001/src/internet/model/ipv4-l3-p...
> src/internet/model/ipv4-l3-protocol.cc:1385:
> Ipv4L3Protocol::HandleFragmentsTimeout (std::pair<uint64_t, uint32_t> key,
> Ipv4Header & ipHeader, uint32_t iif)
> In Ipv4L3Protocol::ProcessFragment:
>       m_fragmentsTimers[key] = Simulator::Schedule (m_waitFragmentsTimeout,
> &Ipv4L3Protocol::HandleFragmentsTimeout, this, key, ipHeader, iif);
> 
> Basically when you receive the first fragment you schedule an event that will
> fire this function. If you receive all the fragments before that, you cancel
the
> scheduled event.
> 

Thanks, I see now.

Regards,

Vedran

[Tommaso Pecorella, 2011-06-25 11:41:19]

On 2011/06/25 08:21:53, Vedran Miletić wrote:
> On 2011/06/24 19:49:00, Tommaso Pecorella wrote:
> > > API change that Tom mentioned can also be implemented here simply by
> renaming
> > > SendRealOut to SendOut.
> > 
> > Can you point me to that discussion? I missed it.
> >  
> 
> See Tom's first reply to this.
> 

Ah, yes. I do remember now.

Well, I didn't change those at all (the APIs I mean) so they're the "old" ones
already in the actual implementation.
Whatever the SendRealOut should be renamed SendOut or not is, in my opinion,
purely cosmetic as it's a private function.

I guess that Tom was referring to the fact that your approach changed that API
slightly by introducing another "Send" function, while here it is preserved the
old one.

Cheers,

Tommaso

[Tommaso Pecorella, 2011-06-28 06:53:43]

Let's not loose the spin... we're close to finalize this small but quite useful
patch.

Vedran: missing comment on SendOut / SendRealOut.
Tom: need more comments (if any).
Myself: need to re-check with uncrustify (was using 0.56, now using 0.58).
Plus: need to document in the tests comments what's being tested and what is NOT
being tested (IPv6 in particular).

Side note: I'm not very happy about how ICMPv6L4 protocol is handling ICMPs.
I'll propose a separate patch.

Sorry (again) if I'm pushing, but if we loose the spin we'll never end this fix.

Cheers,

Tommaso

[Tom Henderson, 2011-06-28 17:45:56]

Regarding uncrustify, you have a lot of trailing whitespace changes in
ipv4-l3-protocol.cc which I suggest that you avoid.  We did run uncrustify 0.58
on this before (but not the level 3 which handles trailing whitespace).

My only lingering comments are that it would be nice to split IPv4 and IPv6
patches when committing, and it would be nice to try to copy the
ipv4-fragmentation-test to ipv6-fragmentation-test (not sure whether it is
blocked by ICMP problems).

http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-p...
File src/internet/model/ipv4-l3-protocol.cc (right):

http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-p...
src/internet/model/ipv4-l3-protocol.cc:1137: // Of course also the reassemby
code shall be changed as well.
s/reassemby/reassembly

you could consider to make the program abort if this is entered with an IPv4
header option present

[Tommaso Pecorella, 2011-06-28 18:50:21]

On 2011/06/28 17:45:56, Tom Henderson wrote:
> Regarding uncrustify, you have a lot of trailing whitespace changes in
> ipv4-l3-protocol.cc which I suggest that you avoid.  We did run uncrustify
0.58
> on this before (but not the level 3 which handles trailing whitespace).

Doh! That's why. I'll make sure to use lvl 2.

> My only lingering comments are that it would be nice to split IPv4 and IPv6
> patches when committing

I'll do.

> and it would be nice to try to copy the
> ipv4-fragmentation-test to ipv6-fragmentation-test (not sure whether it is
> blocked by ICMP problems).

It is. I'm blocked on this point also for another piece of code (not related to
this), and it seems that the ICMPv6L4 proto doesn't have any possible callback
to inform that an ICMP has been received. Plus the IPv4 test program does use
UDP, not available for v6. I'm not keen blocking the v6 patch 'til those two
issues are fixed to be honest, but for sure we can improve the test once they
are.
 
>
http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-p...
> File src/internet/model/ipv4-l3-protocol.cc (right):
> 
>
http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-p...
> src/internet/model/ipv4-l3-protocol.cc:1137: // Of course also the reassemby
> code shall be changed as well.
> s/reassemby/reassembly
> 
> you could consider to make the program abort if this is entered with an IPv4
> header option present

If only I could test for something that's not here... I mean, there's not even
the class to hold IPv4 optional headers in ns-3... testing against them would
mean to define them somewhere, and if I'd define them I could handle them as
well ! It's a cat trying to catch his own tail.

WHat about a warning in the IPv4 doxygen so any (future) implementor will know
that he/she have to check the fragmentation as well ?

Cheers,

Tommaso

[tomh_tomh.org, 2011-06-28 18:57:18]

On 06/28/2011 11:50 AM, tommypec@gmail.com wrote:
> On 2011/06/28 17:45:56, Tom Henderson wrote:
>> Regarding uncrustify, you have a lot of trailing whitespace changes in
>> ipv4-l3-protocol.cc which I suggest that you avoid. We did run
> uncrustify 0.58
>> on this before (but not the level 3 which handles trailing
> whitespace).
>
> Doh! That's why. I'll make sure to use lvl 2.
>
>> My only lingering comments are that it would be nice to split IPv4 and
> IPv6
>> patches when committing
>
> I'll do.
>
>> and it would be nice to try to copy the
>> ipv4-fragmentation-test to ipv6-fragmentation-test (not sure whether
> it is
>> blocked by ICMP problems).
>
> It is. I'm blocked on this point also for another piece of code (not
> related to this), and it seems that the ICMPv6L4 proto doesn't have any
> possible callback to inform that an ICMP has been received. Plus the
> IPv4 test program does use UDP, not available for v6. I'm not keen
> blocking the v6 patch 'til those two issues are fixed to be honest, but
> for sure we can improve the test once they are.

OK, I agree not to block on this.

>
>
>
http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-p...
>
>> File src/internet/model/ipv4-l3-protocol.cc (right):
>
>
>
http://codereview.appspot.com/4440051/diff/44001/src/internet/model/ipv4-l3-p...
>
>> src/internet/model/ipv4-l3-protocol.cc:1137: // Of course also the
> reassemby
>> code shall be changed as well.
>> s/reassemby/reassembly
>
>> you could consider to make the program abort if this is entered with
> an IPv4
>> header option present
>
> If only I could test for something that's not here... I mean, there's
> not even the class to hold IPv4 optional headers in ns-3... testing
> against them would mean to define them somewhere, and if I'd define them
> I could handle them as well ! It's a cat trying to catch his own tail.

i meant trying to determine whether options are present by looking at 
the internet header length field.

[Tommaso Pecorella, 2011-06-28 19:30:08]

On 2011/06/28 18:57:18, tomh_tomh.org wrote:
> >> you could consider to make the program abort if this is entered with
> > an IPv4
> >> header option present
> >
> > If only I could test for something that's not here... I mean, there's
> > not even the class to hold IPv4 optional headers in ns-3... testing
> > against them would mean to define them somewhere, and if I'd define them
> > I could handle them as well ! It's a cat trying to catch his own tail.
> 
> i meant trying to determine whether options are present by looking at 
> the internet header length field.

I should not write replies when I'm tired or when I just ate mah dinner. In the
latter case (it's 21:30 here) all the blood is working for the stomach and the
brain is shut down. You're damn right.

I'm gonna implement it. I'll post a revised patch with this and the fixed
uncrustify.

Cheers,

Tommaso

[Tommaso Pecorella, 2011-06-28 21:04:15]

On 2011/06/28 20:59:48, Tommaso Pecorella wrote:

All done. Plan for the patches is (when everyone will be happy about all):
IPv4 Fragmentation and reassembly:

src/flow-monitor/model/ipv4-flow-probe.cc
src/flow-monitor/model/ipv4-flow-probe.h	
src/internet/model/ipv4-l3-protocol.cc
src/internet/model/ipv4-l3-protocol.h
src/internet/test/error-channel.cc
src/internet/test/error-channel.h
src/internet/test/error-net-device.cc
src/internet/test/error-net-device.h
src/internet/test/ipv4-fragmentation-test.cc
src/internet/wscript

IPv6 fragmentation improvements:

src/internet/model/ipv6-extension.cc
src/internet/model/ipv6-extension.h

Cheers,

Tommaso

[Tommaso Pecorella, 2011-07-05 07:50:19]

Hi guys,

shall we do a final round of checks or do you think this (2) patches are ready
for prime time ?

Cheers,

Tommaso

[Vedran Miletić, 2011-07-06 12:08:59]

Hi Tommaso,

on the IPv4 side, I believe this is ready for prime time. As for (cosmetic)
renaming SendRealOut to SendOut, I would vote for it; it doesn't change the
external API and SendOut would make more sense as a name. The names come from
yans' functions send_out (that was doing fragmentation) and send_real_out (that
was sending fragments to network). Your implementation basically merges them.

Good work.

Regards,

Vedran

[Tom Henderson, 2011-07-08 17:17:44]

just a few more comments

http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-exte...
File src/internet/model/ipv6-extension.cc (right):

http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-exte...
src/internet/model/ipv6-extension.cc:375: //      m_dropTrace (packet);
I didn't understand why you set isDropped = true but delete the droptrace.

http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragm...
File src/internet/test/ipv4-fragmentation-test.cc (right):

http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragm...
src/internet/test/ipv4-fragmentation-test.cc:167: // NS_LOG_UNCOND("**
HandleReadServer " << *packet << " - " << packet->GetSize () );
delete the above?

http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragm...
src/internet/test/ipv4-fragmentation-test.cc:267: //
LogComponentEnable("Icmpv4L4Protocol", LOG_LEVEL_ALL);
delete the above

[Tommaso Pecorella, 2011-07-08 18:49:47]

http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-exte...
File src/internet/model/ipv6-extension.cc (right):

http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-exte...
src/internet/model/ipv6-extension.cc:375: //      m_dropTrace (packet);
On 2011/07/08 17:17:45, Tom Henderson wrote:
> I didn't understand why you set isDropped = true but delete the droptrace.

The isDropped is set because no packet is forwarded up, but the packet (as a
whole) is not dropped, so the trace is not set.

I'll recheck. Maybe it's not necessary to set isDropped to true.

http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragm...
File src/internet/test/ipv4-fragmentation-test.cc (right):

http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragm...
src/internet/test/ipv4-fragmentation-test.cc:167: // NS_LOG_UNCOND("**
HandleReadServer " << *packet << " - " << packet->GetSize () );
On 2011/07/08 17:17:45, Tom Henderson wrote:
> delete the above?

Aye, forgot.

http://codereview.appspot.com/4440051/diff/41003/src/internet/test/ipv4-fragm...
src/internet/test/ipv4-fragmentation-test.cc:267: //
LogComponentEnable("Icmpv4L4Protocol", LOG_LEVEL_ALL);
On 2011/07/08 17:17:45, Tom Henderson wrote:
> delete the above

As before, forgot. Will do.

[Tommaso Pecorella, 2011-07-09 01:20:46]

http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-exte...
File src/internet/model/ipv6-extension.cc (right):

http://codereview.appspot.com/4440051/diff/41003/src/internet/model/ipv6-exte...
src/internet/model/ipv6-extension.cc:375: //      m_dropTrace (packet);
On 2011/07/08 17:17:45, Tom Henderson wrote:
> I didn't understand why you set isDropped = true but delete the droptrace.

Re-checked. I was right, it have to be set.

In Ipv6L3Protocol::LocalDeliver the "isDropped" is used to warn the L3 that the
"packet" (a fragment for real) is on hold and no further actions should be made:
----
          nextHeaderPosition += ipv6Extension->Process (p, nextHeaderPosition,
ip, dst, &nextHeader, isDropped);

          if (isDropped)
            {
              return;
            }
----

Maybe it's a misleading name inside the fragment extension code, but I left the
original naming.

The trace, on the contrary, have to be deleted as the fragment is not dropped at
all at that time. The trace will be called when and if the whole packet will be
dropped along with all the fragments received so far (after the timeout).

So the above code is correct.

Tommaso.