Re: [Devel] [PATCH hci-8.1 v2 1/1] selftests/bpf: run the xsk veth tests in a network namespace of their own

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: test_xsk.sh netns skip path exits 1 via test_exit()
--

commit 8857ec024ef596f27390842fbc1312e800d12531
Author: Eva Kurchatova <eva.kurchatova@virtuozzo.com>

selftests/bpf: run the xsk veth tests in a network namespace of their own

This patch makes test_xsk.sh create a network namespace and re-run itself
inside it, so the veth pair is isolated from the host's traffic. Because
the max_skb_frags sysctl isn't visible outside the initial namespace, it
also adds a -f option to xskxceiver and test_xsk.sh. The commit message
says the tests are skipped when network namespaces are unavailable.

> diff --git a/tools/testing/selftests/bpf/test_xsk.sh b/tools/testing/selftests/bpf/test_xsk.sh
> --- a/tools/testing/selftests/bpf/test_xsk.sh
> +++ b/tools/testing/selftests/bpf/test_xsk.sh

[ ... ]

> @@ -162,8 +167,27 @@ if [ ! -z $ETH ]; then
>  	VETH1=${ETH}
>  else
>  	validate_root_exec
> -	validate_veth_support ${VETH0}
>  	validate_ip_utility
> +	# The host configures and talks on any new interface in its namespace
> +	if [ -z "$XSK_NETNS" ]; then
> +		XSK_NETNS=xsk${VETH0_POSTFIX}
> +		if ! ip netns add ${XSK_NETNS}; then
> +			echo "skip all tests: cannot create a network namespace" >&2
> +			test_exit $ksft_skip
> +		fi

[Severity: Medium]
The commit message says "Without network namespaces the tests are
skipped", but can this path actually report a skip?

test_exit() is defined in xsk_prereqs.sh as:

    test_exit()
    {
        if [ $1 -ne 0 ]; then
            test_status $1 $(basename $0)
        fi
        exit 1
    }

It prints "[ SKIPPED ]" but then does an unconditional "exit 1" and
ignores its argument. When "ip netns add" fails (no netns support, no
permission, read-only /run/netns, name collision), test_xsk.sh will
therefore exit with status 1, not $ksft_skip (4). The kselftest runner
will then count that as a failure.

Before this patch, a host without netns support would still run the tests
in its current namespace. Now such a host reports a failure.

The test_exit() behaviour wasn't introduced by this patch. It already
affects the existing veth-missing and ip-missing skip paths. This patch
adds a new call site that depends on it, though, and that site is more
likely to be hit.

Should test_exit() be changed to "exit $1", or should this path exit with
$ksft_skip directly, so the skip is reported as the commit message
describes?

> +		trap "ip netns del ${XSK_NETNS}" EXIT

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261022024816.2435565-1-eva.kurchatova@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel