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

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

[Eva Kurchatova]

test_xsk.sh makes its veth pair in the namespace it runs in. Whatever
the host does with a new interface, address configuration, neighbour
discovery, happens on that pair, and the AF_XDP sockets under test
receive the host's packets among their own. The tests count and
inspect what they receive, so a preconfigured host fails some of them
in every mode, and a bare one passes.

Run the veth part in a network namespace of its own. The max_skb_frags
sysctl has no file outside the initial namespace, so xskxceiver takes
the limit from a new -f option ahead of the file, and the script hands
it the host's value, or the one given to its own -f. An interface
given with -i is used where it is.

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

Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
---
 tools/testing/selftests/bpf/test_xsk.sh  | 25 ++++++++++++++++++++++--
 tools/testing/selftests/bpf/xskxceiver.c | 13 ++++++++++--
 2 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_xsk.sh b/tools/testing/selftests/bpf/test_xsk.sh
index 65aafe0003db..7859c1d31195 100755
--- 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
+#
 # Display the available command line options
 #   ./test_xsk.sh -h
 
@@ -90,7 +93,7 @@
 
 ETH=""
 
-while getopts "vi:dm:lt:h" flag
+while getopts "vi:dm:lt:f:h" flag
 do
 	case "${flag}" in
 		v) verbose=1;;
@@ -99,6 +102,7 @@ do
 		m) MODE=${OPTARG};;
 		l) list=1;;
 		t) TEST=${OPTARG};;
+		f) FRAGS=${OPTARG};;
 		h) help=1;;
 	esac
 done
@@ -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
+		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}
+		retval=$?
+		# The pair stays up for a run by hand
+		[[ $debug -eq 1 ]] && trap - EXIT && echo "ip netns exec ${XSK_NETNS}"
+		exit $retval
+	fi
+	validate_veth_support ${VETH0}
 	setup_vethPairs
 
 	retval=$?
@@ -187,6 +204,10 @@ if [ -n "$TEST" ]; then
 	ARGS+="-t ${TEST} "
 fi
 
+if [ -n "$FRAGS" ]; then
+	ARGS+="-f ${FRAGS} "
+fi
+
 retval=$?
 test_status $retval "${TEST_NAME}"
 
diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
index 11f047b8af75..c26cc31d21fe 100644
--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c
@@ -113,6 +113,7 @@ static bool opt_verbose;
 static bool opt_print_tests;
 static enum test_mode opt_mode = TEST_MODE_ALL;
 static u32 opt_run_test = RUN_ALL_TESTS;
+static u32 opt_max_skb_frags;
 
 void test__fail(void) { /* for network_helpers.c */ }
 
@@ -351,6 +352,7 @@ static struct option long_options[] = {
 	{"mode", required_argument, 0, 'm'},
 	{"list", no_argument, 0, 'l'},
 	{"test", required_argument, 0, 't'},
+	{"frags", required_argument, 0, 'f'},
 	{"help", no_argument, 0, 'h'},
 	{0, 0, 0, 0}
 };
@@ -366,6 +368,7 @@ static void print_usage(char **argv)
 		"  -m, --mode           Run only mode skb, drv, or zc\n"
 		"  -l, --list           List all available tests\n"
 		"  -t, --test           Run a specific test. Enter number from -l option.\n"
+		"  -f, --frags          Use this max_skb_frags instead of the sysctl\n"
 		"  -h, --help           Display this help and exit\n";
 
 	ksft_print_msg(str, basename(argv[0]));
@@ -389,7 +392,7 @@ static void parse_command_line(struct ifobject *ifobj_tx, struct ifobject *ifobj
 	opterr = 0;
 
 	for (;;) {
-		c = getopt_long(argc, argv, "i:vbm:lt:", long_options, &option_index);
+		c = getopt_long(argc, argv, "i:vbm:lt:f:", long_options, &option_index);
 		if (c == -1)
 			break;
 
@@ -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;
 		case 'h':
 		default:
 			print_usage(argv);
@@ -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;
-- 
2.56.0

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

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

[no-reply]

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