netdev - Re: [PATCH net 5/5] selftests: net: add very basic test for netdev names and namespaces

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <73c1dc5b-4add-0db9-0073-7ae8fed0c747@intel.com>
Date: Tue, 17 Oct 2023 13:25:10 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<davem@...emloft.net>
Subject: Re: [PATCH net 5/5] selftests: net: add very basic test for netdev
 names and namespaces

On 10/16/23 22:16, Jakub Kicinski wrote:
> Add selftest for fixes around naming netdevs and namespaces.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>   tools/testing/selftests/net/Makefile      |  1 +
>   tools/testing/selftests/net/netns-name.sh | 91 +++++++++++++++++++++++
>   2 files changed, 92 insertions(+)
>   create mode 100755 tools/testing/selftests/net/netns-name.sh
> 
> diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> index 8b017070960d..4a2881d43989 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -34,6 +34,7 @@ TEST_PROGS += gro.sh
>   TEST_PROGS += gre_gso.sh
>   TEST_PROGS += cmsg_so_mark.sh
>   TEST_PROGS += cmsg_time.sh cmsg_ipv6.sh
> +TEST_PROGS += netns-name.sh
>   TEST_PROGS += srv6_end_dt46_l3vpn_test.sh
>   TEST_PROGS += srv6_end_dt4_l3vpn_test.sh
>   TEST_PROGS += srv6_end_dt6_l3vpn_test.sh
> diff --git a/tools/testing/selftests/net/netns-name.sh b/tools/testing/selftests/net/netns-name.sh
> new file mode 100755
> index 000000000000..59e4a498aab4
> --- /dev/null
> +++ b/tools/testing/selftests/net/netns-name.sh
> @@ -0,0 +1,91 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +set -o pipefail
> +
> +NS=netns-name-test
> +DEV=dummy-dev0
> +DEV2=dummy-dev1
> +ALT_NAME=some-alt-name
> +
> +RET_CODE=0
> +
> +cleanup() {
> +    ip netns del $NS
> +}
> +
> +trap cleanup EXIT
> +
> +fail() {
> +    if [ ! -z "$1" ]; then

s/! -z/-n/

> +	echo "ERROR: $1"
> +    else
> +	echo "ERROR: unexpected return code"

I see that in some cases unexpected rc is 0, but it's worth printing.

At the expense of requiring reader to know about default values syntax,
whole if could become:
   echo "ERROR: ${1-unexpected return code} (rc=$_)"

I didn't do my homework (of checking expectations of selftests
"framework"), but perhaps errors/diag messages should go to stderr? >&2

> +    fi
> +    RET_CODE=1
> +}
> +
> +ip netns add $NS
> +
> +#
> +# Test basic move without a rename
> +#
> +ip -netns $NS link add name $DEV type dummy || fail
> +ip -netns $NS link set dev $DEV netns 1 || \

\ after ||, |, && is redundant, not sure if it improves readability or not

> +    fail "Can't perform a netns move"
> +ip link show dev $DEV >> /dev/null || fail "Device not found after move"
> +ip link del $DEV || fail
> +
> +#
> +# Test move with a conflict
> +#
> +ip link add name $DEV type dummy
> +ip -netns $NS link add name $DEV type dummy || fail
> +ip -netns $NS link set dev $DEV netns 1 2> /dev/null && \
> +    fail "Performed a netns move with a name conflict"
> +ip link show dev $DEV >> /dev/null || fail "Device not found after move"
> +ip -netns $NS link del $DEV || fail
> +ip link del $DEV || fail
> +
> +#
> +# Test move with a conflict and rename
> +#
> +ip link add name $DEV type dummy
> +ip -netns $NS link add name $DEV type dummy || fail
> +ip -netns $NS link set dev $DEV netns 1 name $DEV2 || \
> +    fail "Can't perform a netns move with rename"
> +ip link del $DEV2 || fail
> +ip link del $DEV || fail
> +
> +#
> +# Test dup alt-name with netns move
> +#
> +ip link add name $DEV type dummy || fail
> +ip link property add dev $DEV altname $ALT_NAME || fail
> +ip -netns $NS link add name $DEV2 type dummy || fail
> +ip -netns $NS link property add dev $DEV2 altname $ALT_NAME || fail
> +
> +ip -netns $NS link set dev $DEV2 netns 1 2> /dev/null && \
> +    fail "Moved with alt-name dup"
> +
> +ip link del $DEV || fail
> +ip -netns $NS link del $DEV2 || fail
> +
> +#
> +# Test creating alt-name in one net-ns and using in another
> +#
> +ip -netns $NS link add name $DEV type dummy || fail
> +ip -netns $NS link property add dev $DEV altname $ALT_NAME || fail
> +ip -netns $NS link set dev $DEV netns 1 || fail
> +ip link show dev $ALT_NAME >> /dev/null || fail "Can't find alt-name after move"
> +ip  -netns $NS link show dev $ALT_NAME 2> /dev/null && \
> +    fail "Can still find alt-name after move"
> +ip link del $DEV || fail
> +
> +echo -ne "$(basename $0) \t\t\t\t"
> +if [ $RET_CODE -eq 0 ]; then
> +    echo "[  OK  ]"
> +else
> +    echo "[ FAIL ]"
> +fi
> +exit $RET_CODE

I like this patch (and the rest of the series), especially for the fact
how easy it is to test (compared to internals of HW drivers ;) )