[PATCH nf 0/2] nft_set_pipapo: Fix AVX2 MAC address match, add test

[PATCH nf 0/2] nft_set_pipapo: Fix AVX2 MAC address match, add test

[Stefano Brivio]

Patch 1/2 fixes the issue reported by Nikita where a MAC address
wouldn't match if given as first field of a set, and patch 2/2 adds
the corresponding test.

Stefano Brivio (2):
  nft_set_pipapo: Fix bucket load in AVX2 lookup routine for six 8-bit
    groups
  selftests: netfilter: Add correctness test for mac,net set type

 net/netfilter/nft_set_pipapo_avx2.c           |  2 +-
 .../selftests/netfilter/nft_concat_range.sh   | 24 ++++++++++++++++---
 2 files changed, 22 insertions(+), 4 deletions(-)

-- 
2.30.2

[PATCH nf 1/2] nft_set_pipapo: Fix bucket load in AVX2 lookup routine for six 8-bit groups

[Stefano Brivio]

The sixth byte of packet data has to be looked up in the sixth group,
not in the seventh one, even if we load the bucket data into ymm6
(and not ymm5, for convenience of tracking stalls).

Without this fix, matching on a MAC address as first field of a set,
if 8-bit groups are selected (due to a small set size) would fail,
that is, the given MAC address would never match.

