Re: nftables >= 0.9.8: atomic update (nft -f ...) of a set not possible any more

[Stefano Brivio <sbrivio@redhat.com>]

Hi Florian,

On Tue, 4 Jan 2022 20:57:28 +0100
Florian Westphal <fw@strlen.de> wrote:

> etkaar <lists.netfilter.org@prvy.eu> wrote:
> 
> [ CC Stefano ]
> 
> > Dear colleagues,
> > 
> > given is following perfectly working ruleset (nft list ruleset), which drops almost all of the IPv4 traffic, but grants access to port 22 (SSH) for two IPv4 addresses provided by the set named 'whitelist_ipv4_tcp':  
> 
> Thanks for reporting, I can reproduce this.
> 
> > +++
> > table inet filter {
> > 	set whitelist_ipv4_tcp {
> > 		type inet_service . ipv4_addr
> > 		flags interval
> > 		elements = { 22 . 111.222.333.444,
> > 			     22 . 555.666.777.888 }
> > 	}  
> 
> I can repro this, looks like missing scratchpad cloning in the set
> backend.
> 
> I can see that after second 'nft -f', avx2_lookup takes the 'if (unlikely(!scratch)) {' branch.
> 
> Can you try this (kernel) patch below?
> 
> As a workaround, you could try removing the 'interval' flag so that
> kernel uses a hash table as set backend instead.
> 
> Stefano, does that patch make sense to you?
> Thanks!

Thanks for checking and fixing this!

Yes, it makes sense, a clone without a subsequent new insertion
wouldn't have a scratchpad otherwise -- I wonder how I missed this.
Just perhaps:

> diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
> --- a/net/netfilter/nft_set_pipapo.c
> +++ b/net/netfilter/nft_set_pipapo.c
> @@ -1271,7 +1271,7 @@ static struct nft_pipapo_match *pipapo_clone(struct nft_pipapo_match *old)
>  {
>  	struct nft_pipapo_field *dst, *src;
>  	struct nft_pipapo_match *new;
> -	int i;
> +	int i, err;
>  
>  	new = kmalloc(sizeof(*new) + sizeof(*dst) * old->field_count,
>  		      GFP_KERNEL);
> @@ -1291,6 +1291,14 @@ static struct nft_pipapo_match *pipapo_clone(struct nft_pipapo_match *old)
>  		goto out_scratch;
>  #endif
>  
> +	err = pipapo_realloc_scratch(new, old->bsize_max);
> +	if (err) {
> +#ifdef NFT_PIPAPO_ALIGN
> +		free_percpu(new->scratch_aligned);
> +#endif

I would use another label for this, "out_scratch_aligned", for
consistency with the rest of the error handling, but it's not a strong
preference.

> +		goto out_scratch;
> +	}
> +
>  	rcu_head_init(&new->rcu);
>  
>  	src = old->f;
> 

-- 
Stefano