[PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Fernando Fernandez Mancera]

Connlimit expression can be used for all kind of packets and not only
for packets with connection state new. See this ruleset as example:

table ip filter {
        chain input {
                type filter hook input priority filter; policy accept;
                tcp dport 22 ct count over 4 counter
        }
}

Currently, if the connection count goes over the limit the counter will
count the packets. When a connection is closed, the connection count
won't decrement as it should because it is only updated for new
connections due to an optimization on __nf_conncount_add() that prevents
updating the list if the connection is duplicated.

In addition, since commit d265929930e2 ("netfilter: nf_conncount: reduce
unnecessary GC") there can be situations where a duplicated connection
is added to the list. This is caused by two packets from the same
connection being processed during the same jiffy.

To solve these problems, check whether this is a new connection and only
add the connection to the list if that is the case during connlimit
evaluation. Otherwise run a GC to update the count. This doesn't yield a
performance degradation.

Fixes: d265929930e2 ("netfilter: nf_conncount: reduce unnecessary GC")
Fixes: 976afca1ceba ("netfilter: nf_conncount: Early exit in nf_conncount_lookup() and cleanup")
Closes: https://lore.kernel.org/netfilter/trinity-85c72a88-d762-46c3-be97-36f10e5d9796-1761173693813@3c-app-mailcom-bs12/
Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
---
 net/netfilter/nft_connlimit.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nft_connlimit.c b/net/netfilter/nft_connlimit.c
index fc35a11cdca2..19c8b5377e35 100644
--- a/net/netfilter/nft_connlimit.c
+++ b/net/netfilter/nft_connlimit.c
@@ -43,9 +43,15 @@ static inline void nft_connlimit_do_eval(struct nft_connlimit *priv,
 		return;
 	}
 
-	if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
-		regs->verdict.code = NF_DROP;
-		return;
+	if (ctinfo == IP_CT_NEW) {
+		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
+			regs->verdict.code = NF_DROP;
+			return;
+		}
+	} else {
+		local_bh_disable();
+		nf_conncount_gc_list(nft_net(pkt), priv->list);
+		local_bh_enable();
 	}
 
 	count = READ_ONCE(priv->list->count);
-- 
2.51.0

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> Connlimit expression can be used for all kind of packets and not only
> for packets with connection state new. See this ruleset as example:
> 
> table ip filter {
>         chain input {
>                 type filter hook input priority filter; policy accept;
>                 tcp dport 22 ct count over 4 counter
>         }
> }

Right.  Would you mind sending a patch for nftables documentation to
recommend combining this with ct state new or similar?

> +	if (ctinfo == IP_CT_NEW) {
> +		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> +			regs->verdict.code = NF_DROP;
> +			return;
> +		}
> +	} else {
> +		local_bh_disable();
> +		nf_conncount_gc_list(nft_net(pkt), priv->list);
> +		local_bh_enable();

LGTM, we could make a __nf_conncount_gc_list and make
nf_conncount_gc_list not need BH disable (and do the jiffies
check first w.o. BH disable) but I think in the interest of
a small patch and given that we should not be dealing with
!new packets anyway I think this change is fine.

Thanks Fernando for fixing this up.

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Fernando Fernandez Mancera]

On 10/27/25 2:47 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> Connlimit expression can be used for all kind of packets and not only
>> for packets with connection state new. See this ruleset as example:
>>
>> table ip filter {
>>          chain input {
>>                  type filter hook input priority filter; policy accept;
>>                  tcp dport 22 ct count over 4 counter
>>          }
>> }
> 
> Right.  Would you mind sending a patch for nftables documentation to
> recommend combining this with ct state new or similar?
> 

Sure, this also requires a documentation fix on the nftables wiki [1] 
but I believe I do not have an account. How could create one for me? :)

[1] https://wiki.nftables.org/wiki-nftables/index.php/Connlimits

Thanks,
Fernando.

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Pablo Neira Ayuso]

On Mon, Oct 27, 2025 at 01:57:30PM +0100, Fernando Fernandez Mancera wrote:

