Re: [PATCH net-next 2/5] selftests/xsk: roll back partial socket setup on configure failures

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Eager rollback in xsk_configure() causes a guaranteed NULL pointer dereference in the parallel TX thread when the RX thread fails.
--

--- Patch [2]: [PATCH net-next 2/5] selftests/xsk: roll back partial socket setup on configure failures ---
commit 80baccb88ac5eb975506a8efbd37137e2c6775b1
Author: Tushar Vyavahare <[email protected]>

selftests/xsk: roll back partial socket setup on configure failures

This commit introduces an eager rollback mechanism to clean up already
configured sockets if subsequent setups or busy-poll configurations fail
during xsk_configure().

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> --- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
[ ... ]
> @@ -1500,14 +1522,18 @@ static int xsk_configure(struct test_spec *test, struct ifobject *ifobject,
>  				break;
>  
>  			/* Retry if it fails as xsk_socket__create() is asynchronous */
> -			if (ctr >= SOCK_RECONF_CTR)
> +			if (ctr >= SOCK_RECONF_CTR) {
> +				xsk_configure_rollback(ifobject, i);
>  				return ret;
> +			}

[Severity: Medium]
Could this new rollback cause a NULL pointer dereference in the parallel TX
thread when an RX thread setup fails?

When xsk_configure_rollback() cleans up the sockets on a failure, it calls
xsk_delete_socket_batch(), which in turn calls xsk_delete_socket(). That
function explicitly clears the umem pointer for the RX thread's sockets:

xsk_delete_socket() {
    ...
    xsk->umem = NULL;
}

If the RX thread fails and exits, the main test thread does not check
test->fail before spawning the TX thread. The TX thread then executes
thread_common_ops_tx(), which unconditionally dereferences the now-NULL
umem pointer:

thread_common_ops_tx() {
    ...
    if (!test->ifobj_rx || !test->ifobj_rx->xsk_arr[0].umem->umem) {
    ...
}

Does the main thread need to check for test failures before spawning the TX
thread, or should thread_common_ops_tx() verify xsk_arr[0].umem is valid
before dereferencing it?

>  			usleep(USLEEP_MAX);
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2