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

["Eva Kurchatova (Virtuozzo)" <eva.kurchatova@virtuozzo.com>]

On 10/26/26 03:29, no-reply@sashiko.virtuozzo.com wrote:
> 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?

Um, this is a vzkernel patch

> 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?
I have decided that such a small change should be bundled here, if the 
v2 is not accepted to begin with. This patch will be sent in similar 
form, albeit altered due to upstream having many changes in this kselftest.
>> 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?
The failure here is intentional, it is either a test bug or a very 
misconfigured host.
> [ ... ]
>
>> 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?

VZkernel only builds with BTF in production. The test never could run in 
a separate namespace before this patch; This is the whole point. It also 
follows the established practice in bpf selftests.

> 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.
>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel