Re: [PATCH nf-next v2 7/8] netfilter: nft_set_pipapo: move cloning of match info to insert/removal path

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Florian,

I have iterated several times over this batch, but I came up with one
late question today, see below.

On Thu, Apr 25, 2024 at 02:06:46PM +0200, Florian Westphal wrote:
> This set type keeps two copies of the sets' content,
>    priv->match (live version, used to match from packet path)
>    priv->clone (work-in-progress version of the 'future' priv->match).
> 
> All additions and removals are done on priv->clone.  When transaction
> completes, priv->clone becomes priv->match and a new clone is allocated
> for use by next transaction.
> 
> Problem is that the cloning requires GFP_KERNEL allocations but we
> cannot fail at either commit or abort time.
> 
> This patch defers the clone until we get an insertion or removal
> request.  This allows us to handle OOM situations correctly.
> 
> This also allows to remove ->dirty in a followup change:
> 
> If ->clone exists, ->dirty is always true
> If ->clone is NULL, ->dirty is always false, no elements were added
> or removed (except catchall elements which are external to the specific
> set backend).
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  net/netfilter/nft_set_pipapo.c | 73 ++++++++++++++++++++++++----------
>  1 file changed, 52 insertions(+), 21 deletions(-)
> 
> diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
> index 9c8da9a0861d..2b1c91e56ca1 100644
> --- a/net/netfilter/nft_set_pipapo.c
> +++ b/net/netfilter/nft_set_pipapo.c
> @@ -615,6 +615,9 @@ nft_pipapo_get(const struct net *net, const struct nft_set *set,
>  	struct nft_pipapo_match *m = priv->clone;
>  	struct nft_pipapo_elem *e;
>  
> +	if (!m)
> +		m = rcu_dereference(priv->match);

nft_pipapo_get() is called from rcu path via _GET netlink command.
Is it safe to walk over priv->clone? Userspace could be updating
(with mutex held) while a request to get an element can be done.

That makes me think nft_pipapo_get() should always use priv->match?

Thanks, and apologies for the late review.