Bugzilla – Bug 1808
FlowMon relies on IPv4's Identification field to trace packets
Last modified: 2014-01-23 13:35:34 UTC
... and this isn't exactly a good idea. According to RFCs the Identification field must be zero. Only fragmented packets should have a non-zero Identification field and it should be random. The changes needed to fix this are: 1) Change IPv4 FlowMon components to identify packets according to something else (maybe the Packet Uid, or a packet's hash, or a Tag, or all of them). 2) Change how the Identification field is handled by IPv4. If this isn't fixed, FlowMon can *not* be used with emulated devices (i.e., in mixed emulated/simulated environments).
Created attachment 1745 [details] Patch to have FlowMon IP classification done with tags With this patch, FlowMon only inspects a packet's content then it is sent the first time (IPv4 SendOutgoing trace). When the packet is forwarded, received at destination or dropped, only the tag is used. The direct result is: 1) We can easily add new data to the probe, e.g., the number of hops the packet did. 2) Adding IPv6 support is a snap (just add the proper traces to Ipv6l3) and copy-paste the probes in FlowMon) 3) We can kill the non-standard use of IP header's Identification field - HOORAY! Moreover, since now also fragments can be classified properly (!!!), the stats are finally "right": <FlowProbes> <FlowProbe index="0"> <FlowStats flowId="1" packets="3735" bytes="2149400" delayFromFirstProbeSum="+0.0ns" > </FlowStats> </FlowProbe> <FlowProbe index="1"> <FlowStats flowId="1" packets="7466" bytes="2224020" delayFromFirstProbeSum="+199415389258.0ns" > </FlowStats> </FlowProbe> <FlowProbe index="2"> <FlowStats flowId="1" packets="3735" bytes="2149400" delayFromFirstProbeSum="+138731526300.0ns" > </FlowStats> </FlowProbe> </FlowProbes> FlowProbe with index "1" (the forwarder) reports more packets and more bytes. Of course, more packets, more IP headers, more overhead !
(In reply to Tommaso Pecorella from comment #0) > ... and this isn't exactly a good idea. > According to RFCs the Identification field must be zero. > Only fragmented packets should have a non-zero Identification field and it > should be random. Not true according to RFC 6864: - ID field must be unique for packets that may be fragmented - ID field can be arbitrary for packets that cannot be fragmented > > The changes needed to fix this are: > 1) Change IPv4 FlowMon components to identify packets according to something > else (maybe the Packet Uid, or a packet's hash, or a Tag, or all of them). > 2) Change how the Identification field is handled by IPv4. > > If this isn't fixed, FlowMon can *not* be used with emulated devices (i.e., > in mixed emulated/simulated environments). The below is not quite the same as bug 904; suggest a more informative comment. + // ConstCast: see http://www.nsnam.org/bugzilla/0 + bool found = ConstCast<Packet> (ipPayload)->PeekPacketTag (fTag);
Probably you're right about ID… I should re-read the RFC carefully. Somewhat I was focused on the IPv6 ID, which *is* zero for non-fragmented packets. Anyway, the patch is about having FlowMon not using the ID field, which I think it's a good idea (and it's required for IPv6 anyway, so it's a matter of uniformity). About that comment, I guess I did copy the lines from somewhere. I'll have to check that as well. T.
The comment was (is) in the original ipv4-flow-probe.cc. See http://code.nsnam.org/ns-3-dev/file/6ead574f588a/src/flow-monitor/model/ipv4-flow-probe.cc I'm up to removing it everywhere. T.
(In reply to Tommaso Pecorella from comment #4) > The comment was (is) in the original ipv4-flow-probe.cc. > > See > http://code.nsnam.org/ns-3-dev/file/6ead574f588a/src/flow-monitor/model/ipv4- > flow-probe.cc > > I'm up to removing it everywhere. > > T. In ipv4-flow-probe.cc, it is pertaining to the RemovePacketTag. I think the comment is fine to leave as is because it does refer to bug 904. In your patch, you also apply it to PeekPacketTag (besides RemovePacketTag). + // ConstCast: see http://www.nsnam.org/bugzilla/0 + bool found = ConstCast<Packet> (ipPayload)->PeekPacketTag (fTag); is this cast even needed (PeekPacketTag already operates on const Packets)? If not, suggest to remove it. If you can remove this cast and the comment preceding it, I don't have other comments and suggest to merge.
Corrected the PeekPacketTag and pushed. changeset: 10586:25727ac2504f