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

[Stefano Brivio <sbrivio@redhat.com>]

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