> diff --git a/net/netfilter/nft_connlimit.c b/net/netfilter/nft_connlimit.c
> index fc35a11cdca2..19c8b5377e35 100644
> --- a/net/netfilter/nft_connlimit.c
> +++ b/net/netfilter/nft_connlimit.c
> @@ -43,9 +43,15 @@ static inline void nft_connlimit_do_eval(struct nft_connlimit *priv,
>  		return;
>  	}
>  
> -	if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> -		regs->verdict.code = NF_DROP;
> -		return;
> +	if (ctinfo == IP_CT_NEW) {

Quick question: Is this good enough to deal with retransmitted packets
while in NEW state?

> +		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> +			regs->verdict.code = NF_DROP;
> +			return;
> +		}
> +	} else {
> +		local_bh_disable();
> +		nf_conncount_gc_list(nft_net(pkt), priv->list);
> +		local_bh_enable();
>  	}
>  
>  	count = READ_ONCE(priv->list->count);
> -- 
> 2.51.0
> 
> 

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > -	if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> > -		regs->verdict.code = NF_DROP;
> > -		return;
> > +	if (ctinfo == IP_CT_NEW) {
> 
> Quick question: Is this good enough to deal with retransmitted packets
> while in NEW state?

Good point, I did not consider retransmit case.
What about if (!nf_ct_is_confirmed(ct)) { ..?

Would need a small comment that this is there instead of NEW
check due to rexmit.

> > +		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> > +			regs->verdict.code = NF_DROP;
> > +			return;
> > +		}
> > +	} else {
> > +		local_bh_disable();
> > +		nf_conncount_gc_list(nft_net(pkt), priv->list);
> > +		local_bh_enable();
> >  	}

As this needs a respin anyway, what about removing the else
clause altogether?  Resp. what was the rationale for doing a gc call
here?

And, do we want same check in xt_connlimit?

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Pablo Neira Ayuso]

On Tue, Oct 28, 2025 at 06:06:47PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > -	if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> > > -		regs->verdict.code = NF_DROP;
> > > -		return;
> > > +	if (ctinfo == IP_CT_NEW) {
> > 
> > Quick question: Is this good enough to deal with retransmitted packets
> > while in NEW state?
> 
> Good point, I did not consider retransmit case.
> What about if (!nf_ct_is_confirmed(ct)) { ..?
> 
> Would need a small comment that this is there instead of NEW
> check due to rexmit.
> 
> > > +		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> > > +			regs->verdict.code = NF_DROP;
> > > +			return;
> > > +		}
> > > +	} else {
> > > +		local_bh_disable();
> > > +		nf_conncount_gc_list(nft_net(pkt), priv->list);
> > > +		local_bh_enable();
> > >  	}
> 
> As this needs a respin anyway, what about removing the else
> clause altogether?  Resp. what was the rationale for doing a gc call
> here?

You mean, call gc for both cases, correct?

> And, do we want same check in xt_connlimit?

Are you referring to the !nf_ct_is_confirmed() check?

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Oct 28, 2025 at 06:06:47PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > -	if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> > > > -		regs->verdict.code = NF_DROP;
> > > > -		return;
> > > > +	if (ctinfo == IP_CT_NEW) {
> > > 
> > > Quick question: Is this good enough to deal with retransmitted packets
> > > while in NEW state?
> > 
> > Good point, I did not consider retransmit case.
> > What about if (!nf_ct_is_confirmed(ct)) { ..?
> > 
> > Would need a small comment that this is there instead of NEW
> > check due to rexmit.
> > 
> > > > +		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> > > > +			regs->verdict.code = NF_DROP;
> > > > +			return;
> > > > +		}
> > > > +	} else {
> > > > +		local_bh_disable();
> > > > +		nf_conncount_gc_list(nft_net(pkt), priv->list);
> > > > +		local_bh_enable();
> > > >  	}
> > 
> > As this needs a respin anyway, what about removing the else
> > clause altogether?  Resp. what was the rationale for doing a gc call
> > here?
> 
> You mean, call gc for both cases, correct?

No, i meant this:

/* comment that explains this */
if (nf_ct_is_confirmed(ct))
	return;

if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
	regs->verdict.code = NF_DROP;
	return;
}

As we don't add anything for 'is confirmed' case, I don't see why
we would need to prune the existing list.

> > And, do we want same check in xt_connlimit?
> 
> Are you referring to the !nf_ct_is_confirmed() check?

Yes, AFAICS xt_connlimit has same issue as nft_conncount given
they use same backend.

Backend doesn't have access to nf_conn/ctinfo so no way to solve it there.

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Fernando Fernandez Mancera]

