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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Duplicated refcount_t via memcpy breaks shared UMEM ownership, causing a double-free.
- [Medium] Stack-allocated structs in ifobj_zc_avail() cause a memory leak when xsk_umem__delete fails.
--

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

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

UMEM ownership is currently implicit, with freeing spread across multiple
error paths, which has led to leaks or use-after-free bugs. This commit
makes ownership explicit by adding a refcount_t users field to struct
xsk_umem_info. It centralizes socket teardown so the last socket to go
away frees the UMEM.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> index 38ce6060b8fac..d0c2d0efb97f5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> @@ -101,6 +101,7 @@ int xsk_configure_umem(struct ifobject *ifobj, struct xsk_umem_info *umem, void
>  		return ret;
>  
>  	umem->buffer = buffer;
> +	refcount_set(&umem->users, 1);

[Severity: Medium]
Will this lead to a double free of the shared UMEM?

In thread_common_ops_tx(), the umem structure is copied using memcpy:

	umem_tx = ifobject->xsk_arr[0].umem_real;
	memcpy(umem_tx, umem_rx, sizeof(*umem_tx));

Since this duplicates the new refcount_t users field, the reference count
between the TX and RX UMEM structures is decoupled.

When the test tears down, both TX and RX sockets will independently
decrement their separate UMEM refcounts to zero, causing
xsk_umem__delete() and munmap() to be executed twice on the same pointers.

>  	if (ifobj->shared_umem && ifobj->rx_on) {
>  		umem->base_addr = umem_size(umem);
>  		umem->next_buffer = umem_size(umem);

[ ... ]

> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 7dad8556a722e..a86eaf141e931 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -117,12 +117,12 @@ static void __exit_with_error(int error, const char *file, const char *func, int
>  
>  #define exit_with_error(error) __exit_with_error(error, __FILE__, __func__, __LINE__)
>  
> -static bool ifobj_zc_avail(struct ifobject *ifobject)
> +static bool ifobj_zc_avail(struct ifobject *ifobj)
>  {
>  	size_t umem_sz = DEFAULT_UMEM_BUFFERS * XSK_UMEM__DEFAULT_FRAME_SIZE;
>  	int mmap_flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE;
> -	struct xsk_socket_info *xsk;
> -	struct xsk_umem_info *umem;
> +	struct xsk_socket_info xsk = {};
> +	struct xsk_umem_info umem = {};

[Severity: Medium]
Could this result in a memory leak if xsk_umem__delete() fails?

The xsk and umem structures are now allocated on the stack. The new cleanup
logic in xsk_delete_socket() defers cleanup if xsk_umem__delete() fails:

	if (refcount_dec_and_test(&umem->users)) {
		if (umem->umem) {
			int err = xsk_umem__delete(umem->umem);
			if (err) {
				...
				/* Keep ownership explicit so a later cleanup pass can retry */
				refcount_set(&umem->users, 1);
				xsk->umem_ref = true;
				xsk->umem = umem;
				return;
			}

If xsk_umem__delete() returns an error, xsk_delete_socket() returns early
and defers munmap() by leaving state in the structure. 

Because the structs are stack-allocated, they are destroyed when
ifobj_zc_avail() returns. The deferred retry can never happen, meaning the
mmapped memory is permanently leaked.

>  	bool zc_avail = false;
>  	void *bufs;
>  	int ret;

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