Re: [Devel] [PATCH hci-8.1 v3 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 3 potential issue(s) to consider:
- [Low] selftests/bpf: unrelated test_exit() fix and internal tracker tags bundled in
- [Low] selftests/bpf: test_xsk.sh skip probe misses the --mount and sysfs steps it later needs
- [Low] selftests/bpf: xskxceiver sysctl fallback for max_skb_frags is dead in the new netns
--

commit e15fddae4e72be15308b41d2c75c44585ec9c80f
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 run the veth part of the xsk tests inside a
private network namespace. It also reads the fragment limit from the
kernel's BTF, with the sysctl as a fallback. It also changes test_exit() to
exit with its argument instead of always exiting 1.

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

[Severity: Low]
This isn't a bug, but the commit message contains an internal tracker URL
and a trailer that mean nothing upstream:

> https://virtuozzo.atlassian.net/browse/VSTOR-150731
> Feature: fix selftests

Could these two lines be dropped?

The test_exit() change below is also a standalone fix, described in the
commit message as "Fix test_exit() return status for skips, as since commit
895b62eed2ab ("selftests: xsk: fix reporting of failed tests"), test_exit()
exited 1."

Would it be better as a separate patch with a Fixes: tag, so it can be
backported and bisected on its own?

> 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 +163,33 @@ 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
> +		if ! unshare --net true; then
> +			echo "skip all tests: cannot create a network namespace" >&2
> +			test_exit $ksft_skip
> +		fi
> +		if [[ $debug -ne 1 ]]; then
> +			# Unnamed, it goes with its last process; its own sysfs shows its interfaces
> +			XSK_NETNS=1 exec unshare --net --mount sh -c \
> +				'mount --make-rprivate / && mount -t sysfs sysfs /sys &&
> +				exec bash "$0" "$@"' "$0" "$@"
> +		fi

[Severity: Low]
Can this turn a skip into a failure? The probe only runs unshare --net
true. The --mount, mount --make-rprivate / and mount -t sysfs sysfs /sys
steps are not probed.

If the namespace probe succeeds but one of those mount steps is denied (for
example by an LSM, a seccomp filter, or a user namespace setup where sysfs
cannot be mounted), the sh -c command exits non-zero.

The exec has already replaced the script by then, so test_exit $ksft_skip
can no longer run. The caller sees a failure instead of a skip.

The commit message says "Other cases after a successful namespace creation
fail."

Should the probe cover the mount steps as well, for example by running the
same unshare --net --mount sh -c 'mount ...' sequence with true in place of
the final exec bash?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c

[ ... ]

>  #define MAX_SKB_FRAGS_PATH "/proc/sys/net/core/max_skb_frags"
>  static unsigned int get_max_skb_frags(void)
>  {
> -	unsigned int max_skb_frags = 0;
> +	unsigned int max_skb_frags = btf_max_skb_frags();
>  	FILE *file;
>  
> +	if (max_skb_frags)
> +		return max_skb_frags;
> +
> +	ksft_print_msg("No MAX_SKB_FRAGS in the kernel's BTF, reading %s\n", MAX_SKB_FRAGS_PATH);
>  	file = fopen(MAX_SKB_FRAGS_PATH, "r");
>  	if (!file) {
>  		ksft_print_msg("Error opening %s\n", MAX_SKB_FRAGS_PATH);

[Severity: Low]
Is the sysctl fallback reachable for the veth runs? test_xsk.sh now always
re-execs into a private network namespace for the veth tests.

In net/core/sysctl_net_core.c, max_skb_frags is in net_core_table, which is
registered only for init_net:

    register_net_sysctl(&init_net, "net/core", net_core_table);

Non-init namespaces get only netns_core_table. So inside the new namespace
the fopen() above always fails, get_max_skb_frags() returns 0, and the
caller falls back to its hardcoded value:

    max_frags = get_max_skb_frags();
    if (!max_frags) {
        ksft_print_msg("Couldn't retrieve MAX_SKB_FRAGS from system, using default (17) value\n");
        max_frags = 17;
    }

On a kernel without vmlinux BTF and with CONFIG_MAX_SKB_FRAGS other than 17,
can the veth multi-buffer tests then fail spuriously?

The fallback still works for -i physical-device runs and for xskxceiver run
by hand in the init namespace. The commit message says "fall back to sysctl
as before if this info is missing", which does not hold for the veth flow.

> diff --git a/tools/testing/selftests/bpf/xsk_prereqs.sh b/tools/testing/selftests/bpf/xsk_prereqs.sh
> --- a/tools/testing/selftests/bpf/xsk_prereqs.sh
> +++ b/tools/testing/selftests/bpf/xsk_prereqs.sh
> @@ -50,7 +50,7 @@ test_exit()
>  	if [ $1 -ne 0 ]; then
>  		test_status $1 $(basename $0)
>  	fi
> -	exit 1
> +	exit $1
>  }

This is the test_exit() change mentioned above.

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

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