[PATCH nf] netfilter: nf_tables: integrate pipapo into commit protocol

[PATCH nf] netfilter: nf_tables: integrate pipapo into commit protocol

[Pablo Neira Ayuso]

The pipapo set backend follows copy-on-update approach, maintaining one
clone of the existing datastructure that is being updated. The clone
and current datastructures are swapped via rcu from the commit step.

The existing integration with the commit protocol is flawed because
there is no operation to clean up the clone if the transaction is
aborted. Moreover, the datastructure swap happens on set element
activation.

This patch adds two new operations for sets: commit and abort, these new
operations are invoked from the commit and abort steps, after the
transactions have been digested, and it updates the pipapo set backend
to use it.

Fixes: 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
Hi Stefano,

I don't see any path to reset ->dirty in case that the transaction is aborted.
This might lead to this sequence:

1) add set element: ->insert adds new entry in the clone (preparation phase)
2) any command that fails (preparation phase)
3) abort phase (->dirty bit is left set on and priv->clone contains an partial update)

I am trying to figure out if this could be related to:
https://bugzilla.netfilter.org/show_bug.cgi?id=1583

 include/net/netfilter/nf_tables.h |  3 +-
 net/netfilter/nf_tables_api.c     | 36 +++++++++++++++++++++
 net/netfilter/nft_set_pipapo.c    | 53 ++++++++++++++++++++++---------
 3 files changed, 76 insertions(+), 16 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 552e19ba4f43..211921dd0ac6 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -462,7 +462,8 @@ struct nft_set_ops {
 					       const struct nft_set *set,
 					       const struct nft_set_elem *elem,
 					       unsigned int flags);
-
+	void				(*commit)(const struct nft_set *set);
+	void				(*abort)(const struct nft_set *set);
 	u64				(*privsize)(const struct nlattr * const nla[],
 						    const struct nft_set_desc *desc);
 	bool				(*estimate)(const struct nft_set_desc *desc,
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 0e072b2365df..ef8d9f6a7e9c 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -9259,6 +9259,22 @@ static void nf_tables_commit_audit_log(struct list_head *adl, u32 generation)
 	}
 }
 
+static void nft_set_commit_update(struct net *net)
+{
+	struct nftables_pernet *nft_net = nft_pernet(net);
+	struct nft_table *table;
+	struct nft_set *set;
+
+	list_for_each_entry(table, &nft_net->tables, list) {
+		list_for_each_entry(set, &table->sets, list) {
+			if (!set->ops->commit)
+				continue;
+
+			set->ops->commit(set);
+		}
+	}
+}
+
 static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 {
 	struct nftables_pernet *nft_net = nft_pernet(net);
@@ -9513,6 +9529,8 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 		}
 	}
 
+	nft_set_commit_update(net);
+
 	nft_commit_notify(net, NETLINK_CB(skb).portid);
 	nf_tables_gen_notify(net, skb, NFT_MSG_NEWGEN);
 	nf_tables_commit_audit_log(&adl, nft_net->base_seq);
@@ -9572,6 +9590,22 @@ static void nf_tables_abort_release(struct nft_trans *trans)
 	kfree(trans);
 }
 
+static void nft_set_abort_update(struct net *net)
+{
+	struct nftables_pernet *nft_net = nft_pernet(net);
+	struct nft_table *table;
+	struct nft_set *set;
+
+	list_for_each_entry(table, &nft_net->tables, list) {
+		list_for_each_entry(set, &table->sets, list) {
+			if (!set->ops->abort)
+				continue;
+
+			set->ops->abort(set);
+		}
+	}
+}
+
 static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
 {
 	struct nftables_pernet *nft_net = nft_pernet(net);
@@ -9737,6 +9771,8 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
 		}
 	}
 