Reported-by: Nikita Yushchenko <nikita.yushchenko@virtuozzo.com>
Cc: <stable@vger.kernel.org> # 5.6.x
Fixes: 7400b063969b ("nft_set_pipapo: Introduce AVX2-based lookup implementation")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 net/netfilter/nft_set_pipapo_avx2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nft_set_pipapo_avx2.c b/net/netfilter/nft_set_pipapo_avx2.c
index e517663e0cd1..6f4116e72958 100644
--- a/net/netfilter/nft_set_pipapo_avx2.c
+++ b/net/netfilter/nft_set_pipapo_avx2.c
@@ -886,7 +886,7 @@ static int nft_pipapo_avx2_lookup_8b_6(unsigned long *map, unsigned long *fill,
 			NFT_PIPAPO_AVX2_BUCKET_LOAD8(4,  lt, 4, pkt[4], bsize);
 
 			NFT_PIPAPO_AVX2_AND(5, 0, 1);
-			NFT_PIPAPO_AVX2_BUCKET_LOAD8(6,  lt, 6, pkt[5], bsize);
+			NFT_PIPAPO_AVX2_BUCKET_LOAD8(6,  lt, 5, pkt[5], bsize);
 			NFT_PIPAPO_AVX2_AND(7, 2, 3);
 
 			/* Stall */
-- 
2.30.2

[PATCH nf 2/2] selftests: netfilter: Add correctness test for mac,net set type

[Stefano Brivio]

The existing net,mac test didn't cover the issue recently reported
by Nikita Yushchenko, where MAC addresses wouldn't match if given
as first field of a concatenated set with AVX2 and 8-bit groups,
because there's a different code path covering the lookup of six
8-bit groups (MAC addresses) if that's the first field.

Add a similar mac,net test, with MAC address and IPv4 address
swapped in the set specification.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 .../selftests/netfilter/nft_concat_range.sh   | 24 ++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/netfilter/nft_concat_range.sh b/tools/testing/selftests/netfilter/nft_concat_range.sh
index 5a4938d6dcf2..ed61f6cab60f 100755
--- a/tools/testing/selftests/netfilter/nft_concat_range.sh
+++ b/tools/testing/selftests/netfilter/nft_concat_range.sh
@@ -23,8 +23,8 @@ TESTS="reported_issues correctness concurrency timeout"
 
 # Set types, defined by TYPE_ variables below
 TYPES="net_port port_net net6_port port_proto net6_port_mac net6_port_mac_proto
-       net_port_net net_mac net_mac_icmp net6_mac_icmp net6_port_net6_port
-       net_port_mac_proto_net"
+       net_port_net net_mac mac_net net_mac_icmp net6_mac_icmp
+       net6_port_net6_port net_port_mac_proto_net"
 
 # Reported bugs, also described by TYPE_ variables below
 BUGS="flush_remove_add"
@@ -277,6 +277,23 @@ perf_entries	1000
 perf_proto	ipv4
 "
 
+TYPE_mac_net="
+display		mac,net
+type_spec	ether_addr . ipv4_addr
+chain_spec	ether saddr . ip saddr
+dst		 
+src		mac addr4
+start		1
+count		5
+src_delta	2000
+tools		sendip nc bash
+proto		udp
+
+race_repeat	0
+
+perf_duration	0
+"
+
 TYPE_net_mac_icmp="
 display		net,mac - ICMP
 type_spec	ipv4_addr . ether_addr
@@ -984,7 +1001,8 @@ format() {
 		fi
 	done
 	for f in ${src}; do
-		__expr="${__expr} . "
+		[ "${__expr}" != "{ " ] && __expr="${__expr} . "
+
 		__start="$(eval format_"${f}" "${srcstart}")"
 		__end="$(eval format_"${f}" "${srcend}")"
 
-- 
2.30.2

Re: [PATCH nf 2/2] selftests: netfilter: Add correctness test for mac,net set type

[Greg KH]

On Sat, Nov 27, 2021 at 11:33:38AM +0100, Stefano Brivio wrote:
> The existing net,mac test didn't cover the issue recently reported
> by Nikita Yushchenko, where MAC addresses wouldn't match if given
> as first field of a concatenated set with AVX2 and 8-bit groups,
> because there's a different code path covering the lookup of six
> 8-bit groups (MAC addresses) if that's the first field.
> 
> Add a similar mac,net test, with MAC address and IPv4 address
> swapped in the set specification.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  .../selftests/netfilter/nft_concat_range.sh   | 24 ++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH nf 2/2] selftests: netfilter: Add correctness test for mac,net set type

[Stefano Brivio]

Hi Greg,

On Sat, 27 Nov 2021 12:22:39 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sat, Nov 27, 2021 at 11:33:38AM +0100, Stefano Brivio wrote:
> > The existing net,mac test didn't cover the issue recently reported
> > by Nikita Yushchenko, where MAC addresses wouldn't match if given
> > as first field of a concatenated set with AVX2 and 8-bit groups,
> > because there's a different code path covering the lookup of six
> > 8-bit groups (MAC addresses) if that's the first field.
> > 
> > Add a similar mac,net test, with MAC address and IPv4 address
> > swapped in the set specification.
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  .../selftests/netfilter/nft_concat_range.sh   | 24 ++++++++++++++++---
> >  1 file changed, 21 insertions(+), 3 deletions(-)  
> 
> <formletter>
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
> 
> </formletter>

This patch (2/2) is not intended for the stable kernel tree, only patch
1/2 is. I Cc'ed stable@kernel.org on the whole series for context.

-- 
Stefano

Re: [PATCH nf 1/2] nft_set_pipapo: Fix bucket load in AVX2 lookup routine for six 8-bit groups

[Nikita Yushchenko]

> The sixth byte of packet data has to be looked up in the sixth group,
> not in the seventh one, even if we load the bucket data into ymm6
> (and not ymm5, for convenience of tracking stalls).
> 
> Without this fix, matching on a MAC address as first field of a set,
> if 8-bit groups are selected (due to a small set size) would fail,
> that is, the given MAC address would never match.
> 
> Reported-by: Nikita Yushchenko <nikita.yushchenko@virtuozzo.com>
> Cc: <stable@vger.kernel.org> # 5.6.x
> Fixes: 7400b063969b ("nft_set_pipapo: Introduce AVX2-based lookup implementation")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Tried it. The issue is indeed fixed.

Tested-By: Nikita Yushchenko <nikita.yushchenko@virtuozzo.com>

Re: [PATCH nf 0/2] nft_set_pipapo: Fix AVX2 MAC address match, add test

[Pablo Neira Ayuso]

On Sat, Nov 27, 2021 at 11:33:36AM +0100, Stefano Brivio wrote:
> Patch 1/2 fixes the issue reported by Nikita where a MAC address
> wouldn't match if given as first field of a set, and patch 2/2 adds
> the corresponding test.

Series applied, thanks