selftests/xsk: improve shared-UMEM coverage

[kernel-patches-daemon-bpf]

Pull request for series with
subject: selftests/xsk: improve shared-UMEM coverage
version: 3
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173

[kernel-patches-daemon-bpf]

Upstream branch: aed1bf1
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-review-bot]

commit efba21e69adbed0fe08ba5e8dcdeb9056ddccd45
Author: Tushar Vyavahare <tushar.vyavahare@intel.com>

selftests/xsk: skip TX setup after RX configuration failure

When RX socket setup fails during the first traffic step, the RX worker
reports test->fail only after waiting at the setup barrier. As a result,
the main thread may begin TX setup before it detects the failure.
In shared-UMEM tests, TX setup relies on the RX-side socket and UMEM state
being initialized successfully. Proceeding with TX setup after an RX
configuration failure can therefore access uninitialized state.
Set test->fail before the RX worker reaches the barrier and verify it
before starting the TX thread. The existing teardown path after thread
join already handles cleanup of the RX socket slots and UMEM.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> index bba5e8d4222d..814b8325493f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> @@ -1699,12 +1699,15 @@ void *worker_testapp_validate_rx(void *arg)
>  				       strerror(-err));
>  	}
>
> +	/* Publish setup failure before releasing the main thread from the barrier. */
> +	if (err)
> +		test->fail = true;
> +
>  	if (test->use_barrier)
>  		pthread_barrier_wait(&barr);
>
>  	/* We leave only now in case of error to avoid getting stuck in the barrier */
>  	if (err) {
> -		test->fail = true;
>  		pthread_exit(NULL);
>  	}

This isn't a bug, but the braces in this if statement are now unnecessary.
You've moved test->fail = true; out, leaving a single-statement block:

	if (err) {
		pthread_exit(NULL);
	}

Could this be collapsed to the brace-less form?

	if (err)
		pthread_exit(NULL);

The lines you added just above already use this style (if (err) \n \t \t
test->fail = true;), so the two forms are inconsistent within the same
window.

[ ... ]

This looks like a bug fix for an initialization ordering issue. Should this
include:

  Fixes: 90d52cd76f2f ("selftests/xsk: add UMEM users refcount and centralize socket teardown")

---

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

In-Reply-To-Subject: selftests/xsk: skip TX setup after RX configuration failure
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32400920730

[kernel-patches-daemon-bpf]

Forwarding comment 5360031636 via email
In-Reply-To: 20260819024443.781764-3-tushar.vyavahare@intel.com
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/20260819024443.781764-3-tushar.vyavahare@intel.com/

[kernel-patches-review-bot]

commit fae088cfc88c570024d39e13c06e1e584f42e9fe
Author: Tushar Vyavahare <tushar.vyavahare@intel.com>

selftests/xsk: add shared-UMEM callback framework and initial test cases

