Re: [PATCH v3 net-next 09/15] selftests: xsk: run one test case per xskxceiver invocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests: sysctl leak of vm.nr_hugepages on exception
- [Low] selftests: unused argument in ksft_print_msg() format string
--

commit 7acec71c768966e15c22ec5e2e55704dfaf4978e
Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

selftests: xsk: run one test case per xskxceiver invocation

This commit replaces test_xsk.sh and xsk_prereqs.sh with test_xsk.py to run
xskxceiver once per mode and case. This ensures that every test case runs in a
fresh process and crashes only fail the specific test.

> diff --git a/tools/testing/selftests/net/lib/xsk/xskxceiver.c b/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> index 02e54b026806b..09ad623333be5 100644
> --- a/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> +++ b/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> @@ -169,14 +164,14 @@ static struct option long_options[] = {
>  static void print_usage(char **argv)
>  {
>  	const char *str =
> -		"  Usage: xskxceiver [OPTIONS]\n"
> +		"  Usage: xskxceiver -i TX_IFACE -i RX_IFACE -m MODE -t TEST [OPTIONS]\n"
>  		"  Options:\n"
>  		"  -i, --interface      Use interface\n"
>  		"  -v, --verbose        Verbose output\n"
>  		"  -b, --busy-poll      Enable busy poll\n"
> -		"  -m, --mode           Run only mode skb, drv, or zc\n"
> +		"  -m, --mode           Mode to run: skb, drv or zc\n"
>  		"  -l, --list           List all available tests\n"
> -		"  -t, --test           Run a specific test. Enter number from -l option.\n"
> +		"  -t, --test           Test case to run, a number from -l\n"
>  		"  -h, --help           Display this help and exit\n";
>  
>  	ksft_print_msg(str, basename(argv[0]));

[Severity: Low]
Does this code silently ignore the executable name argument? The format string
in 'str' does not contain a '%s' specifier, so basename(argv[0]) is not printed
when print_usage() is called.

[ ... ]

> diff --git a/tools/testing/selftests/net/test_xsk.py b/tools/testing/selftests/net/test_xsk.py
> new file mode 100755
> index 0000000000000..acfdba55fbae8
> --- /dev/null
> +++ b/tools/testing/selftests/net/test_xsk.py
> @@ -0,0 +1,140 @@

[ ... ]

> +def main():
> +    # ksft_run() exits on -l and -h before main() restores the huge pages.
> +    if "-l" in sys.argv or "-h" in sys.argv:
> +        sys.exit(f"{sys.argv[0]}: -l and -h are not supported")
> +    if not XSK_BIN.exists():
> +        print(f"1..0 # SKIP {XSK_BIN} was not built")
> +        sys.exit(SKIP_CODE)
> +    saved_hugepages = []
> +    hugepages = xsk_reserve_hugepages(saved_hugepages)
> +    cases = _get_cases(hugepages)
> +    ksft_run([ksft_variants(cases)(test_xsk),
> +              ksft_variants(cases)(test_xsk_busy_poll)])
> +    xsk_release_hugepages(saved_hugepages)
> +    ksft_exit()

[Severity: Medium]
Can this leak the host's vm.nr_hugepages sysctl modifications if an exception
occurs?

If _get_cases() or ksft_run() raise an unhandled exception (such as a
CmdExitFailure if the executable crashes, or a KeyboardInterrupt), execution
will terminate before reaching xsk_release_hugepages(). 

Should these calls be wrapped in a try/finally block to ensure hugepages are
properly restored?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014184539.774708-1-maciej.fijalkowski@intel.com?part=9