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

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 5629 bytes --]

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c

[ ... ]

> @@ -1701,12 +1717,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;
> +
> +	/*
> +	 * Callers must serialize teardown for a given xsk/umem pair.
> +	 * This helper does refcount-based lifetime management only.
> +	 */
> +	if (!xsk)
> +		return;
>
> -	xsk_umem__delete(umem->umem);
> -	munmap(umem->buffer, umem->mmap_size);
> +	umem = xsk->umem;
> +	if (!umem)
> +		return;
> +
> +	/* Delete the socket handle when available, then drop UMEM reference. */
> +	if (xsk->xsk)
> +		xsk_socket__delete(xsk->xsk);
> +	xsk->xsk = NULL;
> +
> +	/* Mark this socket as cleaned up to prevent double-decrement */
> +	xsk->umem = NULL;
> +
> +	/*
> +	 * Teardown may walk the full xsk array, including slots that never completed setup but
> +	 * still carry a preinitialized umem pointer. After a partial-setup rollback, the last UMEM
> +	 * ref may already be dropped.
> +	 */
> +	if (!refcount_read(&umem->users))
> +		return;
> +
> +	/* Always decrement refcount for this socket's UMEM reference */

This isn't a bug, but does this comment still describe the code below it?
The early return just above it skips the decrement when users is already
zero, and three lines down the comment explains why that can happen, so
"Always" might not be the right word here.

Also, could the other comment blocks be trimmed? The four comments in this
roughly 50-line function mostly restate what the line below them does
("Callers must serialize" above an if (!xsk) check, "Delete the socket
handle when available", "Mark this socket as cleaned up"), and the
neighbouring helpers in this file carry no comments at all. The one
non-obvious point (why a slot can be reached with users already at 0)
might be enough on its own.

> +	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;
> +	}
> +}

[ ... ]

> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c

[ ... ]

> @@ -117,12 +117,13 @@ 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_info = {};
> +	struct xsk_umem_info *umem = &umem_info;

This isn't a bug, but would declaring struct xsk_umem_info umem = {}; and
passing &umem directly read a bit more plainly here? The conversion to
stack allocation introduces umem_info for the storage and umem as a
pointer alias on the next line, but the alias is only used four times
(umem->mmap_size, umem->frame_size, and passing umem to two functions),
three of which the patch already rewrites. This struct T *p = &local;
pattern doesn't appear anywhere else in xskxceiver.c, test_xsk.c, or
xsk.c, so it's not following a local idiom.

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

[ ... ]

Commit message question:

>     selftests/xsk: add UMEM users refcount and centralize socket teardown
>
>     Shared-UMEM lifetime can be torn down from multiple setup and cleanup
>     paths. Mixing manual UMEM free/unmap in one path with helper-based
>     teardown in others makes ownership easier to violate and can leak or
>     double-release UMEM during error handling.
>
>     Track UMEM ownership with a refcount_t users field. Initialize it to 1
>     in xsk_configure_umem() and increment it for each additional shared
>     socket. Introduce xsk_delete_socket() as the single teardown helper: it
>     deletes the socket handle, clears state to prevent double-decrement, and
>     releases the UMEM when the last reference drops.
>
>     Replace testapp_clean_xsk_umem(), clean_sockets(), and clean_umem() with
>     xsk_delete_socket_batch() and xsk_delete_all_ifobj_sockets() wrappers at
>     all call sites.

This isn't a bug, but would it help to explain why this refactor is
needed rather than walking through what the diff does? Two of the three
paragraphs describe which function sets the counter to 1, which helper is
introduced, and which three helpers are replaced by which two wrappers.
The opening paragraph states that mixing manual and helper-based teardown
"can leak or double-release UMEM during error handling" but doesn't name
a concrete path, and there's no Fixes: tag, so it's not clear whether an
existing bug is being fixed or the change is preparatory for the
shared-UMEM tests added later in the series.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31554903166