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@xxxxxxxxx>
> ---
> 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
>
>
