Re: [PATCH v3 net-next 08/15] selftests: xsk: split xskxceiver main() into setup, run and cleanup

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Besides restructuring main(), the patch fixes an existing false-
  pass bug in xskxceiver's exit status, but it carries no Fixes: tag and…

Pre-existing issues:
- [Low] cleanup_iface() detaches an XDP program only when
  `ifobj->xdp_prog` is set, and it picks the detach flags from…

--

selftests: xsk: split xskxceiver main() into setup, run and cleanup

This commit splits xskxceiver main() into three parts. Setup is done by
bind_iface() and detect_mode_caps(), the test loop counts the return
value of run_pkt_test(), and cleanup_iface() gives a single exit path.
It also makes hw_ring_size_reset() skip no-op resets and calls
ksft_print_header() right after the command line is parsed.

> diff --git a/tools/testing/selftests/net/lib/xsk/xskxceiver.c b/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> index 8b3e4d18e982b..02e54b026806b 100644
> --- a/tools/testing/selftests/net/lib/xsk/xskxceiver.c
> +++ b/tools/testing/selftests/net/lib/xsk/xskxceiver.c

[ ... ]

> @@ -317,26 +332,46 @@ static bool is_xdp_supported(int ifindex)

[ ... ]

> +static void cleanup_iface(struct ifobject *ifobj)
> +{
> +	if (ifobj_has_cap(ifobj, XSK_CAP_HW_RING))
> +		hw_ring_size_reset(ifobj);
> +	if (ifobj->xdp_prog)
> +		xsk_detach_xdp_program(ifobj->ifindex,
> +				       ifobj->mode == TEST_MODE_SKB ?
> +				       XDP_FLAGS_SKB_MODE : XDP_FLAGS_DRV_MODE);
> +	xsk_unload_xdp_programs(ifobj);
>  }

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. Can a
native XDP program still be left attached after xskxceiver exits?

cleanup_iface() depends on ifobj->xdp_prog and ifobj->mode. In test_xsk.c,
xsk_reattach_xdp() only updates them at the very end:

tools/testing/selftests/net/lib/xsk/test_xsk.c:xsk_reattach_xdp() {
    ...
	err = xsk_attach_xdp_program(xdp_prog, ifobj->ifindex, mode_to_xdp_flags(mode));
	...
	if (ifobj->mode != mode && (mode == TEST_MODE_DRV || mode == TEST_MODE_ZC))
		if (!xsk_is_in_mode(ifobj->ifindex, XDP_FLAGS_DRV_MODE)) {
			ksft_print_msg("ERROR: XDP prog not in DRV mode\n");
			return -EINVAL;
		}

	ifobj->xdp_prog = xdp_prog;
	ifobj->xskmap = xskmap;
	ifobj->mode = mode;
    ...
}

xsk_is_in_mode() returns false whenever bpf_xdp_query() fails, even if
the native attach just succeeded. If this happens on the last attach of
a run, the new program is attached but never recorded.

cleanup_iface() then either skips the detach because xdp_prog is still
NULL, or detaches with the stale SKB mode flags.

Before this patch, main() did not detach anything at exit, so this patch
makes the window smaller rather than opening it. The code is unchanged at
the end of the series. The runners clean up there: xsk.py detaches the
programs after a failed endpoint, and test_xsk.py uses a fresh veth pair
per case.

Could xsk_reattach_xdp() detach the program, or record it, before it
returns -EINVAL?

> @@ -371,95 +410,62 @@ int main(int argc, char **argv)

[ ... ]

>  			test_init(&test, ifobj_tx, ifobj_rx, i, &tests[j]);
> -			run_pkt_test(&test);
> -			usleep(USLEEP_MAX);
> -
> -			if (test.fail)
> +			if (run_pkt_test(&test) == TEST_FAILURE)
>  				failed_tests++;
> +			usleep(USLEEP_MAX);
>  		}
>  	}
> +	ret = failed_tests ? TEST_FAILURE : TEST_PASS;

[Severity: Low]
This change also fixes an existing false pass in the exit status. Should
it go in a separate patch with a Fixes: tag?

Before this patch, failed_tests only went up when test.fail was set.
Several paths return TEST_FAILURE without setting it. One example is in
__testapp_validate_traffic():

	err = test_spec_set_mtu(test, test->mtu);
	if (err) {
		ksft_print_msg("Error, could not set mtu.\n");
		return TEST_FAILURE;
	}

The same is true for these failures:

- xsk_attach_xdp_progs()
- pthread_barrier_init() and pthread_barrier_destroy() in
  __testapp_validate_traffic()
- set_ring_size() in testapp_validate_traffic()

run_pkt_test() printed a "not ok" line for these, but main() still
called ksft_exit_pass() and exited with 0. In xsk_prereqs.sh,
exec_xskxceiver() looks only at the exit status:

	./${XSKOBJ} -i ${VETH0} -i ${VETH1} ${ARGS}
	retval=$?

As a result, these failures were reported as passes.

The commit message does say "which main() counts instead of test->fail,
as not every failing path sets it". Without a Fixes: tag or a small
standalone patch, though, the fix can't be backported without the whole
refactor.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014184539.774708-1-maciej.fijalkowski%40intel.com