Created attachment 2978 [details]
Patch in EpcTftClassifier

This patch solves the segmentation fault. It checks the fragment offset and the payload size before decoding the transport header (TCP/UDP).

If fragment offset is not zero, then port info is not present and the default bearer (TFT ID = 1) is used for this packet.

(In reply to Manuel Requena from comment #1)
> Created attachment 2978 [details]
> Patch in EpcTftClassifier
> 
> This patch solves the segmentation fault. It checks the fragment offset and
> the payload size before decoding the transport header (TCP/UDP).
> 
> If fragment offset is not zero, then port info is not present and the
> default bearer (TFT ID = 1) is used for this packet.

For the case where fragment offset is non-zero, the code falls through to the classification logic.  Should it instead return zero such as in lines 136-139 of the patch? e.g.:

  else
    {
       NS_LOG_INFO ("fragment offset not zero: " << fragmentOffset);
       return 0;  // no match
    }


I suggest to document the behavior in Doxygen:

 * \note this implementation works with IPv4 only.

changed to

 * \note this implementation works with IPv4 only, and 5-tuple match only works on the first fragment if the packet is fragmented (i.e. there is no fragmentation state)

(In reply to Tommaso Pecorella from comment #0)
> EpcTftClassifier::Classify just removes the L4 protocols, without worrying
> that the L4 protocol is there or not.
> 
> When a packet is fragmented the Ip header still carries the L4 protocol
> number, but the L4 protocol is just present in the first fragment (and for
> small fragments it could even be incomplete).
> If only the ports are needed, one should just peek the IP payload, since
> even the smallest fragment will carry the L4 port numbers (thanks to how
> fragments are done).
> 
> One could just skip fragments, but this would mean to assign a default class
> to all the fragments, which is quite unfair.
> A more complex solution is to use a state machine and apply the first
> fragment classification to all the successive fragments. However, the first
> fragment is not guaranteed to be the first packet arriving to the classifier
> (IP packets can be reordered). As a consequence, a form of
> buffering-and-timeout is needed.

Is this a common use case (presence of fragments) in this model, or just a corner case?  That may determine whether we should try for a stateful classifier.

> 
> The last problem is the fragment ID validity. For a long discussion see RFC
> 6864, but the synthesis is: it's not as easy as one could think - fragments
> could be lost and the next fragmented packet wth the same ID could have a
> different classification.
> This could be not a problem for a naive implementation, but it must be
> considered (and documented).
> 

Is this possible in how ns-3 assigns fragment IDs?

(In reply to Tom Henderson from comment #2)
> For the case where fragment offset is non-zero, the code falls through to
> the classification logic.  Should it instead return zero such as in lines
> 136-139 of the patch? e.g.:
> 
>   else
>     {
>        NS_LOG_INFO ("fragment offset not zero: " << fragmentOffset);
>        return 0;  // no match
>     }
> 

The default bearer (with TFT ID = 1) is also in m_tftMap and the matching is found at the end of the Classify method. If this method returns 0, it means there is not match at all.

> 
> I suggest to document the behavior in Doxygen:
> 
>  * \note this implementation works with IPv4 only.
> 
> changed to
> 
>  * \note this implementation works with IPv4 only, and 5-tuple match only
> works on the first fragment if the packet is fragmented (i.e. there is no
> fragmentation state)

I'll add the following:
 * \note this implementation works with IPv4 only, and 5-tuple match
 * (TFT of a dedicated bearer) only works on the first fragment if the packet is
 * fragmented (i.e. there is no fragmentation state). For non-first fragments,
 * the TFT of the default bearer is used.

> 
> I'll add the following:
>  * \note this implementation works with IPv4 only, and 5-tuple match
>  * (TFT of a dedicated bearer) only works on the first fragment if the
> packet is
>  * fragmented (i.e. there is no fragmentation state). For non-first
> fragments,
>  * the TFT of the default bearer is used.

In the above, please change 'IPv4 only' to 'IPv4 UDP and IPv4 TCP only'; I just worry about a future user trying to use a different protocol with this (but maybe they will notice the lack of protocol number supported in the TFT itself).

Created attachment 3036 [details]
Updated patch for EpcTftClassifier

This patch builds and extends Manuel's patch in Comment 1. The changes
made are:
  - Store the classification of the first fragment of a packet, so that
the rest of the fragments are classified in the same way.
  - If fragments arrive out of order and there is not a previous
classification, the fragment is sent through the default bearer.

test-epc-tft-classifier.cc has also been modified to add the payload
size to the IPv4 header and avoid tests failing due to the new logic.

Created attachment 3045 [details]
Patch in EpcTftClassifier after IPv6 support

Hi Richard,

After the support for IPv6, your patch cannot be applied directly. Moreover, there are some issues in you strategy that I have fixed:
- Before trying to get the port info, you search into the cache map. IMO, the cache map should only be used if the port info cannot be get from the packet.
- You store the TEID, but what is missing is the port info.
- If it is not the first fragment, you directly classify the fragment to the default bearer. But maybe port info is not really needed.

This new patch tries to fix these issues and can be merged in the current ns-3-dev.

*** Bug 1861 has been marked as a duplicate of this bug. ***

patch commited in changeset:   13303:acc0012f11dc