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

[PATCH nf v3] 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.

Fixed in xt_connlimit too.

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>
---
v2: use nf_ct_is_confirmed(), add comment about why the gc call is
needed and fix this in xt_connlimit too.

v3: call gc only if the condition is not met and we are not certain that
the list is updated
---
 net/netfilter/nft_connlimit.c | 22 +++++++++++++++++++---
 net/netfilter/xt_connlimit.c  | 14 ++++++++++++--
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nft_connlimit.c b/net/netfilter/nft_connlimit.c
index fc35a11cdca2..d4a132e38199 100644
--- a/net/netfilter/nft_connlimit.c
+++ b/net/netfilter/nft_connlimit.c
@@ -29,6 +29,7 @@ static inline void nft_connlimit_do_eval(struct nft_connlimit *priv,
 	struct nf_conntrack_tuple tuple;
 	enum ip_conntrack_info ctinfo;
 	const struct nf_conn *ct;
+	bool updated = false;
 	unsigned int count;
 
 	tuple_ptr = &tuple;
@@ -43,16 +44,31 @@ 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 (!ct || !nf_ct_is_confirmed(ct)) {
+		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
+			regs->verdict.code = NF_DROP;
+			return;
+		}
+		updated = true;
 	}
 
+check:
 	count = READ_ONCE(priv->list->count);
 
 	if ((count > priv->limit) ^ priv->invert) {
 		regs->verdict.code = NFT_BREAK;
 		return;
+	} else if (!updated) {
+		/* Call gc to update the list count if any connection has been
+		 * closed already. This is useful to softlimit connections
+		 * like limiting bandwidth based on a number of open
+		 * connections.
+		 */
+		local_bh_disable();
+		nf_conncount_gc_list(nft_net(pkt), priv->list);
+		local_bh_enable();
+		updated = true;
+		goto check;
 	}
 }
 
diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index 0189f8b6b0bd..5c90e1929d86 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -69,8 +69,18 @@ connlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
 		key[1] = zone->id;
 	}
 
-	connections = nf_conncount_count(net, info->data, key, tuple_ptr,
-					 zone);
+	if (!ct || !nf_ct_is_confirmed(ct)) {
+		connections = nf_conncount_count(net, info->data, key, tuple_ptr,
+						 zone);
+	} else {
+		/* Call nf_conncount_count() with NULL tuple and zone to update
+		 * the list if any connection has been closed already. This is
+		 * useful to softlimit connections like limiting bandwidth based
+		 * on a number of open connections.
+		 */
+		connections = nf_conncount_count(net, info->data, key, NULL, NULL);
+	}
+
 	if (connections == 0)
 		/* kmalloc failed, drop it entirely */
 		goto hotdrop;
-- 
2.51.0

Re: [PATCH nf v3] 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
>         }
> }
> 
> 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.
> +	if (!ct || !nf_ct_is_confirmed(ct)) {
> +		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
> +			regs->verdict.code = NF_DROP;
> +			return;
> +		}

This means the bug fix won't work when this is hooked before
conntrack.

> diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
> index 0189f8b6b0bd..5c90e1929d86 100644
> --- a/net/netfilter/xt_connlimit.c
> +++ b/net/netfilter/xt_connlimit.c
> @@ -69,8 +69,18 @@ connlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
>  		key[1] = zone->id;
>  	}
>  
> -	connections = nf_conncount_count(net, info->data, key, tuple_ptr,
> -					 zone);
> +	if (!ct || !nf_ct_is_confirmed(ct)) {
> +		connections = nf_conncount_count(net, info->data, key, tuple_ptr,
> +						 zone);

Same here, but usage in -t raw is legal/allowed.

I would suggest to rework this api so that this always passes in
struct nf_conn *ct, either derived from sk_buff or obtained via
nf_conntrack_find_get().

The net, tuple_ptr, and zone argument would be obsoleted and
replaced with nf_conn *ct arg.

This allows the nf_conncount internals to always skip the
insertion for !confirmed case, and the existing suppression
for same-jiffy collect could remain in place as well.

Its more work but since this has been broken forever I don't
think we need a urgent/small fix for this.

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

[Fernando Fernandez Mancera]

On 11/4/25 12:18 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
>>          }
>> }
>>
>> 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.
>> +	if (!ct || !nf_ct_is_confirmed(ct)) {
>> +		if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
>> +			regs->verdict.code = NF_DROP;
>> +			return;
>> +		}
> 
> This means the bug fix won't work when this is hooked before
> conntrack.
> 
>> diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
>> index 0189f8b6b0bd..5c90e1929d86 100644
>> --- a/net/netfilter/xt_connlimit.c
>> +++ b/net/netfilter/xt_connlimit.c
>> @@ -69,8 +69,18 @@ connlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
>>   		key[1] = zone->id;
>>   	}
>>   
>> -	connections = nf_conncount_count(net, info->data, key, tuple_ptr,
>> -					 zone);
>> +	if (!ct || !nf_ct_is_confirmed(ct)) {
>> +		connections = nf_conncount_count(net, info->data, key, tuple_ptr,
>> +						 zone);
> 
> Same here, but usage in -t raw is legal/allowed.
> 
> I would suggest to rework this api so that this always passes in
> struct nf_conn *ct, either derived from sk_buff or obtained via
> nf_conntrack_find_get().
> 
> The net, tuple_ptr, and zone argument would be obsoleted and
> replaced with nf_conn *ct arg.
> 
> This allows the nf_conncount internals to always skip the
> insertion for !confirmed case, and the existing suppression
> for same-jiffy collect could remain in place as well.
> 
> Its more work but since this has been broken forever I don't
> think we need a urgent/small fix for this.

Fair enough, I will handle this in a bigger patch aimed for nf-next 
then. I do not think we should touch the API on 6.18 given we are at rc4 
already.

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

[Florian Westphal]

Fernando Fernandez Mancera <fmancera@suse.de> wrote:
> > The net, tuple_ptr, and zone argument would be obsoleted and
> > replaced with nf_conn *ct arg.
> > 
> > This allows the nf_conncount internals to always skip the
> > insertion for !confirmed case, and the existing suppression
> > for same-jiffy collect could remain in place as well.
> > 
> > Its more work but since this has been broken forever I don't
> > think we need a urgent/small fix for this.
> 
> Fair enough, I will handle this in a bigger patch aimed for nf-next 
> then. I do not think we should touch the API on 6.18 given we are at rc4 
> already.

Makes sense to me.  Thanks Fernando.