This commit adds shared UMEM callback infrastructure and four new test
cases. It introduces pkt_stream_replace_seq() for regenerating per-socket
streams with safe two-phase allocation.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> index 814b8325493f..5d4eb47cbfee 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> @@ -224,6 +224,12 @@ int hw_ring_size_reset(struct ifobject *ifobj)
>  static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
>  			     struct ifobject *ifobj_rx)
>  {
> +	/*
> +	 * Keep the same default as xskxceiver startup: when TX and RX share the same netdev,
> +	 * shared UMEM is the baseline mode for this test harness. Individual tests can still
> +	 * override this as needed.
> +	 */
> +	bool shared_default = ifobj_tx->ifindex == ifobj_rx->ifindex;
>  	u32 i, j;
>
>  	for (i = 0; i < MAX_INTERFACES; i++) {
> @@ -235,6 +241,7 @@ static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
>  		ifobj->use_fill_ring = true;
>  		ifobj->release_rx = true;
>  		ifobj->validation_func = NULL;
> +		ifobj->shared_umem = shared_default;
>  		ifobj->use_metadata = false;

The shared_default derivation here can never be true in this harness,
can it?

prog_tests/xsk.c uses distinct veth pair ends (VETH_RX "veth0" /
VETH_TX "veth1"), and configure_ifobj() resolves two different
ifindexes. Nothing in test_xsk.c ever assigns ifobj->shared_umem =
true. The only other writer is testapp_poll_txq_tmout(), which merely
saves and restores the current value.

Consequently the umem->base_addr offsetting in xsk_configure_umem(),
the 'shared = (ifobject->shared_umem && tx)' branch in
xsk_configure(), and thread_common_ops_tx() remain unreachable.

The comment asserts the opposite: "when TX and RX share the same
netdev, shared UMEM is the baseline mode for this test harness." In
xskxceiver.c the equivalent derivation can be true because that
binary accepts '-i' twice with the same interface; the ported
harness has no such option, so the copied logic plus its three-line
justification describe behavior that cannot occur.

Should this assignment and comment be removed, or is there a plan to
add same-netdev configuration so the mode is actually exercised?

[ ... ]

> @@ -593,28 +600,95 @@ static int pkt_stream_receive_half(struct test_spec *test)
>  	return 0;
>  }
>
> +static int pkt_stream_replace_seq(struct test_spec *test, pkt_stream_dims_fn dims,
> +				  const void *ctx)
> +{
> +	struct pkt_stream *tx_streams[MAX_SOCKETS] = {};
> +	struct pkt_stream *rx_streams[MAX_SOCKETS] = {};
> +	u32 i, nb_pkts, pkt_len;
> +
> +	for (i = 0; i < test->nb_sockets; i++) {
> +		dims(test->ifobj_tx->xsk_arr[i].pkt_stream, i, ctx, &nb_pkts, &pkt_len);
> +		tx_streams[i] = __pkt_stream_generate(nb_pkts, pkt_len, i, 2);
> +		if (!tx_streams[i])
> +			goto err;
> +
> +		dims(test->ifobj_rx->xsk_arr[i].pkt_stream, i, ctx, &nb_pkts, &pkt_len);
> +		rx_streams[i] = __pkt_stream_generate(nb_pkts, pkt_len, i, 2);
> +		if (!rx_streams[i])
> +			goto err;
> +	}

The hardcoded stride of 2 in __pkt_stream_generate() is correct for the
pre-existing two-socket XDP_SHARED_UMEM test (it produces the even/odd
split), but is it also correct for the new SHARED_UMEM_4_SOCKETS case?

With four sockets the four generated sequences are not disjoint: socket 0
gets pkt_nb = {0,2,4,...,4094} and socket 2 gets pkt_nb = {2,4,...,4096};
socket 1 gets {1,3,...,4095} and socket 3 gets {3,5,...,4097}.

Because the payload written by pkt_generate()/write_payload() and
validated by is_frag_valid() is a function of pkt_nb only (the seqnum
word is '(pkt_nb << 16) | word_index'), a packet that leaks from TX
socket 0 to RX socket 2 (or 1 to 3, and vice versa) is indistinguishable
from the packet that socket really expected, and it is also received in
the expected order.

The only thing that would still catch cross-delivery between those pairs
is the aggregate per-socket count check in receive_pkts(), so the test
largely cannot detect the very misrouting it is meant to cover. The
changelog's '4-socket even/odd split' does not describe the generated
streams (there is no 4-way split).

Using test->nb_sockets as the stride argument would give each socket a
disjoint pkt_nb class while preserving current 2-socket behavior. Does
that make sense here?

[ ... ]

> +static void uneven_dist_dims(struct pkt_stream *orig, u32 sock_id, const void *ctx,
> +			     u32 *nb_pkts, u32 *pkt_len)
> +{
> +	const struct shared_umem_uneven_dist_ctx *cfg = ctx;
> +	u32 pkts_sock0 = cfg->total_pkts / 4;
> +
> +	*nb_pkts = sock_id ? cfg->total_pkts - pkts_sock0 : pkts_sock0;
> +	*pkt_len = cfg->pkt_len;
> +}

[ ... ]

> +static int pkt_stream_uneven_dist_seq(struct test_spec *test,
> +				      const struct shared_umem_uneven_dist_ctx *cfg)
> +{
> +	if (test->nb_sockets < 2 || cfg->total_pkts < 4)
> +		return -EINVAL;
> +
> +	return pkt_stream_replace_seq(test, uneven_dist_dims, cfg);
> +}

The guard validates nb_sockets < 2, but uneven_dist_dims() only
distinguishes socket 0 from "everything else":

	u32 pkts_sock0 = cfg->total_pkts / 4;
	*nb_pkts = sock_id ? cfg->total_pkts - pkts_sock0 : pkts_sock0;

So the 1:3 split it implements is only meaningful for exactly two
sockets. With nb_sockets == 3 or 4 every non-zero slot gets 3/4 *
total_pkts and the aggregate becomes (n - 1) * 3/4 + 1/4 of total_pkts
instead of total_pkts, silently contradicting the ctx the caller
supplied.

The check accepts precisely the configurations the callback cannot
express while rejecting nothing that matters (the only caller,
testapp_shared_umem_uneven_dist(), passes 2).

Should the condition be 'test->nb_sockets != 2', or should
uneven_dist_dims() distribute the remaining 3/4 across the remaining
sockets?

[ ... ]

---

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

In-Reply-To-Subject: selftests/xsk: add shared-UMEM callback framework and initial test cases
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32400920730

[kernel-patches-review-bot]

commit 90d52cd76f2fe834a2cca6abb76d0ae53c96b8e5
Author: Tushar Vyavahare <tushar.vyavahare@intel.com>

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

This commit refactors XSK selftest cleanup by adding explicit refcount
tracking to UMEM ownership, fixing memory leaks and use-after-unmap bugs
where teardown paths could unmap buffers while sockets still referenced
them.

> commit 90d52cd76f2fe834a2cca6abb76d0ae53c96b8e5
> Author: Tushar Vyavahare <tushar.vyavahare@intel.com>
> Date:   Wed Aug 19 02:44:40 2026 +0000
>
>     selftests/xsk: add UMEM users refcount and centralize socket teardown
>
>     UMEM ownership is currently implicit. xsk_configure_umem() maps the
>     buffer, but freeing it is spread across testapp_clean_xsk_umem(),
>     clean_sockets() and clean_umem(), each called from a different error path.
>
>     thread_common_ops() returns without releasing the UMEM when
>     xsk_configure() fails after the UMEM was already mapped, leaking the
>     mmap()ed buffer.
>
>     The pthread_barrier_destroy() failure path in __testapp_validate_traffic()
>     unmaps the UMEM owned by ifobj1 while sockets belonging to ifobj2 are left
>     alive. With shared_umem, those sockets reference the buffer that was just
>     unmapped.
>
>     Make ownership explicit instead. Add a refcount_t users field to struct
>     xsk_umem_info and release exactly one reference per socket on teardown, so
>     that the last socket to go away frees the UMEM regardless of which path
>     tears it down. Keep shared sockets pointing to the same xsk_umem_info
>     instead of copying ownership state, so all users update the same reference
>     count.
>
>     Propagate UMEM deletion errors from xsk_delete_socket() so callers with
>     stack-allocated state can handle a deferred cleanup before that state goes
>     out of scope. The in-test teardown paths cannot retry, because
>     __test_spec_init() clears the UMEM state at the start of the next test and
>     the mapping is then unrecoverable, so xsk_delete_all_ifobj_sockets()
>     records the failure in test->fail instead of dropping it. A UMEM that
>     survives teardown now fails the test rather than leaking silently.
>
>     clean_umem() skipped ifobj2 whenever it shared the UMEM owned by ifobj1,
>     which was only there to avoid unmapping the same buffer twice. The
>     reference count makes that distinction unnecessary, so the helper and its
>     special case are removed along with the other ad-hoc cleanup paths.
>
>     Co-developed-by: Magnus Karlsson <magnus.karlsson@intel.com>
>     Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
>     Signed-off-by: Tushar Vyavahare <tushar.vyavahare@intel.com>

This looks like a bug fix for the shared UMEM feature. Should this
include:

  Fixes: 6d198a89c004 ("selftests/xsk: Add a test for shared umem feature")

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> index 4549358cc8c2..bba5e8d4222d 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c

[ ... ]

> @@ -1508,8 +1529,15 @@ static int thread_common_ops_tx(struct test_spec *test, struct ifobject *ifobjec
>  	}
>
>  	umem_rx = test->ifobj_rx->xsk_arr[0].umem;
> +	/* Non-owning view used only for TX buffer arithmetic; the sockets below bind to
> +	 * the RX-owned UMEM, so the handle, cq and refcount stay in one place.
> +	 */
>  	umem_tx = ifobject->xsk_arr[0].umem_real;

A subsystem pattern flags this as potentially concerning: BPF code style
expects multi-line comments to have the opening /* on its own line
(bpf.md guide), even when surrounding code uses the old style. The patch
shows the correct format in xsk_configure_socket():

	/*
	 * For shared sockets refcount_inc hasn't run yet, so clear umem to mark this slot
	 * as having no reference. For the owner (non-shared) the reference was taken by
	 * xsk_configure_umem; leave umem set so the caller's rollback path can release it
	 * via umem_ref.
	 */

but three other new comments in this patch start their text on the same
line as the opening /* (this one, the "Keep ownership explicit" comment
in xsk_delete_socket, and the "A UMEM that could not be deleted" comment
in xsk_delete_all_ifobj_sockets). Would it be clearer to follow the same
format throughout?

> -	memcpy(umem_tx, umem_rx, sizeof(*umem_tx));
> +	umem_tx->num_frames = umem_rx->num_frames;
> +	umem_tx->frame_headroom = umem_rx->frame_headroom;
> +	umem_tx->buffer = umem_rx->buffer;
> +	umem_tx->frame_size = umem_rx->frame_size;
> +	umem_tx->unaligned_mode = umem_rx->unaligned_mode;
>  	umem_tx->base_addr = 0;
>  	umem_tx->next_buffer = 0;
>
> @@ -1599,6 +1627,8 @@ static int thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
>  	ret = xsk_configure_umem(ifobject, umem, bufs, umem_sz);
>  	if (ret)
>  		return ret;
> +	/* Mark before xsk_configure so rollback can release the UMEM if it fails. */
> +	ifobject->xsk->umem_ref = true;
>
>  	ret = xsk_configure(test, ifobject, umem, false);
>  	if (ret)

[ ... ]

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.h b/tools/testing/selftests/bpf/prog_tests/test_xsk.h
> index 03753ddc5dcd..15d6fe2b9568 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xsk.h
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.h
> @@ -4,6 +4,7 @@
>
>  #include <linux/ethtool.h>
>  #include <linux/if_xdp.h>
> +#include <linux/refcount.h>
>
>  #include "../kselftest.h"
>  #include "xsk.h"

[ ... ]

> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 7dad8556a722..5d8a29e7224e 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c

[ ... ]

---

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

In-Reply-To-Subject: selftests/xsk: add UMEM users refcount and centralize socket teardown
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32400920730

[kernel-patches-daemon-bpf]

Forwarding comment 5360114428 via email
In-Reply-To: 20260819024443.781764-5-tushar.vyavahare@intel.com
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/20260819024443.781764-5-tushar.vyavahare@intel.com/

[kernel-patches-daemon-bpf]

Forwarding comment 5360178594 via email
In-Reply-To: 20260819024443.781764-2-tushar.vyavahare@intel.com
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/20260819024443.781764-2-tushar.vyavahare@intel.com/

[kernel-patches-daemon-bpf]

Upstream branch: d19862d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-daemon-bpf]

Upstream branch: 4191e49
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-daemon-bpf]

Upstream branch: 4954de7
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-daemon-bpf]

Upstream branch: e40f61a
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-daemon-bpf]

Upstream branch: 669e4fa
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-daemon-bpf]

Upstream branch: a284ed4
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-daemon-bpf]

Upstream branch: 5e289c5
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-daemon-bpf]

Upstream branch: 5e289c5
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-daemon-bpf]

Upstream branch: d83fba2
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-daemon-bpf]

Upstream branch: ce36e38
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-daemon-bpf]

Upstream branch: 05ea1b6
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3

[kernel-patches-daemon-bpf]

Upstream branch: 1555de3
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1148173
version: 3