Re: [PATCH nf 2/3] nft_set_pipapo: fix incorrect avx2 match of 5th field octet

[Stefano Brivio <sbrivio@redhat.com>]

On Fri,  4 Apr 2025 08:20:53 +0200
Florian Westphal <fw@strlen.de> wrote:

> Given a set element like:
> 
> 	icmpv6 . dead:beef:00ff::1
> 
> The value of 'ff' is irrelevant, any address will be matched
> as long as the other octets are the same.
> 
> This is because of too-early register clobbering:
> ymm7 is reloaded with new packet data (pkt[9])  but it still holds data
> of an earlier load that wasn't processed yet.
> 
> The existing tests in nft_concat_range.sh selftests do exercise this code
> path, but do not trigger incorrect matching due to the network prefix
> limitation.
> 
> Cc: Stefano Brivio <sbrivio@redhat.com>
> Reported-by: sontu mazumdar <sontu21@gmail.com>
> Closes: https://marc.info/?l=netfilter&m=174369594208899&w=2
> Fixes: 7400b063969b ("nft_set_pipapo: Introduce AVX2-based lookup implementation")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/netfilter/nft_set_pipapo_avx2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nft_set_pipapo_avx2.c b/net/netfilter/nft_set_pipapo_avx2.c
> index 8ce7154b678a..87cb0183cd79 100644
> --- a/net/netfilter/nft_set_pipapo_avx2.c
> +++ b/net/netfilter/nft_set_pipapo_avx2.c
> @@ -1120,8 +1120,9 @@ static int nft_pipapo_avx2_lookup_8b_16(unsigned long *map, unsigned long *fill,
>  		NFT_PIPAPO_AVX2_BUCKET_LOAD8(5, lt,  8,  pkt[8], bsize);
>  
>  		NFT_PIPAPO_AVX2_AND(6, 2, 3);
> +		NFT_PIPAPO_AVX2_AND(3, 4, 7);
>  		NFT_PIPAPO_AVX2_BUCKET_LOAD8(7, lt,  9,  pkt[9], bsize);
> -		NFT_PIPAPO_AVX2_AND(0, 4, 5);
> +		NFT_PIPAPO_AVX2_AND(0, 3, 5);

Ouch, this is embarrassing, so it's great to see 1/3 and the fact that
it doesn't trigger other splats is a big relief.

Thanks Florian for fixing this and thanks Sontu for the detailed
report. I'm still reviewing patches 1/3 and 3/3.

If it matters, for now, for this one,

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano