Re: [PATCH nf-next 2/4] netfilter: nft_set_pipapo: do not rely on ZERO_SIZE_PTR

[Stefano Brivio <sbrivio@xxxxxxxxxx>]

On Mon, 12 Feb 2024 11:01:51 +0100
Florian Westphal <fw@xxxxxxxxx> wrote:

> pipapo relies on kmalloc(0) returning ZERO_SIZE_PTR (i.e., not NULL
> but pointer is invalid).
> 
> Rework this to not call slab allocator when we'd request a 0-byte
> allocation.
> 
> While at it, also use GFP_KERNEL allocations here, this is only called
> from control plane.
> 
> Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
> ---
>  net/netfilter/nft_set_pipapo.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
> index 395420fa71e5..6a79ec98de86 100644
> --- a/net/netfilter/nft_set_pipapo.c
> +++ b/net/netfilter/nft_set_pipapo.c
> @@ -526,13 +526,16 @@ static struct nft_pipapo_elem *pipapo_get(const struct net *net,
>  	const struct nft_pipapo_field *f;
>  	int i;
>  
> -	res_map = kmalloc_array(m->bsize_max, sizeof(*res_map), GFP_ATOMIC);
> +	if (m->bsize_max == 0)
> +		return ret;
> +
> +	res_map = kmalloc_array(m->bsize_max, sizeof(*res_map), GFP_KERNEL);
>  	if (!res_map) {
>  		ret = ERR_PTR(-ENOMEM);
>  		goto out;
>  	}
>  
> -	fill_map = kcalloc(m->bsize_max, sizeof(*res_map), GFP_ATOMIC);
> +	fill_map = kcalloc(m->bsize_max, sizeof(*res_map), GFP_KERNEL);

I haven't re-checked the whole logic, but can't nft_pipapo_deactivate()
(hence pipapo_deactivate() and pipapo_get()) be called from the data
path for some reason?

If I recall correctly that's why I used GFP_ATOMIC here, but I'm not
sure anymore and I guess you know better.

>  	if (!fill_map) {
>  		ret = ERR_PTR(-ENOMEM);
>  		goto out;
> @@ -1367,11 +1370,16 @@ static struct nft_pipapo_match *pipapo_clone(struct nft_pipapo_match *old)
>  		       src->bsize * sizeof(*dst->lt) *
>  		       src->groups * NFT_PIPAPO_BUCKETS(src->bb));
>  
> -		dst->mt = kvmalloc(src->rules * sizeof(*src->mt), GFP_KERNEL);
> -		if (!dst->mt)
> -			goto out_mt;
> +		if (src->rules > 0) {
> +			dst->mt = kvmalloc_array(src->rules, sizeof(*src->mt), GFP_KERNEL);

Nit: equally readable within 80 columns:

			dst->mt = kvmalloc_array(src->rules, sizeof(*src->mt),
						 GFP_KERNEL);

> +			if (!dst->mt)
> +				goto out_mt;
> +
> +			memcpy(dst->mt, src->mt, src->rules * sizeof(*src->mt));
> +		} else {
> +			dst->mt = NULL;
> +		}
>  
> -		memcpy(dst->mt, src->mt, src->rules * sizeof(*src->mt));
>  		src++;
>  		dst++;
>  	}

-- 
Stefano