+	nft_set_abort_update(net);
+
 	synchronize_rcu();
 
 	list_for_each_entry_safe_reverse(trans, next,
diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
index 06d46d182634..06b8b26e666a 100644
--- a/net/netfilter/nft_set_pipapo.c
+++ b/net/netfilter/nft_set_pipapo.c
@@ -1600,17 +1600,10 @@ static void pipapo_free_fields(struct nft_pipapo_match *m)
 	}
 }
 
-/**
- * pipapo_reclaim_match - RCU callback to free fields from old matching data
- * @rcu:	RCU head
- */
-static void pipapo_reclaim_match(struct rcu_head *rcu)
+static void pipapo_free_match(struct nft_pipapo_match *m)
 {
-	struct nft_pipapo_match *m;
 	int i;
 
-	m = container_of(rcu, struct nft_pipapo_match, rcu);
-
 	for_each_possible_cpu(i)
 		kfree(*per_cpu_ptr(m->scratch, i));
 
@@ -1625,7 +1618,19 @@ static void pipapo_reclaim_match(struct rcu_head *rcu)
 }
 
 /**
- * pipapo_commit() - Replace lookup data with current working copy
+ * pipapo_reclaim_match - RCU callback to free fields from old matching data
+ * @rcu:	RCU head
+ */
+static void pipapo_reclaim_match(struct rcu_head *rcu)
+{
+	struct nft_pipapo_match *m;
+
+	m = container_of(rcu, struct nft_pipapo_match, rcu);
+	pipapo_free_match(m);
+}
+
+/**
+ * nft_pipapo_commit() - Replace lookup data with current working copy
  * @set:	nftables API set representation
  *
  * While at it, check if we should perform garbage collection on the working
@@ -1635,7 +1640,7 @@ static void pipapo_reclaim_match(struct rcu_head *rcu)
  * We also need to create a new working copy for subsequent insertions and
  * deletions.
  */
-static void pipapo_commit(const struct nft_set *set)
+static void nft_pipapo_commit(const struct nft_set *set)
 {
 	struct nft_pipapo *priv = nft_set_priv(set);
 	struct nft_pipapo_match *new_clone, *old;
@@ -1660,6 +1665,24 @@ static void pipapo_commit(const struct nft_set *set)
 	priv->clone = new_clone;
 }
 
+static void nft_pipapo_abort(const struct nft_set *set)
+{
+	struct nft_pipapo *priv = nft_set_priv(set);
+	struct nft_pipapo_match *new_clone;
+
+	if (!priv->dirty)
+		return;
+
+	new_clone = pipapo_clone(priv->match);
+	if (IS_ERR(new_clone))
+		return;
+
+	priv->dirty = false;
+
+	pipapo_free_match(priv->clone);
+	priv->clone = new_clone;
+}
+
 /**
  * nft_pipapo_activate() - Mark element reference as active given key, commit
  * @net:	Network namespace
@@ -1667,8 +1690,7 @@ static void pipapo_commit(const struct nft_set *set)
  * @elem:	nftables API element representation containing key data
  *
  * On insertion, elements are added to a copy of the matching data currently
- * in use for lookups, and not directly inserted into current lookup data, so
- * we'll take care of that by calling pipapo_commit() here. Both
+ * in use for lookups, and not directly inserted into current lookup data. Both
  * nft_pipapo_insert() and nft_pipapo_activate() are called once for each
  * element, hence we can't purpose either one as a real commit operation.
  */
