[ovs-dev] RFC: correct way to deal with runt packets

[Ben Pfaff]

Hi Jesse (and everyone else).

In a recent thread, you pointed out that the kernel and user datapaths
mishandle packets that have too-short IP headers.  That is, if a frame
has an Ethernet type of 0x0800 (IP), but the Ethernet header is followed
by less than 20 bytes (the minimum length IP header), then the flow will
have dl_type 0x0800 and the L3 header pointer in the skb will point to
the incomplete IP header, with no easy way for code that receives the
skb and flow to tell that the IP header was missing or truncated.

I can think of a few ways to handle this:

    1. Zero out the dl_type instead of claiming that it's IP.  It's not
       really IP, after all, since that header isn't really there.

       There is precedent for this behavior because it's what OVS does
       for L4: if the TCP or UDP header is missing, we zero out
       nw_proto.  It's also what I thought OVS was doing already.

       On the other hand, I'm not sure that this is great behavior.
       It's kind of weird to lie about the contents of an outer header
       based on the absence or incorrectness of an inner header.

       This choice also implies that extending OVS to support more
       protocols will change the interpretation of headers that we
       already support.  For example, if we add IPv6 parsing, then
       suddenly short IPv6 packets will have their dl_type zeroed out
       whereas currently it would be 0x86dd regardless of IPv6 header
       correctness.  This seems odd to me.

       This choice also makes it impossible to set up a flow that will
       catch short different kinds of short frames, e.g. if you want
       short IPv4 packets you also get short ARP packets.  I don't know
       if I care.

    2. Set the L3 header pointer to null.  This works in userspace,
       where the L3 header pointer is always a pointer.  It doesn't work
       gracefully in the kernel, because the L3 header pointer is not
       necessarily a pointer there and there's no corresponding
       "sentinel" value that means "not really set" (as far as I can
       tell).

    3. Leave the dl_type and L3 header pointer set as they are now, but
       make code that cares about them itself check that the packet is
       long enough.  This has the potential advantage of dropping
       branches from the flow_extract() code, which is probably the fast
       path here, putting all of the checking burden into the code that
       actually needs to change headers, which probably doesn't get hit
       as much.

    4. Just drop short packets.

I'm leaning slightly toward #3.

What do you think?