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

Pablo Neira Ayuso
2026-02-26T16:28:12Z

On Thu, Feb 26, 2026 at 09:19:37AM +0100, Florian Westphal wrote:
> Jakub Kicinski <kuba@kernel.org> wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email says "Is this legit?"
>
> Looks legit to me.
>
> > 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.
> >
> > > 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.