[ovs-dev] [IP DEFRAG ACTION RFC 2/2] datapath: add ip_defag action

[Thomas Graf]

On 11/11/14 at 12:03pm, Andy Zhou wrote:
> On Tue, Nov 11, 2014 at 2:48 AM, Thomas Graf <tgraf at noironetworks.com> wrote:
> > Don't you need to pull off the ethernet header first? ip_defrag()
> > expects an IP header at skb->data.
> I could be wrong on this, but It looks to me ip_defrag uses ip_hdr()
> to get the IP header information, thus pull
> off ethernet header is not necessary.

ip_frag_queue() pulls off the IP header:

        if (pskb_pull(skb, ihl) == NULL)
                goto err;

        err = pskb_trim_rcsum(skb, end - offset);
        if (err)
                goto err;

It also assumes skb->len to not include any L2 headers if I read the
code correctly.

> > Given I remember the defrag implementation correctly:
> >
> > We have to be clear about the side effects of this if we are reusing the
> > existing CONNTRACK_IN zones. A packet processed in parallel outside of
> > OVS on another net_device in the same net namespace can be reassembled
> > into the OVS owned skb even though that packet may not match the original
> > flow. The iif check not part of the ipq lookup.
> 
> Should zone be assigned in such a way to avoid those conflicts?  In
> addition, OVS IP_DEFRAG
> can skip any SKBs that has already filtered by netfilter.

I think the zone support is what makes the behaviour as proposed
acceptable. If documented, it is up to the user to configure the
system in a way that does not include these holes.

> A cleaner option is to add new zone for OVS so we can avoid reusing
> CONTRACK_IN zone.

I would like to keep the option to share the same zones because I
can see a lot of value to match on CTs which have been created by
netfilter or vice versa.

> Technically this cleaner. OVS IP_DEFRAG can be applied to either IN or
> OUT direction,
> depending on OF rules, thus only one zone is needed.  I did not add a
> new zone  for this patch
> since I have not found a good way to add compat header for it.
> >
> > A typical example would be:
> >
> >         in_port(2),actions=ip_defrag(),...
> 
> Why is this typical?

Should have been more precise. A typical example in which the match
associated with the flow running the defrag action may not match all
packets which end up being reassembled.

This specific discussion was held a couple of years back in the context
of netfilter where packets could bypass the iptables rules by getting
reassembled/defraged into a packet which was already accepted. Such a
packet may contain L2 bits or metadata which would require it to get
dropped by rules.