@@ -1684,8 +1706,6 @@ static void nft_pipapo_activate(const struct net *net,
 
 	nft_set_elem_change_active(net, set, &e->ext);
 	nft_set_elem_clear_busy(&e->ext);
-
-	pipapo_commit(set);
 }
 
 /**
@@ -1931,7 +1951,6 @@ static void nft_pipapo_remove(const struct net *net, const struct nft_set *set,
 		if (i == m->field_count) {
 			priv->dirty = true;
 			pipapo_drop(m, rulemap);
-			pipapo_commit(set);
 			return;
 		}
 
@@ -2230,6 +2249,8 @@ const struct nft_set_type nft_set_pipapo_type = {
 		.init		= nft_pipapo_init,
 		.destroy	= nft_pipapo_destroy,
 		.gc_init	= nft_pipapo_gc_init,
+		.commit		= nft_pipapo_commit,
+		.abort		= nft_pipapo_abort,
 		.elemsize	= offsetof(struct nft_pipapo_elem, ext),
 	},
 };
@@ -2252,6 +2273,8 @@ const struct nft_set_type nft_set_pipapo_avx2_type = {
 		.init		= nft_pipapo_init,
 		.destroy	= nft_pipapo_destroy,
 		.gc_init	= nft_pipapo_gc_init,
+		.commit		= nft_pipapo_commit,
+		.abort		= nft_pipapo_abort,
 		.elemsize	= offsetof(struct nft_pipapo_elem, ext),
 	},
 };
-- 
2.30.2

Re: [PATCH nf] netfilter: nf_tables: integrate pipapo into commit protocol

[Stefano Brivio]

Sorry for the delay.

On Mon, 24 Apr 2023 15:33:20 +0200
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> The pipapo set backend follows copy-on-update approach, maintaining one
> clone of the existing datastructure that is being updated. The clone
> and current datastructures are swapped via rcu from the commit step.
> 
> The existing integration with the commit protocol is flawed because
> there is no operation to clean up the clone if the transaction is
> aborted. Moreover, the datastructure swap happens on set element
> activation.
> 
> This patch adds two new operations for sets: commit and abort, these new
> operations are invoked from the commit and abort steps, after the
> transactions have been digested, and it updates the pipapo set backend
> to use it.
> 
> Fixes: 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> Hi Stefano,
> 
> I don't see any path to reset ->dirty in case that the transaction is aborted.
> This might lead to this sequence:
> 
> 1) add set element: ->insert adds new entry in the clone (preparation phase)
> 2) any command that fails (preparation phase)
> 3) abort phase (->dirty bit is left set on and priv->clone contains an partial update)

That's because nft_pipapo_activate() can't fail on elements that are
successfully inserted by nft_pipapo_insert() -- that is, the
pipapo_get() call from nft_pipapo_activate() should always succeed.

Now, let's say that one call to nft_pipapo_insert() fails: at that
point I would expect all the pending insertions to be deleted before
the abort phase completes. So yes, we have a dirty bit unnecessarily
set, but the clone shouldn't actually contain a partial update.

Regardless of that, having an actual abort operation is cleaner than
this and definitely welcome.

> I am trying to figure out if this could be related to:
> https://bugzilla.netfilter.org/show_bug.cgi?id=1583

For sure this simplifies the matter, but I'm not sure exactly in which
way it could be related (I'm not saying that there's no way, though).

>  include/net/netfilter/nf_tables.h |  3 +-
>  net/netfilter/nf_tables_api.c     | 36 +++++++++++++++++++++
>  net/netfilter/nft_set_pipapo.c    | 53 ++++++++++++++++++++++---------
>  3 files changed, 76 insertions(+), 16 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 552e19ba4f43..211921dd0ac6 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -462,7 +462,8 @@ struct nft_set_ops {
>  					       const struct nft_set *set,
>  					       const struct nft_set_elem *elem,
>  					       unsigned int flags);
> -
> +	void				(*commit)(const struct nft_set *set);
> +	void				(*abort)(const struct nft_set *set);
>  	u64				(*privsize)(const struct nlattr * const nla[],
>  						    const struct nft_set_desc *desc);
>  	bool				(*estimate)(const struct nft_set_desc *desc,
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 0e072b2365df..ef8d9f6a7e9c 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -9259,6 +9259,22 @@ static void nf_tables_commit_audit_log(struct list_head *adl, u32 generation)
>  	}
>  }
>  
> +static void nft_set_commit_update(struct net *net)
> +{
> +	struct nftables_pernet *nft_net = nft_pernet(net);
> +	struct nft_table *table;
> +	struct nft_set *set;
> +
> +	list_for_each_entry(table, &nft_net->tables, list) {
> +		list_for_each_entry(set, &table->sets, list) {
> +			if (!set->ops->commit)
> +				continue;
> +
> +			set->ops->commit(set);
> +		}
> +	}
> +}
> +
>  static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>  {
>  	struct nftables_pernet *nft_net = nft_pernet(net);
> @@ -9513,6 +9529,8 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>  		}
>  	}
>  
> +	nft_set_commit_update(net);
> +
>  	nft_commit_notify(net, NETLINK_CB(skb).portid);
>  	nf_tables_gen_notify(net, skb, NFT_MSG_NEWGEN);
>  	nf_tables_commit_audit_log(&adl, nft_net->base_seq);
> @@ -9572,6 +9590,22 @@ static void nf_tables_abort_release(struct nft_trans *trans)
>  	kfree(trans);
>  }
>  
> +static void nft_set_abort_update(struct net *net)
> +{
> +	struct nftables_pernet *nft_net = nft_pernet(net);
> +	struct nft_table *table;
> +	struct nft_set *set;
> +
> +	list_for_each_entry(table, &nft_net->tables, list) {
> +		list_for_each_entry(set, &table->sets, list) {
> +			if (!set->ops->abort)
> +				continue;
> +
> +			set->ops->abort(set);
> +		}
> +	}
> +}
> +
>  static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
>  {
>  	struct nftables_pernet *nft_net = nft_pernet(net);
> @@ -9737,6 +9771,8 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
>  		}
>  	}
>  
> +	nft_set_abort_update(net);
> +
>  	synchronize_rcu();
>  
>  	list_for_each_entry_safe_reverse(trans, next,

I didn't imagine it would be so simple -- I would have gone with this
right away. On the other hand, I had no idea where to put those calls :)

Now that we have commit and abort operations, I'm wondering: what if we
make activate and deactivate callbacks optional? They're not really
needed by nft_set_pipapo: all the elements could be directly "active"
on insertion, and remain active until we have a commit operation after
they are deleted.

I'm not sure if there are drawbacks, and I think that your patch works
anyway as expected.

> diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
> index 06d46d182634..06b8b26e666a 100644
> --- a/net/netfilter/nft_set_pipapo.c
> +++ b/net/netfilter/nft_set_pipapo.c
> @@ -1600,17 +1600,10 @@ static void pipapo_free_fields(struct nft_pipapo_match *m)
>  	}
>  }
>  
> -/**
> - * pipapo_reclaim_match - RCU callback to free fields from old matching data
> - * @rcu:	RCU head
> - */
> -static void pipapo_reclaim_match(struct rcu_head *rcu)

