Re: [PATCH nf,v2 2/2] netfilter: nf_tables: call set ops .commit when building new ruleset

On 8/6/26 4:35 PM, Pablo Neira Ayuso wrote:
> Hi Fernando,
> 
> On Thu, Aug 06, 2026 at 12:30:14PM +0200, Fernando Fernandez Mancera wrote:
>> On 8/5/26 7:11 PM, Pablo Neira Ayuso wrote:
>>> The rbtree set only builds the b-search array after the new ruleset has
>>> been exposed through set ops .commit.
>>>
>>> This is currently needed by pipapo because it purges the elements from
>>> the clone after the transactions are handled, therefore, pipapo still
>>> needs the delayed set ops .commit call after the transaction handling.
>>>
>>> Allow the rbtree to call .commit before the transaction handling which
>>> purges the stale elements from the frontend rbtree datastructure.
>>>
>>> Update rbtree .commit to skip deactivated and expired elements when
>>> building the new b-search array.
>>>
>>> Signed-off-by: Pablo Neira Ayuso <[email protected]>
>>> ---
>>> sashiko: there is a timestamp that prevents elements from expiring while
>>> handling the transaction.
>>>
>>> v2: - use NFT_GENMASK_ANY for GC scan, theoretically no new element in
>>>         this transaction can expire while handling the transaction but
>>>         let's just skip either new element or deactivated elements from
>>>         the GC scan.
>>>       - remove incorrect reset of previous rbe when performing GC scan.
>>>
>>>    net/netfilter/nf_tables_api.c  |  9 ++++++--
>>>    net/netfilter/nft_set_rbtree.c | 38 ++++++++++++++++++++++++++++------
>>>    2 files changed, 39 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>>> index 90a379533e08..a7006725c307 100644
>>> --- a/net/netfilter/nf_tables_api.c
>>> +++ b/net/netfilter/nf_tables_api.c
>>> @@ -10853,11 +10853,14 @@ static void nf_tables_commit_audit_log(struct list_head *adl, u32 generation)
>>>    	}
>>>    }
>>> -static void nft_set_commit_update(struct nftables_pernet *nft_net)
>>> +static void nft_set_commit_update(struct nftables_pernet *nft_net, bool early_commit)
>>>    {
>>>    	struct nft_set *set, *next;
>>>    	list_for_each_entry_safe(set, next, &nft_net->set_update_list, pending_update) {
>>> +		if (set->ops->abort_skip_removal && early_commit)
>>> +			continue;
>>> +
>>
>> Hi Pablo,
>>
>> wouldn't it be better to add the early_commit flag to set->ops? This way we
>> don't need to check abort_skip_removal flag.
>>
>> I could foresee problems in the future using the same flag for this skip..
>>
>> What about creating a nft_commit_phase enum:
>>
>> enum nft_commit_phase {
>> 	NFT_COMMIT_PHASE_EARLY,
>> 	NFT_COMMIT_PHASE_DELAYED,
>> };
>>
>> and then we can use it in nft_set_commit_update() as argument, something
>> like:
>>
>> static void nft_set_commit_update(struct nftables_pernet *nft_net,
>>                                    enum nft_commit_phase phase)
>>
>> This way, early_commit makes sense on set->ops..
>>
>> What do you think? I am trying to think in a way to make the code more
>> readable.
> 
> The (ab)use of ->abort_skip_removal is hack to address the reported
> regression with minimal changes.
> 
> I can add an .early_commit interface instead, but where I want to go
> is to move .commit before transaction handling, but pipapo needs to be
> revisited to make it. Because pipapo has the same issue reported here:
> 
> https://lore.kernel.org/netfilter-devel/[email protected]/
> 
> The update for pipapo would be targetted at the nf-next.
> 
> I can add a .early_commit, that would be cleaner and it is not a lot
> more code.
> 
> Eventually, .early_commit will just become .commit once pipapo becomes
> safe to be used before transaction handling.
> 

Sounds good to me, in essence I would like to avoid re-using a flag that 
isn't supposed to be used for that. In any case, if this is a temporary 
change it might be fine too..

Thanks!