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

[Fernando Fernandez Mancera <fmancera@suse.de>]

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.
---
 net/netfilter/nft_connlimit.c | 17 ++++++++++++++---
 net/netfilter/xt_connlimit.c  | 14 ++++++++++++--
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nft_connlimit.c b/net/netfilter/nft_connlimit.c
index fc35a11cdca2..dedea1681e73 100644
--- a/net/netfilter/nft_connlimit.c
+++ b/net/netfilter/nft_connlimit.c
@@ -43,9 +43,20 @@ 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;
+		}
+	} else {
+		/* 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();
 	}
 
 	count = READ_ONCE(priv->list->count);
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