/**
 * pipapo_free_match() - Free fields from unused matching data
 * @m:		Matching data
 */

> +static void pipapo_free_match(struct nft_pipapo_match *m)
>  {
> -	struct nft_pipapo_match *m;
>  	int i;
>  
> -	m = container_of(rcu, struct nft_pipapo_match, rcu);
> -
>  	for_each_possible_cpu(i)
>  		kfree(*per_cpu_ptr(m->scratch, i));
>  
> @@ -1625,7 +1618,19 @@ static void pipapo_reclaim_match(struct rcu_head *rcu)
>  }
>  
>  /**
> - * pipapo_commit() - Replace lookup data with current working copy
> + * pipapo_reclaim_match - RCU callback to free fields from old matching data

 * pipapo_reclaim_match() - RCU callback to free unused matching data

...it's not necessarily "old" anymore.

> + * @rcu:	RCU head
> + */
> +static void pipapo_reclaim_match(struct rcu_head *rcu)
> +{
> +	struct nft_pipapo_match *m;
> +
> +	m = container_of(rcu, struct nft_pipapo_match, rcu);
> +	pipapo_free_match(m);
> +}
> +
> +/**
> + * nft_pipapo_commit() - Replace lookup data with current working copy
>   * @set:	nftables API set representation
>   *
>   * While at it, check if we should perform garbage collection on the working
> @@ -1635,7 +1640,7 @@ static void pipapo_reclaim_match(struct rcu_head *rcu)
>   * We also need to create a new working copy for subsequent insertions and
>   * deletions.
>   */
> -static void pipapo_commit(const struct nft_set *set)
> +static void nft_pipapo_commit(const struct nft_set *set)
>  {
>  	struct nft_pipapo *priv = nft_set_priv(set);
>  	struct nft_pipapo_match *new_clone, *old;
> @@ -1660,6 +1665,24 @@ static void pipapo_commit(const struct nft_set *set)
>  	priv->clone = new_clone;
>  }
>  
> +static void nft_pipapo_abort(const struct nft_set *set)

