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

[Jesse Gross]

On Tue, Aug 10, 2010 at 4:27 PM, Ben Pfaff <blp at nicira.com> wrote:

> 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.


Options #3 sounds reasonable to me.  What would you do if the header is too
short to perform an action?  Drop the packet?  Not do the action?  Would you
also make the behavior of the L4 code match?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20100810/69d9f416/attachment-0003.html>