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

Pablo Neira Ayuso <[email protected]> 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/[email protected]/

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.

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.

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.