On 10/28/25 6:26 PM, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> On Tue, Oct 28, 2025 at 06:06:47PM +0100, Florian Westphal wrote:
>>> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>>>>> -	if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
>>>>> -		regs->verdict.code = NF_DROP;
>>>>> -		return;
>>>>> +	if (ctinfo == IP_CT_NEW) {
>>>>
>>>> Quick question: Is this good enough to deal with retransmitted packets
>>>> while in NEW state?
>>>
>>> Good point, I did not consider retransmit case.
>>> What about if (!nf_ct_is_confirmed(ct)) { ..?
>>>
>>> Would need a small comment that this is there instead of NEW
>>> check due to rexmit.
>>>
>>>>> +		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
>>>>> +			regs->verdict.code = NF_DROP;
>>>>> +			return;
>>>>> +		}
>>>>> +	} else {
>>>>> +		local_bh_disable();
>>>>> +		nf_conncount_gc_list(nft_net(pkt), priv->list);
>>>>> +		local_bh_enable();
>>>>>   	}
>>>
>>> As this needs a respin anyway, what about removing the else
>>> clause altogether?  Resp. what was the rationale for doing a gc call
>>> here?
>>
>> You mean, call gc for both cases, correct?
> 
> No, i meant this:
> 
> /* comment that explains this */
> if (nf_ct_is_confirmed(ct))
> 	return;
> 
> if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> 	regs->verdict.code = NF_DROP;
> 	return;
> }
> 
> As we don't add anything for 'is confirmed' case, I don't see why
> we would need to prune the existing list.
> 

We need this gc call, it is what fixes the use-case reported by the 
user. If the user is using this expression without a ct state new check, 
we must check if some connection closed already and update the 
connection count properly, then evaluate if the connection count greater 
than the limit for all the packets.

Otherwise, this change will introduce a change in behavior.. AFAICT

>>> And, do we want same check in xt_connlimit?
>>
>> Are you referring to the !nf_ct_is_confirmed() check?
> 
> Yes, AFAICS xt_connlimit has same issue as nft_conncount given
> they use same backend.
> 
> Backend doesn't have access to nf_conn/ctinfo so no way to solve it there.

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> We need this gc call, it is what fixes the use-case reported by the 
> user. If the user is using this expression without a ct state new check, 
> we must check if some connection closed already and update the 
> connection count properly, then evaluate if the connection count greater 
> than the limit for all the packets.

I don't think so.  AFAICS the NEW/!confirmed check is enough, a
midstream packet (established connection) isn't added anymore so 'ct
count' can't go over the budget.

If last real-add brought us over the budget, then it wasn't added
(we were over budget), so next packet of existing flow will still be
within budget.

Does that make sense to you?

> Otherwise, this change will introduce a change in behavior.. AFAICT

Yes, the existing behaviour is random?  (Due to multiple re-adds).
With NEW/!confirmed check: no multiadd possible, so no need to ad-hoc
gc.

Did I miss anything?

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Fernando Fernandez Mancera]

On 10/28/25 7:33 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> We need this gc call, it is what fixes the use-case reported by the
>> user. If the user is using this expression without a ct state new check,
>> we must check if some connection closed already and update the
>> connection count properly, then evaluate if the connection count greater
>> than the limit for all the packets.
> 
> I don't think so.  AFAICS the NEW/!confirmed check is enough, a
> midstream packet (established connection) isn't added anymore so 'ct
> count' can't go over the budget.
> 
> If last real-add brought us over the budget, then it wasn't added
> (we were over budget), so next packet of existing flow will still be
> within budget.
> 
> Does that make sense to you?
> 

It does for standard use case but not for "inverted" flag - the 
expression will continue matching and letting packets pass even if count 
is NOT over the limit anymore because the count is not being updated 
until a new connection arrives.

>> Otherwise, this change will introduce a change in behavior.. AFAICT
> 
> Yes, the existing behaviour is random?  (Due to multiple re-adds).
> With NEW/!confirmed check: no multiadd possible, so no need to ad-hoc
> gc.
> 
> Did I miss anything?

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> On 10/28/25 7:33 PM, Florian Westphal wrote:
> > Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> >> We need this gc call, it is what fixes the use-case reported by the
> >> user. If the user is using this expression without a ct state new check,
> >> we must check if some connection closed already and update the
> >> connection count properly, then evaluate if the connection count greater
> >> than the limit for all the packets.
> > 
> > I don't think so.  AFAICS the NEW/!confirmed check is enough, a
> > midstream packet (established connection) isn't added anymore so 'ct
> > count' can't go over the budget.
> > 
> > If last real-add brought us over the budget, then it wasn't added
> > (we were over budget), so next packet of existing flow will still be
> > within budget.
> > 
> > Does that make sense to you?
> > 
> 
> It does for standard use case but not for "inverted" flag - the 
> expression will continue matching and letting packets pass even if count 
> is NOT over the limit anymore because the count is not being updated 
> until a new connection arrives.

I don't really see how.  Empty ruleset with single

'ct count over 3 reject'

... is broken flat out broken. I mean, whats that supposed to do?

1st connection comes in -> 1
2nd connection comes in -> 2
3rd connection comes in -> 3
4th connection comes in -> 4 -> rejects happen (for all matching _packets_!)

