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

[Aaron Conole <aconole@redhat.com>]

Florian Westphal <fw@strlen.de> writes:

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

Makes sense to me, thanks!