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

[Stefano Brivio <sbrivio@redhat.com>]

On Thu,  8 Jun 2023 03:57:00 +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.
> 
> This patch adds a new ->pending_update field to sets to maintain a list
> of sets that require this new commit and abort operations.

As I mentioned on v1, maybe we can actually make activate and
deactivate calls optional (in the API) and drop them from
nft_set_pipapo, but this is something I can investigate (and test)
separately.

Some nits (code comments only) that went probably lost from my comments
on v1:

> Fixes: 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> v3: fix sparse issue reported by robot.
> 
>  include/net/netfilter/nf_tables.h |  4 ++-
>  net/netfilter/nf_tables_api.c     | 56 +++++++++++++++++++++++++++++++
>  net/netfilter/nft_set_pipapo.c    | 55 +++++++++++++++++++++---------
>  3 files changed, 99 insertions(+), 16 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 2e24ea1d744c..83db182decc8 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,
> @@ -557,6 +558,7 @@ struct nft_set {
>  	u16				policy;
>  	u16				udlen;
>  	unsigned char			*udata;
> +	struct list_head		pending_update;
>  	/* runtime data below here */
>  	const struct nft_set_ops	*ops ____cacheline_aligned;
>  	u16				flags:14,
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 0519d45ede6b..3bb0800b3849 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -4919,6 +4919,7 @@ static int nf_tables_newset(struct sk_buff *skb, const struct nfnl_info *info,
>  
>  	set->num_exprs = num_exprs;
>  	set->handle = nf_tables_alloc_handle(table);
> +	INIT_LIST_HEAD(&set->pending_update);
>  
>  	err = nft_trans_set_add(&ctx, NFT_MSG_NEWSET, set);
>  	if (err < 0)
> @@ -9275,10 +9276,25 @@ 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)
> +{
> +	struct nft_set *set, *next;
> +
> +	list_for_each_entry_safe(set, next, set_update_list, pending_update) {
> +		list_del_init(&set->pending_update);
> +
> +		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);
>  	struct nft_trans *trans, *next;
> +	LIST_HEAD(set_update_list);
>  	struct nft_trans_elem *te;
>  	struct nft_chain *chain;
>  	struct nft_table *table;
> @@ -9453,6 +9469,11 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>  			nf_tables_setelem_notify(&trans->ctx, te->set,
>  						 &te->elem,
>  						 NFT_MSG_NEWSETELEM);
> +			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:
> @@ -9467,6 +9488,11 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>  				atomic_dec(&te->set->nelems);
>  				te->set->ndeact--;
>  			}
> +			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)) {
> @@ -9529,6 +9555,8 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
>  		}
>  	}
>  
> +	nft_set_commit_update(&set_update_list);
> +
>  	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);
> @@ -9588,10 +9616,25 @@ static void nf_tables_abort_release(struct nft_trans *trans)
>  	kfree(trans);
>  }
>  
> +static void nft_set_abort_update(struct list_head *set_update_list)
> +{
> +	struct nft_set *set, *next;
> +
> +	list_for_each_entry_safe(set, next, set_update_list, pending_update) {
> +		list_del_init(&set->pending_update);
> +
> +		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);
>  	struct nft_trans *trans, *next;
> +	LIST_HEAD(set_update_list);
>  	struct nft_trans_elem *te;
>  
>  	if (action == NFNL_ABORT_VALIDATE &&
> @@ -9701,6 +9744,12 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
>  			nft_setelem_remove(net, te->set, &te->elem);
>  			if (!nft_setelem_is_catchall(te->set, &te->elem))
>  				atomic_dec(&te->set->nelems);
> +
> +			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:
> @@ -9711,6 +9760,11 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
>  			if (!nft_setelem_is_catchall(te->set, &te->elem))
>  				te->set->ndeact--;
>  
> +			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:
> @@ -9753,6 +9807,8 @@ static int __nf_tables_abort(struct net *net, enum nfnl_abort_action action)
>  		}
>  	}
>  
> +	nft_set_abort_update(&set_update_list);
> +
>  	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..15e451dc3fc4 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)

Before this:

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

>  {
> -	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,26 @@ 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
 */

The rest looks good to me, thanks.

-- 
Stefano