Re: [PATCH net-next] openvswitch: prepare for stolen verdict coming from conntrack and nat engine

[Florian Westphal <fw@strlen.de>]

Aaron Conole <aconole@redhat.com> wrote:
> > verdict with NF_DROP_REASON() helper,
> >
> > This helper releases the skb instantly (so drop_monitor can pinpoint
> > precise location) and returns NF_STOLEN.
> >
> > Prepare call sites to deal with this before introducing such changes
> > in conntrack and nat core.
> >
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> 
> AFAIU, these changes are only impacting the existing NF_DROP cases, and
> won't impact how ovs + netfilter communicate about invalid packets.  One
> important thing to note is that we rely on:
> 
>  * Note that if the packet is deemed invalid by conntrack, skb->_nfct will be
>  * set to NULL and 0 will be returned.

Right, this is about how to communicate 'packet dropped'.

NF_DROP means 'please call kfree_skb for me'.  Problem from introspection point
of view is that drop monitor will blame nf_hook_slow() (for netfilter)
and ovs resp. act_ct for the drop.

Plan is to allow conntrack/nat engine to return STOLEN verdict ("skb
might have been free'd already").

Example change:
@@ -52,10 +53,8 @@ nf_nat_masquerade_ipv4(struct sk_buff *skb, unsigned int hooknum,
        rt = skb_rtable(skb);
        nh = rt_nexthop(rt, ip_hdr(skb)->daddr);
        newsrc = inet_select_addr(out, nh, RT_SCOPE_UNIVERSE);
-       if (!newsrc) {
-               pr_info("%s ate my IP address\n", out->name);
-               return NF_DROP;
-       }
+       if (!newsrc)
+               return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EADDRNOTAVAIL);


Where NF_DROP_REASON() is:

static __always_inline int
NF_DROP_REASON(struct sk_buff *skb, enum skb_drop_reason reason, u32 err)
{
        BUILD_BUG_ON(err > 0xffff);

        kfree_skb_reason(skb, reason);

        return ((err << 16) | NF_STOLEN);
}

So drop monitoring tools will blame nf_nat_masquerade.c:nf_nat_masquerade_ipv4 and not
the consumer of the NF_DROP verdict.

I can't make such changes ATM because ovs and act_ct assume conntrack
returns only ACCEPT and DROP, so we'd get double-free.  Hope that makes
sense.

Thanks!