[PATCH linux-next v3] selftests: net: udpgso_bench_rx: Fix verifty exceptions

[PATCH linux-next v3] selftests: net: udpgso_bench_rx: Fix verifty exceptions

[yang.yang29]

From: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>

The verification function of this test case is likely to encounter the
following error, which may confuse users. The problem is easily
reproducible in the latest kernel.

Environment A, the sender:
bash# udpgso_bench_tx -l 4 -4 -D "$IP_B"
udpgso_bench_tx: write: Connection refused

Environment B, the receiver:
bash# udpgso_bench_rx -4 -G -S 1472 -v
udpgso_bench_rx: data[1472]: len 17664, a(97) != q(113)

If the packet is captured, you will see:
Environment A, the sender:
bash# tcpdump -i eth0 host "$IP_B" &
IP $IP_A.41025 > $IP_B.8000: UDP, length 1472
IP $IP_A.41025 > $IP_B.8000: UDP, length 1472
IP $IP_B > $IP_A: ICMP $IP_B udp port 8000 unreachable, length 556

Environment B, the receiver:
bash# tcpdump -i eth0 host "$IP_B" &
IP $IP_A.41025 > $IP_B.8000: UDP, length 7360
IP $IP_A.41025 > $IP_B.8000: UDP, length 14720
IP $IP_B > $IP_A: ICMP $IP_B udp port 8000 unreachable, length 556

In one test, the verification data is printed as follows:
abcd...xyz           | 1...
..                  |
abcd...xyz           |
abcd...opabcd...xyz  | ...1472... Not xyzabcd, messages are merged
..                  |

The issue is that the test on receive for expected payload pattern 
{AB..Z}+ fail for GRO packets if segment payload does not end on a Z.

The issue still exists when using the GRO with -G, but not using the -S
to obtain gsosize. Therefore, a print has been added to remind users.

Changes in v3:
- Simplify description and adjust judgment order.

Changes in v2:
- Fix confusing descriptions.

Signed-off-by: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>
Reviewed-by: Xu Xin (CGEL ZTE) <xu.xin16@zte.com.cn>
Reviewed-by: Yang Yang (CGEL ZTE) <yang.yang29@zte.com.cn>
Cc: Xuexin Jiang (CGEL ZTE) <jiang.xuexin@zte.com.cn>
---
 tools/testing/selftests/net/udpgso_bench_rx.c | 34 +++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
index f35a924d4a30..3ad18cbc570d 100644
--- a/tools/testing/selftests/net/udpgso_bench_rx.c
+++ b/tools/testing/selftests/net/udpgso_bench_rx.c
@@ -189,26 +189,45 @@ static char sanitized_char(char val)
 	return (val >= 'a' && val <= 'z') ? val : '.';
 }

