[PATCH nf,v2 1/2] netfilter: nf_tables: move set_update_list to nftables per-netns

[PATCH nf,v2 1/2] netfilter: nf_tables: move set_update_list to nftables per-netns

[Pablo Neira Ayuso]

This list is used to invoke the set .commit and .abort ops for the
rbtree and pipapo to run GC on expired elements and replace the current
datastructure view by the clone. For the rbtree, this also rebuild the
datapath b-search array.

From abort path, remove the set from the update_list if it is already
bound to rule, then the rule itself takes care of releasing the set and
its elements, otherwise, memleak is possible because set ops .abort
only deals with removing the set data structure, not the elements.

This is a preparation patch to call set .commit before processing the
transaction list for the rbtree, no functional changes are intended.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: comestic: s/make move/move/ in patch subject.

 include/net/netfilter/nf_tables.h |  1 +
 net/netfilter/nf_tables_api.c     | 49 ++++++++++---------------------
 2 files changed, 16 insertions(+), 34 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 3be612145c13..238f6ecb90e9 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -1949,6 +1949,7 @@ struct nftables_pernet {
 	struct list_head	binding_list;
 	struct list_head	module_list;
 	struct list_head	notify_list;
+	struct list_head	set_update_list;
 	struct mutex		commit_mutex;
 	u64			table_handle;
 	u64			tstamp;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index af357f6c5070..90a379533e08 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -595,10 +595,15 @@ static void nft_trans_commit_list_add_tail(struct net *net, struct nft_trans *tr
 static void nft_trans_commit_list_add_elem(struct net *net, struct nft_trans *trans)
 {
 	struct nftables_pernet *nft_net = nft_pernet(net);
+	struct nft_trans_elem *te;
 
 	WARN_ON_ONCE(trans->msg_type != NFT_MSG_NEWSETELEM &&
 		     trans->msg_type != NFT_MSG_DELSETELEM);
 
+	te = nft_trans_container_elem(trans);
+	if (te->set->ops->commit && list_empty(&te->set->pending_update))
+		list_add_tail(&te->set->pending_update, &nft_net->set_update_list);
+
 	if (nft_trans_try_collapse(nft_net, trans)) {
 		kfree(trans);
 		return;
@@ -10848,11 +10853,11 @@ static void nf_tables_commit_audit_log(struct list_head *adl, u32 generation)
 	}
 }
 
-static void nft_set_commit_update(struct list_head *set_update_list)
+static void nft_set_commit_update(struct nftables_pernet *nft_net)
 {
 	struct nft_set *set, *next;
 
-	list_for_each_entry_safe(set, next, set_update_list, pending_update) {
+	list_for_each_entry_safe(set, next, &nft_net->set_update_list, pending_update) {
 		list_del_init(&set->pending_update);
 
 		if (!set->ops->commit || set->dead)
@@ -10885,7 +10890,6 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 	struct nft_trans_binding *trans_binding;
 	struct nft_trans *trans, *next;
 	unsigned int base_seq, gc_seq;
-	LIST_HEAD(set_update_list);
 	struct nft_trans_elem *te;
 	struct nft_chain *chain;
 	struct nft_table *table;
@@ -11091,27 +11095,13 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 			break;
 		case NFT_MSG_NEWSETELEM:
 			te = nft_trans_container_elem(trans);
-
 			nft_trans_elems_add(&ctx, te);
-
-			if (te->set->ops->commit &&
-			    list_empty(&te->set->pending_update)) {
-				list_add_tail(&te->set->pending_update,
-					      &set_update_list);
-			}
 			nft_trans_destroy(trans);
 			break;
 		case NFT_MSG_DELSETELEM:
 		case NFT_MSG_DESTROYSETELEM:
 			te = nft_trans_container_elem(trans);
-
 			nft_trans_elems_remove(&ctx, te);
-
-			if (te->set->ops->commit &&
-			    list_empty(&te->set->pending_update)) {
-				list_add_tail(&te->set->pending_update,
-					      &set_update_list);
-			}
 			break;
 		case NFT_MSG_NEWOBJ:
 			if (nft_trans_obj_update(trans)) {
@@ -11180,7 +11170,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 		}
 	}
 
-	nft_set_commit_update(&set_update_list);
+	nft_set_commit_update(nft_net);
 
 	nft_commit_notify(net, NETLINK_CB(skb).portid);
 	nf_tables_gen_notify(net, skb, NFT_MSG_NEWGEN);
@@ -11247,11 +11237,11 @@ static void nf_tables_abort_release(struct nft_trans *trans)
 	kfree(trans);
 }
 
-static void nft_set_abort_update(struct list_head *set_update_list)
+static void nft_set_abort_update(struct nftables_pernet *nft_net)
 {
 	struct nft_set *set, *next;
 
-	list_for_each_entry_safe(set, next, set_update_list, pending_update) {
+	list_for_each_entry_safe(set, next, &nft_net->set_update_list, pending_update) {
 		list_del_init(&set->pending_update);
 
 		if (!set->ops->abort)
@@ -11386,33 +11376,22 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
 			nft_trans_destroy(trans);
 			break;
 		case NFT_MSG_NEWSETELEM:
+			te = nft_trans_container_elem(trans);
 			if (nft_trans_elem_set_bound(trans)) {
+				list_del_init(&te->set->pending_update);
 				nft_trans_destroy(trans);
 				break;
 			}
-			te = nft_trans_container_elem(trans);
 			if (!nft_trans_elems_new_abort(&ctx, te)) {
 				nft_trans_destroy(trans);
 				break;
 			}
-
-			if (te->set->ops->abort &&
-			    list_empty(&te->set->pending_update)) {
-				list_add_tail(&te->set->pending_update,
-					      &set_update_list);
-			}
 			break;
 		case NFT_MSG_DELSETELEM:
 		case NFT_MSG_DESTROYSETELEM:
 			te = nft_trans_container_elem(trans);
 
 			nft_trans_elems_destroy_abort(&ctx, te);
-
-			if (te->set->ops->abort &&
-			    list_empty(&te->set->pending_update)) {
-				list_add_tail(&te->set->pending_update,
-					      &set_update_list);
-			}
 			nft_trans_destroy(trans);
 			break;
 		case NFT_MSG_NEWOBJ:
@@ -11458,7 +11437,7 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
 
 	WARN_ON_ONCE(!list_empty(&nft_net->commit_set_list));
 
-	nft_set_abort_update(&set_update_list);
+	nft_set_abort_update(nft_net);
 
 	synchronize_rcu();
 
@@ -12142,6 +12121,7 @@ static int __net_init nf_tables_init_net(struct net *net)
 	INIT_LIST_HEAD(&nft_net->binding_list);
 	INIT_LIST_HEAD(&nft_net->module_list);
 	INIT_LIST_HEAD(&nft_net->notify_list);
+	INIT_LIST_HEAD(&nft_net->set_update_list);
 	mutex_init(&nft_net->commit_mutex);
 	net->nft.base_seq = 1;
 	nft_net->gc_seq = 0;
@@ -12186,6 +12166,7 @@ static void __net_exit nf_tables_exit_net(struct net *net)
 	WARN_ON_ONCE(!list_empty(&nft_net->module_list));
 	WARN_ON_ONCE(!list_empty(&nft_net->notify_list));
 	WARN_ON_ONCE(!list_empty(&nft_net->destroy_list));
+	WARN_ON_ONCE(!list_empty(&nft_net->set_update_list));
 }
 
 static void nf_tables_exit_batch(struct list_head *net_exit_list)
-- 
2.47.3

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

[Pablo Neira Ayuso]

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;
+
 		list_del_init(&set->pending_update);
 
 		if (!set->ops->commit || set->dead)
@@ -10964,6 +10967,8 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 	}
 
 	/* step 2.  Make rules_gen_X visible to packet path */
+	nft_set_commit_update(nft_net, true);
+
 	list_for_each_entry(table, &nft_net->tables, list) {
 		list_for_each_entry(chain, &table->chains, list)
 			nf_tables_commit_chain(net, chain);
@@ -11170,7 +11175,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 		}
 	}
 
-	nft_set_commit_update(nft_net);
+	nft_set_commit_update(nft_net, false);
 
 	nft_commit_notify(net, NETLINK_CB(skb).portid);
 	nf_tables_gen_notify(net, skb, NFT_MSG_NEWGEN);
diff --git a/net/netfilter/nft_set_rbtree.c b/net/netfilter/nft_set_rbtree.c
index 6222e9bb57bc..d908b94540ef 100644
--- a/net/netfilter/nft_set_rbtree.c
+++ b/net/netfilter/nft_set_rbtree.c
@@ -901,6 +901,8 @@ static void nft_rbtree_gc_scan(struct nft_set *set)
 		next = rb_next(node);
 
 		rbe = rb_entry(node, struct nft_rbtree_elem, node);
+		if (!nft_set_elem_active(&rbe->ext, NFT_GENMASK_ANY))
+			continue;
 
 		/* elements are reversed in the rbtree for historical reasons,
 		 * from highest to lowest value, that is why end element is
@@ -1036,10 +1038,32 @@ static void nft_array_free_rcu(struct rcu_head *rcu_head)
 	__nft_array_free(array);
 }
 
+static struct nft_rbtree_elem *
+__nft_rbtree_prev_active(struct rb_node **pnode, u8 genmask)
+{
+	struct nft_rbtree_elem *prev_rbe;
+	struct rb_node *node = *pnode;
+
+	while (node) {
+		prev_rbe = rb_entry(node, struct nft_rbtree_elem, node);
+		if (!nft_set_elem_active(&prev_rbe->ext, genmask)) {
+			node = rb_prev(node);
+			continue;
+		}
+
+		*pnode = node;
+		return prev_rbe;
+	}
+
+	return NULL;
+}
+
 static void nft_rbtree_commit(struct nft_set *set)
 {
 	struct nft_rbtree *priv = nft_set_priv(set);
 	struct nft_rbtree_elem *rbe, *prev_rbe;
+	struct net *net = read_pnet(&set->net);
+	u8 genmask = nft_genmask_next(net);
 	struct nft_array *old;
 	u32 num_intervals = 0;
 	struct rb_node *node;
@@ -1061,12 +1085,12 @@ static void nft_rbtree_commit(struct nft_set *set)
 
 	/* Reverse walk to create an array from smaller to largest interval. */
 	node = rb_last(&priv->root);
-	if (node)
-		prev_rbe = rb_entry(node, struct nft_rbtree_elem, node);
-	else
-		prev_rbe = NULL;
 
-	while (prev_rbe) {
+	while (node) {
+		prev_rbe = __nft_rbtree_prev_active(&node, genmask);
+		if (!prev_rbe)
+			break;
+
 		rbe = prev_rbe;
 
 		if (nft_rbtree_interval_start(rbe))
@@ -1083,7 +1107,9 @@ static void nft_rbtree_commit(struct nft_set *set)
 		if (!node)
 			break;
 
-		prev_rbe = rb_entry(node, struct nft_rbtree_elem, node);
+		prev_rbe = __nft_rbtree_prev_active(&node, genmask);
+		if (!prev_rbe)
+			break;
 
 		/* For anonymous sets, when adjacent ranges are found,
 		 * the end element is not added to the set to pack the set
-- 
2.47.3

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

[Fernando Fernandez Mancera]

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.

Thanks!
Fernando.

>   		list_del_init(&set->pending_update);
>   
>   		if (!set->ops->commit || set->dead)
> @@ -10964,6 +10967,8 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>   	}
>   
>   	/* step 2.  Make rules_gen_X visible to packet path */
> +	nft_set_commit_update(nft_net, true);
> +
>   	list_for_each_entry(table, &nft_net->tables, list) {
>   		list_for_each_entry(chain, &table->chains, list)
>   			nf_tables_commit_chain(net, chain);
> @@ -11170,7 +11175,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>   		}
>   	}
>   
> -	nft_set_commit_update(nft_net);
> +	nft_set_commit_update(nft_net, false);
>   
>   	nft_commit_notify(net, NETLINK_CB(skb).portid);
>   	nf_tables_gen_notify(net, skb, NFT_MSG_NEWGEN);
> diff --git a/net/netfilter/nft_set_rbtree.c b/net/netfilter/nft_set_rbtree.c
> index 6222e9bb57bc..d908b94540ef 100644
> --- a/net/netfilter/nft_set_rbtree.c
> +++ b/net/netfilter/nft_set_rbtree.c
> @@ -901,6 +901,8 @@ static void nft_rbtree_gc_scan(struct nft_set *set)
>   		next = rb_next(node);
>   
>   		rbe = rb_entry(node, struct nft_rbtree_elem, node);
> +		if (!nft_set_elem_active(&rbe->ext, NFT_GENMASK_ANY))
> +			continue;
>   
>   		/* elements are reversed in the rbtree for historical reasons,
>   		 * from highest to lowest value, that is why end element is
> @@ -1036,10 +1038,32 @@ static void nft_array_free_rcu(struct rcu_head *rcu_head)
>   	__nft_array_free(array);
>   }
>   
> +static struct nft_rbtree_elem *
> +__nft_rbtree_prev_active(struct rb_node **pnode, u8 genmask)
> +{
> +	struct nft_rbtree_elem *prev_rbe;
> +	struct rb_node *node = *pnode;
> +
> +	while (node) {
> +		prev_rbe = rb_entry(node, struct nft_rbtree_elem, node);
> +		if (!nft_set_elem_active(&prev_rbe->ext, genmask)) {
> +			node = rb_prev(node);
> +			continue;
> +		}
> +
> +		*pnode = node;
> +		return prev_rbe;
> +	}
> +
> +	return NULL;
> +}
> +
>   static void nft_rbtree_commit(struct nft_set *set)
>   {
>   	struct nft_rbtree *priv = nft_set_priv(set);
>   	struct nft_rbtree_elem *rbe, *prev_rbe;
> +	struct net *net = read_pnet(&set->net);
> +	u8 genmask = nft_genmask_next(net);
>   	struct nft_array *old;
>   	u32 num_intervals = 0;
>   	struct rb_node *node;
> @@ -1061,12 +1085,12 @@ static void nft_rbtree_commit(struct nft_set *set)
>   
>   	/* Reverse walk to create an array from smaller to largest interval. */
>   	node = rb_last(&priv->root);
> -	if (node)
> -		prev_rbe = rb_entry(node, struct nft_rbtree_elem, node);
> -	else
> -		prev_rbe = NULL;
>   
> -	while (prev_rbe) {
> +	while (node) {
> +		prev_rbe = __nft_rbtree_prev_active(&node, genmask);
> +		if (!prev_rbe)
> +			break;
> +
>   		rbe = prev_rbe;
>   
>   		if (nft_rbtree_interval_start(rbe))
> @@ -1083,7 +1107,9 @@ static void nft_rbtree_commit(struct nft_set *set)
>   		if (!node)
>   			break;
>   
> -		prev_rbe = rb_entry(node, struct nft_rbtree_elem, node);
> +		prev_rbe = __nft_rbtree_prev_active(&node, genmask);
> +		if (!prev_rbe)
> +			break;
>   
>   		/* For anonymous sets, when adjacent ranges are found,
>   		 * the end element is not added to the set to pack the set

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

[Pablo Neira Ayuso]

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.

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

[Fernando Fernandez Mancera]

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 <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.
> 

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!

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> 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.

I'm not following, sorry.  What's the difference between pipapo and rbtree?

static void nft_pipapo_commit(struct nft_set *set)
{
[..]
        if (time_after_eq(jiffies, priv->last_gc + nft_set_gc_interval(set)))
                pipapo_gc_scan(set, priv->clone);

        old = rcu_replace_pointer(priv->match, priv->clone,
                                  nft_pipapo_transaction_mutex_held(set));

[..] after this, new incarnation is live.

static void nft_rbtree_commit(struct nft_set *set)
{
[..]
        if (time_after_eq(jiffies, priv->last_gc + nft_set_gc_interval(set)))
                nft_rbtree_gc_scan(set);

[ ... build the new blob ... ]

err_out:
        priv->array_next->num_intervals = num_intervals;
        old = rcu_replace_pointer(priv->array, priv->array_next,
                                  lockdep_is_held(&nft_pernet(read_pnet(&set->net))->commit_mutex));


[..] after this, new incarnation is live.

What is the problem?  These two functions do the same thing, no?
Why must the blob be rebuilt before stale node purge in rbtree case?

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

[Pablo Neira Ayuso]

On Thu, Aug 06, 2026 at 01:08:30PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> 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.
> 
> I'm not following, sorry.  What's the difference between pipapo and rbtree?

pipapo needs to call nft_setelem_remove() to purge the deactivated
elements from priv->clone, then the new version of the pipapo lookup
table datastructure can be published.

The rbtree does not need to wait to call nft_setelem_remove() since it
updates the rbtree frontend datastructure that is only exposed to
control plane.

> static void nft_pipapo_commit(struct nft_set *set)
> {
> [..]
>         if (time_after_eq(jiffies, priv->last_gc + nft_set_gc_interval(set)))
>                 pipapo_gc_scan(set, priv->clone);
> 
>         old = rcu_replace_pointer(priv->match, priv->clone,
>                                   nft_pipapo_transaction_mutex_held(set));
> 
> [..] after this, new incarnation is live.
> 
> static void nft_rbtree_commit(struct nft_set *set)
> {
> [..]
>         if (time_after_eq(jiffies, priv->last_gc + nft_set_gc_interval(set)))
>                 nft_rbtree_gc_scan(set);
> 
> [ ... build the new blob ... ]
> 
> err_out:
>         priv->array_next->num_intervals = num_intervals;
>         old = rcu_replace_pointer(priv->array, priv->array_next,
>                                   lockdep_is_held(&nft_pernet(read_pnet(&set->net))->commit_mutex));
> 
> 
> [..] after this, new incarnation is live.
> 
> What is the problem? These two functions do the same thing, no?

Apparently yes, but...

> 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/

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

[Florian Westphal]

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.

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.