/**
 * nft_pipapo_abort() - Drop uncommitted matching data if any, reset dirty flag
 * @set:	nftables API set representation
 */

> +{
> +	struct nft_pipapo *priv = nft_set_priv(set);
> +	struct nft_pipapo_match *new_clone;
> +
> +	if (!priv->dirty)
> +		return;
> +
> +	new_clone = pipapo_clone(priv->match);
> +	if (IS_ERR(new_clone))
> +		return;
> +
> +	priv->dirty = false;
> +
> +	pipapo_free_match(priv->clone);
> +	priv->clone = new_clone;

This is essentially a non-RCU version of (the new) nft_pipapo_commit(),
minus the garbage collection stuff, but I think a tiny bit of
duplication here as you already implemented it is clearer than trying
to factor this out into some helper.

> +}
> +
>  /**
>   * nft_pipapo_activate() - Mark element reference as active given key, commit
>   * @net:	Network namespace
> @@ -1667,8 +1690,7 @@ static void pipapo_commit(const struct nft_set *set)
>   * @elem:	nftables API element representation containing key data
>   *
>   * On insertion, elements are added to a copy of the matching data currently
> - * in use for lookups, and not directly inserted into current lookup data, so
> - * we'll take care of that by calling pipapo_commit() here. Both
> + * in use for lookups, and not directly inserted into current lookup data. Both
>   * nft_pipapo_insert() and nft_pipapo_activate() are called once for each
>   * element, hence we can't purpose either one as a real commit operation.
>   */
> @@ -1684,8 +1706,6 @@ static void nft_pipapo_activate(const struct net *net,
>  
>  	nft_set_elem_change_active(net, set, &e->ext);
>  	nft_set_elem_clear_busy(&e->ext);
> -
> -	pipapo_commit(set);
>  }
>  
>  /**
> @@ -1931,7 +1951,6 @@ static void nft_pipapo_remove(const struct net *net, const struct nft_set *set,
>  		if (i == m->field_count) {
>  			priv->dirty = true;
>  			pipapo_drop(m, rulemap);
> -			pipapo_commit(set);
>  			return;
>  		}
>  
> @@ -2230,6 +2249,8 @@ const struct nft_set_type nft_set_pipapo_type = {
>  		.init		= nft_pipapo_init,
>  		.destroy	= nft_pipapo_destroy,
>  		.gc_init	= nft_pipapo_gc_init,
> +		.commit		= nft_pipapo_commit,
> +		.abort		= nft_pipapo_abort,
>  		.elemsize	= offsetof(struct nft_pipapo_elem, ext),
>  	},
>  };
> @@ -2252,6 +2273,8 @@ const struct nft_set_type nft_set_pipapo_avx2_type = {
>  		.init		= nft_pipapo_init,
>  		.destroy	= nft_pipapo_destroy,
>  		.gc_init	= nft_pipapo_gc_init,
> +		.commit		= nft_pipapo_commit,
> +		.abort		= nft_pipapo_abort,
>  		.elemsize	= offsetof(struct nft_pipapo_elem, ext),
>  	},
>  };

Everything else looks good to me, thanks!

-- 
Stefano