-static void do_verify_udp(const char *data, int len)
+static void do_verify_udp(const char *data, int start, int len)
 {
-	char cur = data[0];
+	char cur = data[start];
 	int i;

 	/* verify contents */
 	if (cur < 'a' || cur > 'z')
 		error(1, 0, "data initial byte out of range");

-	for (i = 1; i < len; i++) {
+	for (i = start + 1; i < start + len; i++) {
 		if (cur == 'z')
 			cur = 'a';
 		else
 			cur++;

-		if (data[i] != cur)
+		if (data[i] != cur) {
+			if (cfg_gro_segment && !cfg_expected_gso_size)
+				error(0, 0, "Use -S to obtain gsosize to guide "
+					"splitting and verification.");
+
 			error(1, 0, "data[%d]: len %d, %c(%hhu) != %c(%hhu)\n",
 			      i, len,
 			      sanitized_char(data[i]), data[i],
 			      sanitized_char(cur), cur);
+		}
+	}
+}
+
+static void do_verify_udp_gro(const char *data, int len, int segment_size)
+{
+	int start = 0;
+
+	while (len - start > 0) {
+		if (len - start > segment_size)
+			do_verify_udp(data, start, segment_size);
+		else
+			do_verify_udp(data, start, len - start);
+
+		start += segment_size;
 	}
 }

@@ -268,7 +287,12 @@ static void do_flush_udp(int fd)
 			if (ret == 0)
 				error(1, errno, "recv: 0 byte datagram\n");

-			do_verify_udp(rbuf, ret);
+			if (!cfg_gro_segment)
+				do_verify_udp(rbuf, 0, ret);
+			else if (gso_size > 0)
+				do_verify_udp_gro(rbuf, ret, gso_size);
+			else
+				do_verify_udp_gro(rbuf, ret, ret);
 		}
 		if (cfg_expected_gso_size && cfg_expected_gso_size != gso_size)
 			error(1, 0, "recv: bad gso size, got %d, expected %d "
-- 
2.15.2

RE: [PATCH linux-next v3] selftests: net: udpgso_bench_rx: Fix verifty exceptions

[Willem de Bruijn]

yang.yang29@ wrote:
> From: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>
> 
> The verification function of this test case is likely to encounter the
> following error, which may confuse users. The problem is easily
> reproducible in the latest kernel.
> 
> Environment A, the sender:
> bash# udpgso_bench_tx -l 4 -4 -D "$IP_B"
> udpgso_bench_tx: write: Connection refused

This error is not relevant to the bug that is being fixed
 
> Environment B, the receiver:
> bash# udpgso_bench_rx -4 -G -S 1472 -v
> udpgso_bench_rx: data[1472]: len 17664, a(97) != q(113)
> 
> If the packet is captured, you will see:
> Environment A, the sender:
> bash# tcpdump -i eth0 host "$IP_B" &
> IP $IP_A.41025 > $IP_B.8000: UDP, length 1472
> IP $IP_A.41025 > $IP_B.8000: UDP, length 1472
> IP $IP_B > $IP_A: ICMP $IP_B udp port 8000 unreachable, length 556

Same here

> Environment B, the receiver:
> bash# tcpdump -i eth0 host "$IP_B" &
> IP $IP_A.41025 > $IP_B.8000: UDP, length 7360
> IP $IP_A.41025 > $IP_B.8000: UDP, length 14720
> IP $IP_B > $IP_A: ICMP $IP_B udp port 8000 unreachable, length 556

And here
 
> In one test, the verification data is printed as follows:
> abcd...xyz           | 1...
> ..                  |
> abcd...xyz           |
> abcd...opabcd...xyz  | ...1472... Not xyzabcd, messages are merged
> ..                  |
> 
> The issue is that the test on receive for expected payload pattern 
> {AB..Z}+ fail for GRO packets if segment payload does not end on a Z.

This is really the only pertinent explanation needed for the fix.

> The issue still exists when using the GRO with -G, but not using the -S
> to obtain gsosize. Therefore, a print has been added to remind users.

So the issue is that -G/cfg_gro_segment enables UDP_GRO, but
-S/cfg_expected_gso_size enables recvmsg cmsg UDP_GRO. We need
gso_size to know whether discontinuities will appear, so cannot
verify payload for -G without -S. There really is no reason to
ever run the test in that configuration, should perhaps fail.

Btw title should start with PATCH net as this is a fix, instead of
PATCH linux-next. And it is verify not verifty. Also needs a Fixes tag:

Fixes: 0a9ac2e954091 ("selftests: add GRO support to udp bench rx program")

> Changes in v3:
> - Simplify description and adjust judgment order.
> 
> Changes in v2:
> - Fix confusing descriptions.
> 
> Signed-off-by: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>
> Reviewed-by: Xu Xin (CGEL ZTE) <xu.xin16@zte.com.cn>
> Reviewed-by: Yang Yang (CGEL ZTE) <yang.yang29@zte.com.cn>
> Cc: Xuexin Jiang (CGEL ZTE) <jiang.xuexin@zte.com.cn>
> ---
>  tools/testing/selftests/net/udpgso_bench_rx.c | 34 +++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
> index f35a924d4a30..3ad18cbc570d 100644
> --- a/tools/testing/selftests/net/udpgso_bench_rx.c
> +++ b/tools/testing/selftests/net/udpgso_bench_rx.c
> @@ -189,26 +189,45 @@ static char sanitized_char(char val)
>  	return (val >= 'a' && val <= 'z') ? val : '.';
>  }
> 
> -static void do_verify_udp(const char *data, int len)
> +static void do_verify_udp(const char *data, int start, int len)
>  {
> -	char cur = data[0];
> +	char cur = data[start];
>  	int i;
> 
>  	/* verify contents */
>  	if (cur < 'a' || cur > 'z')
>  		error(1, 0, "data initial byte out of range");
> 
> -	for (i = 1; i < len; i++) {
> +	for (i = start + 1; i < start + len; i++) {
>  		if (cur == 'z')
>  			cur = 'a';
>  		else
>  			cur++;
> 
> -		if (data[i] != cur)
> +		if (data[i] != cur) {
> +			if (cfg_gro_segment && !cfg_expected_gso_size)
> +				error(0, 0, "Use -S to obtain gsosize to guide "
> +					"splitting and verification.");
> +

This is not the place to add a gso_size test. Drop.

>  			error(1, 0, "data[%d]: len %d, %c(%hhu) != %c(%hhu)\n",
>  			      i, len,
>  			      sanitized_char(data[i]), data[i],
>  			      sanitized_char(cur), cur);
> +		}
> +	}
> +}
> +
> +static void do_verify_udp_gro(const char *data, int len, int segment_size)
> +{
> +	int start = 0;
> +
> +	while (len - start > 0) {
> +		if (len - start > segment_size)
> +			do_verify_udp(data, start, segment_size);
> +		else
> +			do_verify_udp(data, start, len - start);

Instead of adding start argument, just pass data + start as first argument.

> +
> +		start += segment_size;
>  	}
>  }
> 
> @@ -268,7 +287,12 @@ static void do_flush_udp(int fd)
>  			if (ret == 0)
>  				error(1, errno, "recv: 0 byte datagram\n");
> 
> -			do_verify_udp(rbuf, ret);
> +			if (!cfg_gro_segment)
> +				do_verify_udp(rbuf, 0, ret);
> +			else if (gso_size > 0)
> +				do_verify_udp_gro(rbuf, ret, gso_size);
> +			else
> +				do_verify_udp_gro(rbuf, ret, ret);

This only test a fraction of the payload. The test should always test
the entire payload. It should just be aware of discontinuity at gso_size.
>  		}
>  		if (cfg_expected_gso_size && cfg_expected_gso_size != gso_size)
>  			error(1, 0, "recv: bad gso size, got %d, expected %d "
> -- 
> 2.15.2