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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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 <pablo@netfilter.org>
> > ---
> > 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/589d243b-3d88-4138-9786-1bbb4347e79d@app.fastmail.com/

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.