Re: [net,2/2] netfilter: nf_tables: unconditionally bump set->nelems before insertion

On Thu, Feb 26, 2026 at 09:19:37AM +0100, Florian Westphal wrote:

Jakub Kicinski [off-list ref] wrote:

quoted

This is an AI-generated review of your patch. The human sending this
email says "Is this legit?"

Looks legit to me.

quoted

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
netfilter: nf_tables: unconditionally bump set->nelems before insertion

This commit addresses a race condition where a new element gets published
then removed without waiting for the RCU grace period when the set is full.
The fix moves the nelems increment before insertion and adds cleanup logic
using the abort path to safely unwind the set.

quoted

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 0c5a4855b97d..834736237b09 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c

@@ -7171,6 +7171,7 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	struct nft_data_desc desc;
 	enum nft_registers dreg;
 	struct nft_trans *trans;
+	bool set_full = false;
 	u64 expiration;
 	u64 timeout;
 	int err, i;

@@ -7462,10 +7463,18 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 	if (err < 0)
 		goto err_elem_free;

+	if (!(flags & NFT_SET_ELEM_CATCHALL)) {
+		unsigned int max = nft_set_maxsize(set), nelems;
+
+		nelems = atomic_inc_return(&set->nelems);
+		if (nelems > max)
+			set_full = true;
+	}
+
 	trans = nft_trans_elem_alloc(ctx, NFT_MSG_NEWSETELEM, set);
 	if (trans == NULL) {
 		err = -ENOMEM;
-		goto err_elem_free;
+		goto err_set_size;
 	}

 	ext->genmask = nft_genmask_cur(ctx->net);

@@ -7517,7 +7526,7 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,

 					ue->priv = elem_priv;
 					nft_trans_commit_list_add_elem(ctx->net, trans);
-					goto err_elem_free;
+					goto err_set_size;
 				}
 			}
 		}

@@ -7535,23 +7544,16 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
 		goto err_element_clash;
 	}

-	if (!(flags & NFT_SET_ELEM_CATCHALL)) {
-		unsigned int max = nft_set_maxsize(set);
-
-		if (!atomic_add_unless(&set->nelems, 1, max)) {
-			err = -ENFILE;
-			goto err_set_full;
-		}
-	}
-
 	nft_trans_container_elem(trans)->elems[0].priv = elem.priv;
 	nft_trans_commit_list_add_elem(ctx->net, trans);
-	return 0;

-err_set_full:
-	nft_setelem_remove(ctx->net, set, elem.priv);
+	return set_full ? -ENFILE : 0;
+
 err_element_clash:
 	kfree(trans);
+err_set_size:
+	if (!(flags & NFT_SET_ELEM_CATCHALL) && !set_full)
+		atomic_dec(&set->nelems);

Does this properly handle the element update path when the set is full?

Pablo, why does that check !set_full?  The increment is unconditional,
so why is the decrement asymmetric?

err_set_size:
        if (!(flags & NFT_SET_ELEM_CATCHALL))
                atomic_dec(&set->nelems);

?

I think so, this is a leftover from initial patches that where still
using conditional atomic_add_unless().

I'm preparing a re-spin.