The extra gc doesn't change anything here except that when one
connection has closed this gets 'healed'.  But I argue that this is
nonsensical ruleset, given the connection has to time out (even fin/rst etc.
won't pass, so normal closing possible).

If we look at a better example:

ct state new ct count over 3 reject
(or same as before but with earlier 'ct state established accept'):

1st connection comes in -> 1
..
4th connection comes in -> 4 -> new connection attempt gets rejected

... some connection closes ...

-> next connection attempt passes, as one entry is reaped and replaced
with another one.

Can you describe a sane failing case?
It should be in a comment if we need to retain the else branch since I
can't come up with a single example of where its needed.

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Fernando Fernandez Mancera]

On 10/28/25 8:10 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> On 10/28/25 7:33 PM, Florian Westphal wrote:
>>> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>>>> We need this gc call, it is what fixes the use-case reported by the
>>>> user. If the user is using this expression without a ct state new check,
>>>> we must check if some connection closed already and update the
>>>> connection count properly, then evaluate if the connection count greater
>>>> than the limit for all the packets.
>>>
>>> I don't think so.  AFAICS the NEW/!confirmed check is enough, a
>>> midstream packet (established connection) isn't added anymore so 'ct
>>> count' can't go over the budget.
>>>
>>> If last real-add brought us over the budget, then it wasn't added
>>> (we were over budget), so next packet of existing flow will still be
>>> within budget.
>>>
>>> Does that make sense to you?
>>>
>>
>> It does for standard use case but not for "inverted" flag - the
>> expression will continue matching and letting packets pass even if count
>> is NOT over the limit anymore because the count is not being updated
>> until a new connection arrives.
> 
> I don't really see how.  Empty ruleset with single
> 
> 'ct count over 3 reject'
> 
> ... is broken flat out broken. I mean, whats that supposed to do?
> 
> 1st connection comes in -> 1
> 2nd connection comes in -> 2
> 3rd connection comes in -> 3
> 4th connection comes in -> 4 -> rejects happen (for all matching _packets_!)
> 
> The extra gc doesn't change anything here except that when one
> connection has closed this gets 'healed'.  But I argue that this is
> nonsensical ruleset, given the connection has to time out (even fin/rst etc.
> won't pass, so normal closing possible).
> 

The use-case I have on mind (which is similar to what user described, 
but he uses a counter which I guess is just for debugging):

ip saddr 192.168.1.100 tcp dport 22 ct counter over 4 mark set 0x1

later, the mark can be used for tc or policy-based routing - e.g 
limiting bandwidth if the ip address has too many connections open.

To me this seems a valid use case..

1st connection comes in -> 1
2nd connection comes in -> 2
3rd connection comes in -> 3
4th connection comes in -> 4

all the packets are now being marked with 0x1 and QoS is being performed 
with tc (limiting the bandwidth). Later, 1st and 2nd connection are 
closed but the packets from 3rd and 4th connection are still being 
marked as 0x1 until a new connnection arrives and the count is updated.

This behavior seems wrong to me.. and while I know it isn't supposed to 
be used without "ct state new" check, I could find some examples doing 
this on the internet..

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> The use-case I have on mind (which is similar to what user described, 
> but he uses a counter which I guess is just for debugging):
> 
> ip saddr 192.168.1.100 tcp dport 22 ct counter over 4 mark set 0x1
> 
> later, the mark can be used for tc or policy-based routing - e.g 
> limiting bandwidth if the ip address has too many connections open.
> 
> To me this seems a valid use case..

It is.  Please add a comment as to why the extra gc is needed.

Its not needed for the 'limit this address/network to only have
x concurrent connections'.

But it is for 'softlimit-like' things as you explained above
(which I failed to consider).  Thanks Fernando.

Re: [PATCH nf] netfilter: nft_connlimit: fix duplicated tracking of a connection

[Fernando Fernandez Mancera]

On 10/28/25 9:54 PM, Florian Westphal wrote:
> Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>> The use-case I have on mind (which is similar to what user described,
>> but he uses a counter which I guess is just for debugging):
>>
>> ip saddr 192.168.1.100 tcp dport 22 ct counter over 4 mark set 0x1
>>
>> later, the mark can be used for tc or policy-based routing - e.g
>> limiting bandwidth if the ip address has too many connections open.
>>
>> To me this seems a valid use case..
> 
> It is.  Please add a comment as to why the extra gc is needed.
> 
> Its not needed for the 'limit this address/network to only have
> x concurrent connections'.
> 
> But it is for 'softlimit-like' things as you explained above
> (which I failed to consider).  Thanks Fernando.
> 

Will send a v2, thank you Florian & Pablo!