netdev - Re: [PATCH net-next] selftests: udpgso: Pull up network setup into shell script

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87zfwlv0sx.fsf@cloudflare.com>
Date: Wed, 31 Jan 2024 18:47:16 +0100
From: Jakub Sitnicki <jakub@...udflare.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, Willem de Bruijn <willemb@...gle.com>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 kernel-team@...udflare.com
Subject: Re: [PATCH net-next] selftests: udpgso: Pull up network setup into
 shell script

On Tue, Jan 30, 2024 at 03:26 PM -05, Willem de Bruijn wrote:
> Jakub Sitnicki wrote:
>> udpgso regression test configures routing and device MTU directly through
>> uAPI (Netlink, ioctl) to do its job. While there is nothing wrong with it,
>> it takes more effort than doing it from shell.
>> 
>> Looking forward, we would like to extend the udpgso regression tests to
>> cover the EIO corner case [1], once it gets addressed. That will require a
>> dummy device and device feature manipulation to set it up. Which means more
>> Netlink code.
>> 
>> So, in preparation, pull out network configuration into the shell script
>> part of the test, so it is easily extendable in the future.
>> 
>> Also, because it now easy to setup routing, add a second local IPv6
>> address. Because the second address is not managed by the kernel, we can
>> "replace" the corresponding local route with a reduced-MTU one. This
>> unblocks the disabled "ipv6 connected" test case. Add a similar setup for
>> IPv4 for symmetry.
>
> Nice!
>
> Just a few small nits.
>

Thanks for quick feedback. Will apply it and respin.

>> 
>> [1] https://lore.kernel.org/netdev/87jzqsld6q.fsf@cloudflare.com/
>> 
>> Signed-off-by: Jakub Sitnicki <jakub@...udflare.com>
>> ---
>>  tools/testing/selftests/net/udpgso.c  | 134 ++------------------------
>>  tools/testing/selftests/net/udpgso.sh |  50 ++++++++--
>>  2 files changed, 48 insertions(+), 136 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/net/udpgso.c b/tools/testing/selftests/net/udpgso.c
>> index 7badaf215de2..79fd3287ff60 100644
>> --- a/tools/testing/selftests/net/udpgso.c
>> +++ b/tools/testing/selftests/net/udpgso.c
>> @@ -56,7 +56,6 @@ static bool		cfg_do_msgmore;
>>  static bool		cfg_do_setsockopt;
>>  static int		cfg_specific_test_id = -1;
>>  
>> -static const char	cfg_ifname[] = "lo";
>>  static unsigned short	cfg_port = 9000;
>>  
>>  static char buf[ETH_MAX_MTU];
>> @@ -69,8 +68,13 @@ struct testcase {
>>  	int r_len_last;		/* recv(): size of last non-mss dgram, if any */
>>  };
>>  
>> -const struct in6_addr addr6 = IN6ADDR_LOOPBACK_INIT;
>> -const struct in_addr addr4 = { .s_addr = __constant_htonl(INADDR_LOOPBACK + 2) };
>> +const struct in6_addr addr6 = {
>> +	{ { 0x20, 0x01, 0x0d, 0xb8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0x00, 0x01 } },
>> +};
>> +
>> +const struct in_addr addr4 = {
>> +	__constant_htonl(0xc0000201), /* 192.0.2.1 */
>> +};
>
> Prefer an address from a private range?

No preference. I saw both in use across net selftests and went with one.

[...]

>> diff --git a/tools/testing/selftests/net/udpgso.sh b/tools/testing/selftests/net/udpgso.sh
>> index fec24f584fe9..d7fb71e132bb 100755
>> --- a/tools/testing/selftests/net/udpgso.sh
>> +++ b/tools/testing/selftests/net/udpgso.sh
>> @@ -3,27 +3,57 @@
>>  #
>>  # Run a series of udpgso regression tests
>>  
>> +set -o errexit
>> +set -o nounset
>> +# set -o xtrace
>
> Leftover debug comment?
>

Left it on purpose for the next person debugging it. But can remove it.

>> +
>> +setup_loopback() {
>> +  ip addr add dev lo 192.0.2.1/32
>> +  ip addr add dev lo 2001:db8::1/128 nodad noprefixroute
>> +}
>> +
>> +test_dev_mtu() {
>> +  setup_loopback
>> +  # Reduce loopback MTU
>> +  ip link set dev lo mtu 1500
>> +}
>> +
>> +test_route_mtu() {
>> +  setup_loopback
>> +  # Remove default local routes
>> +  ip route del local 192.0.2.1/32 table local dev lo
>> +  ip route del local 2001:db8::1/128 table local dev lo
>> +  # Install local routes with reduced MTU
>> +  ip route add local 192.0.2.1/32 table local dev lo mtu 1500
>> +  ip route add local 2001:db8::1/128 table local dev lo mtu 1500
>
> ip route change?
>

I've tried it. Doesn't work as expected. Only del+add seems do to
it. You end up with two routes if you use `ip route replace`:

  # ip route replace local 2001:db8::1/128 table local dev lo mtu 1500
  # ip -6 route show table local
  local ::1 dev lo proto kernel metric 0 pref medium
  local 2001:db8::1 dev lo proto kernel metric 0 pref medium
  local 2001:db8::1 dev lo metric 1024 mtu 1500 pref medium
  #

I didn't dig into why, though.

[...]