Re: [PATCH net-next 1/5] selftests/xsk: add UMEM users refcount and centralize socket teardown

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Premature UMEM destruction during batch teardown of partially configured socket arrays causes `xsk_umem` file descriptor and memory leaks.
--

--- Patch [1]: [PATCH net-next 1/5] selftests/xsk: add UMEM users refcount and centralize socket teardown ---
commit 99dd730a09377ba26c180094a39cc7721ae72648
Author: Tushar Vyavahare <[email protected]>

selftests/xsk: add UMEM users refcount and centralize socket teardown

This commit introduces a reference count for shared UMEM users in the XSK
selftests and centralizes socket teardown in xsk_delete_socket(). It
replaces manual UMEM teardown functions with a unified batch deletion
approach to avoid leaks or double-releases during error handling.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> index 6eb9096d084c0..43ab61faad024 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
[ ... ]
> @@ -1702,12 +1718,71 @@ void *worker_testapp_validate_rx(void *arg)
>  	pthread_exit(NULL);
>  }
>  
> -static void testapp_clean_xsk_umem(struct ifobject *ifobj)
> +void xsk_delete_socket(struct xsk_socket_info *xsk)
>  {
> -	struct xsk_umem_info *umem = ifobj->xsk->umem;
> +	struct xsk_umem_info *umem;
[ ... ]
> +	/* Always decrement refcount for this socket's UMEM reference */
> +	if (refcount_dec_and_test(&umem->users)) {
> +		if (umem->umem) {
> +			int err = xsk_umem__delete(umem->umem);
> +
> +			if (err)
> +				ksft_print_msg("xsk_umem__delete failed: %d (umem still busy?)\n",
> +					       err);
> +		}
> +		if (umem->buffer && umem->mmap_size)
> +			munmap(umem->buffer, umem->mmap_size);
> +		umem->umem = NULL;
> +		umem->buffer = NULL;
> +		umem->mmap_size = 0;
> +	}
> +}
> +
> +static void xsk_delete_socket_batch(struct ifobject *ifobject, u32 count)
> +{
> +	u32 i;
> +
> +	if (!ifobject)
> +		return;
> +
> +	for (i = count; i > 0; i--)
> +		xsk_delete_socket(&ifobject->xsk_arr[i - 1]);
> +}

[Severity: Medium]
Could this result in a premature UMEM destruction and a subsequent file
descriptor leak during partial setup failures?

Since xsk->umem is pre-initialized for all array slots in
__test_spec_init(), uninitialized slots will still have a non-NULL
umem pointer. 

If setup fails halfway through (e.g., enable_busy_poll() fails for i = 0 in
xsk_configure()), the batch teardown iterates backwards from test->nb_sockets.
When it reaches an uninitialized slot (like i = 1), it will see the
pre-initialized umem pointer and decrement the refcount.

If this brings the refcount to 0, it prematurely attempts teardown. Since the
valid socket at i = 0 is still active, xsk_umem__delete() returns -EBUSY.
However, the code still proceeds to munmap the buffer and set umem->umem = NULL.

When the loop finally processes i = 0, the refcount is already 0, so it skips
calling xsk_umem__delete() entirely, permanently leaking the UMEM and its
file descriptor.

Is there a way to distinguish between a pre-initialized slot that never took a
reference and a successfully configured slot?

> +
> +static void xsk_delete_all_ifobj_sockets(struct test_spec *test, struct ifobject *ifobj)
> +{
> +	if (!ifobj)
> +		return;
> +
> +	xsk_delete_socket_batch(ifobj, test->nb_sockets);
>  }

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