Re: [PATCH nf-next] netfilter: conntrack: remove flowtable early-drop test

Re: [PATCH nf-next] netfilter: conntrack: remove flowtable early-drop test

[Vlad Buslov]

On Tue 23 Apr 2024 at 15:44, Florian Westphal <fw@strlen.de> wrote:
> Not sure why this special case exists.  Early drop logic
> (which kicks in when conntrack table is full) should be independent
> of flowtable offload and only consider assured bit (i.e., two-way
> traffic was seen).
>
> flowtable entries hold a reference to the conntrack entry (struct
> nf_conn) that has been offloaded. The conntrack use count is not
> decremented until after the entry is free'd.
>
> This change therefore will not result in exceeding the conntrack table
> limit.  It does allow early-drop of tcp flows even when they've been
> offloaded, but only if they have been offloaded before syn-ack was
> received or after at least one peer has sent a fin.
>
> Currently 'fin' packet reception already stops offloading, so this
> should not impact offloading either.
>
> Cc: Vlad Buslov <vladbu@nvidia.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  Vlad, do you remember why you added this test?

I added it when I introduced UDP NEW connection offload. As far as I
remember the concern was that since at the time early drop algorithm
completely ignored all offloaded connections malicious user could fill
the whole table by just sending a single packet per range of distinct 5
tuples and none of the resulting connections would be early dropped
until they expire.

>
>  For reference, this came in
>  df25455e5a48 ("netfilter: nf_conntrack: allow early drop of offloaded UDP conns")
>  and maybe was just a 'move-it-around' from the check in
>  early_drop_list, which would mean this was there from the
>  beginning.  Doesn't change "i don't understand why this test
>  exists" though :-)
>
>  net/netfilter/nf_conntrack_core.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index c63868666bd9..43629e79067d 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1440,8 +1440,6 @@ static bool gc_worker_can_early_drop(const struct nf_conn *ct)
>  	const struct nf_conntrack_l4proto *l4proto;
>  	u8 protonum = nf_ct_protonum(ct);
>  
> -	if (test_bit(IPS_OFFLOAD_BIT, &ct->status) && protonum != IPPROTO_UDP)
> -		return false;
>  	if (!test_bit(IPS_ASSURED_BIT, &ct->status))
>  		return true;

Re: [PATCH nf-next] netfilter: conntrack: remove flowtable early-drop test

[Florian Westphal]

Vlad Buslov <vladbu@nvidia.com> wrote:
> > ---
> >  Vlad, do you remember why you added this test?
> 
> I added it when I introduced UDP NEW connection offload. As far as I
> remember the concern was that since at the time early drop algorithm
> completely ignored all offloaded connections malicious user could fill
> the whole table by just sending a single packet per range of distinct 5
> tuples and none of the resulting connections would be early dropped
> until they expire.

Ok, so it was indeed this:

> >  and maybe was just a 'move-it-around' from the check in
> >  early_drop_list, which would mean this was there from the
> >  beginning.  Doesn't change "i don't understand why this test
> >  exists" though :-)

In this case I think this change is fine, ie. remove offload
special treatment, its not needed.

Thanks for checking!

[PATCH nf-next] netfilter: conntrack: remove flowtable early-drop test

[Florian Westphal]

Not sure why this special case exists.  Early drop logic
(which kicks in when conntrack table is full) should be independent
of flowtable offload and only consider assured bit (i.e., two-way
traffic was seen).

flowtable entries hold a reference to the conntrack entry (struct
nf_conn) that has been offloaded. The conntrack use count is not
decremented until after the entry is free'd.

This change therefore will not result in exceeding the conntrack table
limit.  It does allow early-drop of tcp flows even when they've been
offloaded, but only if they have been offloaded before syn-ack was
received or after at least one peer has sent a fin.

Currently 'fin' packet reception already stops offloading, so this
should not impact offloading either.

Cc: Vlad Buslov <vladbu@nvidia.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Vlad, do you remember why you added this test?

 For reference, this came in
 df25455e5a48 ("netfilter: nf_conntrack: allow early drop of offloaded UDP conns")
 and maybe was just a 'move-it-around' from the check in
 early_drop_list, which would mean this was there from the
 beginning.  Doesn't change "i don't understand why this test
 exists" though :-)

 net/netfilter/nf_conntrack_core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index c63868666bd9..43629e79067d 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1440,8 +1440,6 @@ static bool gc_worker_can_early_drop(const struct nf_conn *ct)
 	const struct nf_conntrack_l4proto *l4proto;
 	u8 protonum = nf_ct_protonum(ct);
 
-	if (test_bit(IPS_OFFLOAD_BIT, &ct->status) && protonum != IPPROTO_UDP)
-		return false;
 	if (!test_bit(IPS_ASSURED_BIT, &ct->status))
 		return true;
 
-- 
2.43.2

Re: [PATCH nf-next] netfilter: conntrack: remove flowtable early-drop test

[Vlad Buslov]

On Tue 23 Apr 2024 at 15:05, Florian Westphal <fw@strlen.de> wrote:
> Vlad Buslov <vladbu@nvidia.com> wrote:
>> > ---
>> >  Vlad, do you remember why you added this test?
>> 
>> I added it when I introduced UDP NEW connection offload. As far as I
>> remember the concern was that since at the time early drop algorithm
>> completely ignored all offloaded connections malicious user could fill
>> the whole table by just sending a single packet per range of distinct 5
>> tuples and none of the resulting connections would be early dropped
>> until they expire.
>
> Ok, so it was indeed this:
>
>> >  and maybe was just a 'move-it-around' from the check in
>> >  early_drop_list, which would mean this was there from the
>> >  beginning.  Doesn't change "i don't understand why this test
>> >  exists" though :-)
>
> In this case I think this change is fine, ie. remove offload
> special treatment, its not needed.

The change will also enable early dropping offloaded non-ASSURED
connections for all other protocols though.

>
> Thanks for checking!

Re: [PATCH nf-next] netfilter: conntrack: remove flowtable early-drop test

[Florian Westphal]

Vlad Buslov <vladbu@nvidia.com> wrote:
> The change will also enable early dropping offloaded non-ASSURED
> connections for all other protocols though.

But I don't think we support that.
act_ct supports offload of non-assured (unidirectional) UDP.

But TCP needs to have seen two-way communication.
So, AFAICT the check isn't needed.