[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f7ttth6njse.fsf@redhat.com>
Date: Wed, 03 Jul 2024 19:21:53 -0400
From: Aaron Conole <aconole@...hat.com>
To: Florian Westphal <fw@...len.de>
Cc: dev@...nvswitch.org, Dumitru Ceara <dceara@...hat.com>,
netdev@...r.kernel.org
Subject: Re: [ovs-dev] [PATCH net-next] openvswitch: prepare for stolen
verdict coming from conntrack and nat engine
Florian Westphal <fw@...len.de> writes:
> Aaron Conole <aconole@...hat.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@...len.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!
Powered by blists - more mailing lists