[PATCH nf] netfilter: nf_tables: inconditionally bump set->nelems before insertion

[PATCH nf] netfilter: nf_tables: inconditionally bump set->nelems before insertion

[Pablo Neira Ayuso]

In case that the set is full, a new element gets published then removed
without waiting for the RCU grace period, while RCU reader can be
walking over it already.

To address this issue, add the element transaction even if set is full,
but toggle the set_full flag to report -ENFILE so the abort path safely
unwinds the set to its previous state.

As for element updates, decrement set->nelems to restore it.

Fixes: 35d0ac9070ef ("netfilter: nf_tables: fix set->nelems counting with no NLM_F_EXCL")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

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);
 err_elem_free:
 	nf_tables_set_elem_destroy(ctx, set, elem.priv);
 err_parse_data:
-- 
2.47.3

Re: [PATCH nf] netfilter: nf_tables: inconditionally bump set->nelems before insertion

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> In case that the set is full, a new element gets published then removed
> without waiting for the RCU grace period, while RCU reader can be
> walking over it already.
> 
> To address this issue, add the element transaction even if set is full,
> but toggle the set_full flag to report -ENFILE so the abort path safely
> unwinds the set to its previous state.
> 
> As for element updates, decrement set->nelems to restore it.

While I think this patch is correct and fixes the bug, I would
prefer the one-liner from Inseo An, it will be easier to backport it.
I propose we do this:

I do a nf pull request now, with Inseos version.

Then, after that has been merged back into nf-next, rebase this patch
on top of it and apply it.

Then, in 2nd step, also rework 71e99ee20fc3 ("netfilter: nf_tables: fix use-after-free in nf_tables_addchain()")
to follow same pattern as in your patch, i.e. defer the release to the
abort path instead.  This way we have easier to backport fixes while we
establish this new pattern of adding to-be-aborted transaction objects to
the list.

Makes sense to you?

Re: [PATCH nf] netfilter: nf_tables: inconditionally bump set->nelems before insertion

[Pablo Neira Ayuso]

On Tue, Feb 24, 2026 at 07:55:26PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > In case that the set is full, a new element gets published then removed
> > without waiting for the RCU grace period, while RCU reader can be
> > walking over it already.
> > 
> > To address this issue, add the element transaction even if set is full,
> > but toggle the set_full flag to report -ENFILE so the abort path safely
> > unwinds the set to its previous state.
> > 
> > As for element updates, decrement set->nelems to restore it.
> 
> While I think this patch is correct and fixes the bug, I would
> prefer the one-liner from Inseo An, it will be easier to backport it.
>
> I propose we do this:
> 
> I do a nf pull request now, with Inseos version.
> 
> Then, after that has been merged back into nf-next, rebase this patch
> on top of it and apply it.
> 
> Then, in 2nd step, also rework 71e99ee20fc3 ("netfilter: nf_tables: fix use-after-free in nf_tables_addchain()")
> to follow same pattern as in your patch, i.e. defer the release to the
> abort path instead.  This way we have easier to backport fixes while we
> establish this new pattern of adding to-be-aborted transaction objects to
> the list.
> 
> Makes sense to you?

My concern is that this slows down a scenario that is possible, ie.
adding an element to a full set.

... compared to 71e99ee20fc3, where it is almost *impossible* to reach
that synchronize_rcu() in a real use-case since you have to register
1024 basechains.

Re: [PATCH nf] netfilter: nf_tables: inconditionally bump set->nelems before insertion

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Makes sense to you?
> 
> My concern is that this slows down a scenario that is possible, ie.
> adding an element to a full set.
> 
> ... compared to 71e99ee20fc3, where it is almost *impossible* to reach
> that synchronize_rcu() in a real use-case since you have to register
> 1024 basechains.

Thats a good point, actually.  Let me think about it. I'll do a nf-next
PR now and will get back to this today.