Re: [PATCH nf-next 1/3] netfilter: nf_set_pipapo_avx2: fix initial map fill

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

Hi Florian, Stefano,

On Fri, May 23, 2025 at 02:20:44PM +0200, Florian Westphal wrote:
> If the first field doesn't cover the entire start map, then we must zero
> out the remainder, else we leak those bits into the next match round map.
> 
> The earlie fix was incomplete and did only fix up the generic C
> implementation.
> 
> A followup patch adds a test case to nft_concat_range.sh.
> 
> Fixes: 791a615b7ad2 ("netfilter: nf_set_pipapo: fix initial map fill")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/netfilter/nft_set_pipapo_avx2.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nft_set_pipapo_avx2.c b/net/netfilter/nft_set_pipapo_avx2.c
> index c15db28c5ebc..be7c16c79f71 100644
> --- a/net/netfilter/nft_set_pipapo_avx2.c
> +++ b/net/netfilter/nft_set_pipapo_avx2.c
> @@ -1113,6 +1113,25 @@ bool nft_pipapo_avx2_estimate(const struct nft_set_desc *desc, u32 features,
>  	return true;
>  }
>  
> +/**
> + * pipapo_resmap_init_avx2() - Initialise result map before first use
> + * @m:		Matching data, including mapping table
> + * @res_map:	Result map
> + *
> + * Like pipapo_resmap_init() but do not set start map bits covered by the first field.
> + */
> +static inline void pipapo_resmap_init_avx2(const struct nft_pipapo_match *m, unsigned long *res_map)
> +{
> +	const struct nft_pipapo_field *f = m->f;
> +	int i;
> +
> +	/* Starting map doesn't need to be set to all-ones for this implementation,
> +	 * but we do need to zero the remaining bits, if any.
> +	 */
> +	for (i = f->bsize; i < m->bsize_max; i++)
> +		res_map[i] = 0ul;
> +}
> +
>  /**
>   * nft_pipapo_avx2_lookup() - Lookup function for AVX2 implementation
>   * @net:	Network namespace
> @@ -1171,7 +1190,7 @@ bool nft_pipapo_avx2_lookup(const struct net *net, const struct nft_set *set,
>  	res  = scratch->map + (map_index ? m->bsize_max : 0);
>  	fill = scratch->map + (map_index ? 0 : m->bsize_max);
>  
> -	/* Starting map doesn't need to be set for this implementation */
> +	pipapo_resmap_init_avx2(m, res);

nitpick:

nft_pipapo_avx2_lookup_slow() calls pipapo_resmap_init() for
non-optimized fields, eg. 8 bytes, which is unlikely to be seen.
IIUC this resets it again.

Maybe revisit this in nf-next? Would be worth to cover this avx2 path
with 8 bytes in tests?

Thanks.

>  
>  	nft_pipapo_avx2_prepare();
>  
> -- 
> 2.49.0
> 
>