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

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

Hi Florian,

On Thu, Aug 06, 2026 at 08:01:57PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Why must the blob be rebuilt before stale node purge in rbtree case?
> > 
> > ... there is a gap between the ruleset blob is built and published and
> > the set .commit interface is called to publish the new version of the
> > rbtree/pipapo datastructure.
> > 
> > See:
> > https://lore.kernel.org/netfilter-devel/589d243b-3d88-4138-9786-1bbb4347e79d@app.fastmail.com/
> 
> Ah. That makes sense. So the problem is not rbtree specific.  Problem is
> that packets switch over to the *new base chain* (linked to nf machinery,
> nft rule blob becomes reachable on base_seq swap:
> 
> 1. commit phase starts.
> 2. seqcount gets bumped.
>  A.  	Packet p1 enters machinery
> 3. elements get purged / chains / tables unlinked, netlink notificatons
>    etc. etc.
>  B.     Packet p1 is in nft_lookup, which gets updated base_seq,
>         but no match because rbtree blob resp. pipapo live blob
> 	are empty in the 'flush ruleset; table t { ..' case.
> 4. nft_set_commit_update() is called.

Yes. The empty set is exposed to the packet path for a short time span,
until nft_set_commit_update() is called.

> If the above is right your patch makes much more sense now :-)
> 
> The logic with (set->ops->abort_skip_removal && early_commit)
> however is hard to grasp.

I can add a specific .early_commit in v3 as Fernando suggested.

> Even with NFT_COMMIT_PHASE_EARLY or whatever its bad because
> the set implementation details leak into the transaction phase.
> But I understand that you'd like to at least solve it for rbtree
> with a smaller change, so thats ok.
>
> Is there a long-term plan?
>
> Maybe your 'pre-commit' phase could iterate the transaction
> log and relink transactions (add/del/update of set elements
> ) to the owning set?  Contradicting transactions (destroy set x,
> then remove element from x) should have been caught earlier, so
> this delete-from-transaction-log-and-link-to-per-set-struct should
> not be a problem.  Perhaps we might see issues with changes in the
> netlink reporting order... but thats hopefully easy to avoid.
>
> the ->commit() callback could then access the pending transactions
> for the set (element adds/deletes) and always get invoked early.
>
> Pipapo could walk its specific elem deletions internally, then swap.
> 
> The only other issue I see is that we need a second
> list_for_each_entry_safe(trans, next, &nft_net->commit_list, list)
> 
> ... walk, because the step-1 walk is allowed to fail.

I was considering to address this from the pipapo datastructure itself.
The idea would be similar to what the rbtree does, which is to offer a
front-end pipapo datastructure that is exposed to control plane that can
be updated under the commit mutex and that is not exposed to packet
path. Then use it from this new .early_commit interface to clone it,
then publish the new version and release the stale via RCU.

Same approach for the bitmap set.

As for the non-rhashtable version, it could be possible to build a
compact hashtable based on the control plane datastructure, ie.
rhashtable would be only used for control plane and a compact hashtable
(using array for buckets and chains) will be built from the new
.early_commit interface to publish to datapath.

Exception are dynsets, where a single rhashtable can be used, both to
control and datapath.