Re: [Devel] [PATCH hci-8.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 4 potential issue(s) to consider:
- [Low] selftests/bpf: test_xsk.sh passes a bare -f when max_skb_frags is unreadable
- [Low] selftests/bpf: xskxceiver -f parsing can wrap u32 and the calloc() size
- [Low] selftests/bpf: test_xsk.sh -f only changes the test's assumed max_skb_frags
- [Low] selftests/bpf: test_xsk.sh reports FAIL when the netns cannot be created
--

commit 035a16fff590cf5d4d3fde38f45f552e4cab308c
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 its veth pair inside a private network
namespace, so host traffic does not reach the AF_XDP sockets under test. It
adds a -f option to both test_xsk.sh and xskxceiver so the max_skb_frags
value can be passed in from outside the namespace.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-150731

> 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

[ ... ]

> @@ -83,6 +83,9 @@
>  # Run a specific test from the test suite
>  #   sudo ./test_xsk.sh -t TEST_NAME
>  #
> +# Run with a max_skb_frags limit other than the host's
> +#   sudo ./test_xsk.sh -f FRAGS
> +#

[Severity: Low]
This isn't a regression in the mechanics, but is this description accurate?

Nothing writes net.core.max_skb_frags. The -f value is only forwarded to
xskxceiver, where testapp_too_many_frags() uses it to size the packet
stream, while the kernel keeps enforcing its real limit.

If FRAGS differs from the host's sysctl, can the too-many-frags test build
the wrong fragment count and fail or give misleading results? Should the
comment and the commit message wording say that -f only changes the value
the test assumes?

[ ... ]

> @@ -162,8 +166,21 @@ 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}
> +		ip netns add ${XSK_NETNS} || test_exit $ksft_fail

[Severity: Low]
When "ip netns add" fails (no CONFIG_NET_NS, a container without
/run/netns, or a collision on the random suffix), this now reports a
failure through test_exit $ksft_fail. Before the patch the test simply ran
in the current namespace.

The script uses ksft_skip for other missing prerequisites. Should this case
skip rather than fail?

> +		trap "ip netns del ${XSK_NETNS}" EXIT
> +		# The max_skb_frags sysctl has no file outside the initial namespace
> +		[ -n "$FRAGS" ] || FRAGS=$(cat /proc/sys/net/core/max_skb_frags)
> +		XSK_NETNS=${XSK_NETNS} ip netns exec ${XSK_NETNS} bash "$0" "$@" -f ${FRAGS}

[Severity: Low]
If /proc/sys/net/core/max_skb_frags is missing or unreadable (for example
on a kernel without the sysctl), the failed cat is not checked and FRAGS
stays empty.

The unquoted ${FRAGS} then expands to nothing, so the child is started
with a trailing "-f" and no argument. The child's getopts "vi:dm:lt:f:h"
prints "option requires an argument -- f". The case statement has no "?"
arm, so the run continues with FRAGS empty and xskxceiver falls back to 17.

Can the "-f ${FRAGS}" be appended only when FRAGS is non-empty?

> +		retval=$?
> +		# The pair stays up for a run by hand
> +		[[ $debug -eq 1 ]] && trap - EXIT && echo "ip netns exec ${XSK_NETNS}"

In the -d case, the hint printed is only "ip netns exec ${XSK_NETNS}",
which is not a complete command. The inner script has already exited and
the namespace and veth pair are left behind. Could the hint include a
command to run, or the namespace name in a more useful form?

> +		exit $retval
> +	fi
> +	validate_veth_support ${VETH0}
>  	setup_vethPairs

[ ... ]

> 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

[ ... ]

> @@ -437,6 +440,12 @@ static void parse_command_line(struct ifobject *ifobj_tx, struct ifobject *ifobj
>  			if (errno)
>  				print_usage(argv);
>  			break;
> +		case 'f':
> +			errno = 0;
> +			opt_max_skb_frags = strtoul(optarg, NULL, 0);
> +			if (errno || !opt_max_skb_frags)
> +				print_usage(argv);
> +			break;

[Severity: Low]
Can this wrap opt_max_skb_frags? strtoul() returns an unsigned long that
is stored in a u32, and only errno and zero are checked. There is no upper
bound and no endptr check, so input such as "17abc" is accepted too.

> @@ -2271,7 +2280,7 @@ static int testapp_too_many_frags(struct test_spec *test)
>  	if (test->mode == TEST_MODE_ZC) {
>  		max_frags = test->ifobj_tx->xdp_zc_max_segs;
>  	} else {
> -		max_frags = get_max_skb_frags();
> +		max_frags = opt_max_skb_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;
>  		}
>  		max_frags += 1;

Can this overflow the pkts[] allocation in testapp_too_many_frags()? With
-f 4294967295, max_frags += 1 wraps to 0. With a value around 0x80000000,
the size in calloc(2 * max_frags + 2, sizeof(struct pkt)) wraps to a tiny
value.

The later loops that fill pkts[i] up to 2 * max_frags + 1 would then write
far beyond the allocation. Before this patch the value came only from the
range-limited sysctl. test_xsk.sh -f forwards the value verbatim to
xskxceiver, so both entry points can reach this. Should -f be validated
with an upper bound and an endptr check?

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

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