Bug 2078

Summary: Packet metadata disabling on a per-packet basis
Product: ns-3 Reporter: Tommaso Pecorella <tommaso.pecorella>
Component: networkAssignee: ns-bugs <ns-bugs>
Status: PATCH PENDING ---    
Severity: normal CC: tomh
Priority: P5    
Version: ns-3-dev   
Hardware: All   
OS: All   
Attachments: example script
possible patch
revised patch for ns-3-dev tip
New patch
Newest patch

Description Tommaso Pecorella 2015-03-15 04:09:01 EDT
Some packets may not have metadata, and still be perfectly valid.
One example is in Icmpv6L4Protocol::HandleDestinationUnreachable.
The packet being carried by the ICMP is "rebuilt" from a byte buffer, and it doesn't contain any metadata.

If the user enables metadata checking ("Packet::EnableChecking ();"), any ICMPv6 Destination Unreachable will cause a crash.

One solution (without adding flags to the packet class) is to add a new RemoveHeader (and RemoveTrailer) functions, explicitly stating that metadata are not to be checked.
Comment 1 Tommaso Pecorella 2015-03-15 19:04:03 EDT
Created attachment 1991 [details]
example script

Test script (it could be reduced to "send an Icmpv6 Destination Unreachable")
Comment 2 Tommaso Pecorella 2015-03-15 19:07:52 EDT
Created attachment 1992 [details]
possible patch

This patch disables metadata removal from headers or trailers if the packet has been created from a plain byte buffer.

I still don't know why the 
Packet::Packet (uint8_t const*buffer, uint32_t size) 
constructor implicitly creates a "size" fake header metadata. Removing it is harmful.
Comment 3 Tom Henderson 2016-02-26 00:52:14 EST
I tried the patch on ns-3-dev and while I found that it fixed the example program, it caused other example/test failures.

./waf --run examples/ipv6/icmpv6-redirect

'build' finished successfully (4.052s)
Node: 1, Time: +0.0s, Local time: +0.0s, Ipv6StaticRouting table
Destination                    Next Hop                   Flag Met Ref Use If
::1/128                        ::                         UH   0   -   -   0
fe80::/64                      ::                         U    0   -   -   1
2001:1::/64                    ::                         U    0   -   -   1
2001:2::200:ff:fe00:5/128      fe80::200:ff:fe00:3        UH   0   -   -   1

assert failed. cond="m_current >= delta", file=./ns3/buffer.h, line=859
terminate called without an active exception

These are the new failures due to the patch I will attach.

List of FAILed tests:
    packet-metadata
List of CRASHed tests:
    examples/ipv6/icmpv6-redirect
    examples/ipv6/radvd
    examples/ipv6/radvd-two-prefix
    examples/routing/simple-routing-ping6
    examples/routing/simple-routing-ping6.py
    sixlowpan-fragmentation
Comment 4 Tom Henderson 2016-02-26 00:53:22 EST
Created attachment 2300 [details]
revised patch for ns-3-dev tip
Comment 5 Tommaso Pecorella 2016-02-27 07:12:02 EST
Created attachment 2306 [details]
New patch

Changed the approach to much, much better one.

The developer have to *explicitly* mark the packet as non-metadata safe.
The previous idea was not considering the case of payloads built by a buffer with metadata-safe headers.

Note that there's still one problem, as is what to do with the case of mix-n-match.
Example: IP packet built from a buffer (non-metadata-safe). Then you add the Ethernet header / trailer. those *will* add metadata.
Upon removing the Ethernet header/trailer, metadata are not removed, because the packet is not safe.

So, the question is: shall we remove all the metadata if the packet is not safe ?
What to do when two packets are merged (AddAtEnd) and only one of them is non-metadata-safe ?

The actual patch fixes an actual bug, but the above points could come back sooner or later.
Comment 6 Tom Henderson 2016-05-24 14:29:05 EDT
(In reply to Tommaso Pecorella from comment #5)
> Created attachment 2306 [details]
> New patch
> 
> Changed the approach to much, much better one.
> 
> The developer have to *explicitly* mark the packet as non-metadata safe.
> The previous idea was not considering the case of payloads built by a buffer
> with metadata-safe headers.

What about making m_metadataDisabled an instance member of PacketMetadata instead, and then the only touch to class Packet is:

/**
 * Disable, on a per-packet basis, the use of metadata.  This must be used
 * if metadata checking is globally enabled, and if the packet is built in a 
 * manner (e.g. from a raw byte buffer) such that the metadata is
 * incomplete.
 */
void
Packet::DisableMetadata (void)
{
  m_metadata.Disable ();
}

('Disabled' seems a clearer term than 'Dirty' to me)

(also, document that the Create<Packet> () method of packet creation from
raw byte buffer will disable metadata even if it is globally enabled)

> 
> Note that there's still one problem, as is what to do with the case of
> mix-n-match.
> Example: IP packet built from a buffer (non-metadata-safe). Then you add the
> Ethernet header / trailer. those *will* add metadata.
> Upon removing the Ethernet header/trailer, metadata are not removed, because
> the packet is not safe.
> 
> So, the question is: shall we remove all the metadata if the packet is not
> safe ?

Suggest to disable if this doesn't cause problems elsewhere (i.e. prevent mix-n-match from occurring).

> What to do when two packets are merged (AddAtEnd) and only one of them is
> non-metadata-safe ?

Suggest to disable and document this as part of AddAtEnd().

> 
> The actual patch fixes an actual bug, but the above points could come back
> sooner or later.
Comment 7 Tommaso Pecorella 2016-05-25 20:49:14 EDT
Created attachment 2456 [details]
Newest patch

Trying again. This time the changes are (almost) only to PacketMetadata.
If the approach is ok, the only thing missing is to modify the metadata tests to check this new functionality.