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

Paolo Abeni
2026-02-26T17:19:56Z

On 2/26/26 5:28 PM, Pablo Neira Ayuso wrote:
> 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.

Please note that patch 1/2 as been already applied to the net tree.

/P