| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 24 Apr 2023 10:10:54 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: yang.yang29@....com.cn, davem@...emloft.net,
willemdebruijn.kernel@...il.com
Cc: edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
shuah@...nel.org, netdev@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
zhang.yunkai@....com.cn, yang.yang29@....com.cn,
xu.xin16@....com.cn
Subject: RE: [PATCH linux-next v3] selftests: net: udpgso_bench_rx: Fix
verifty exceptions
yang.yang29@ wrote:
> From: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@....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@....com.cn>
> Reviewed-by: Xu Xin (CGEL ZTE) <xu.xin16@....com.cn>
> Reviewed-by: Yang Yang (CGEL ZTE) <yang.yang29@....com.cn>
> Cc: Xuexin Jiang (CGEL ZTE) <jiang.xuexin@....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
Powered by blists - more mailing lists