Re: [PATCH net-next v2 4/4] selftests/xsk: Introduce mmap_size in umem struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] Worker thread calls `pthread_barrier_wait` on an uninitialized barrier during single-thread tests.
- [High] Async-signal-unsafe call to `pthread_exit()` in `SIGUSR1` handler exposes the framework to deadlocks.
- [High] Single-thread tests are instantly aborted and completely bypass their testing logic.
- [Medium] Missing `pthread_join()` on the `pthread_barrier_destroy()` error path leads to a use-after-free race.
- [Medium] Missing error check for `pthread_create` leads to undefined behavior.
- [Low] Unmapped memory leak when `xsk_configure_umem` fails.
--

commit 154c93204cd9eb3588c96c98454860ecbe0e819c
Author: Tushar Vyavahare <tushar.vyavahare@intel.com>
Subject: selftests/xsk: Introduce mmap_size in umem struct

This patch stores the final mapping length in xsk_umem_info as mmap_size
when the UMEM mapping is created. This value is then used during teardown,
avoiding duplicated size arithmetic in cleanup. It also joins the RX worker
thread before cleanup in the single-thread path.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xsk.c b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> index f1730466ffd9c..e94c38a1faf47 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xsk.c
> @@ -1595,10 +1595,15 @@ static int thread_common_ops(struct test_spec *test, struct ifobject *ifobject)
>  	if (ifobject->shared_umem)
>  		umem_sz *= 2;
>  
> -	bufs = mmap(NULL, umem_sz, PROT_READ | PROT_WRITE, mmap_flags, -1, 0);
> +	mmap_sz = umem->unaligned_mode ?
> +		ceil_u64(umem_sz, HUGEPAGE_SIZE) * HUGEPAGE_SIZE : umem_sz;
> +
> +	bufs = mmap(NULL, mmap_sz, PROT_READ | PROT_WRITE, mmap_flags, -1, 0);
>  	if (bufs == MAP_FAILED)
>  		return -errno;
>  
> +	umem->mmap_size = mmap_sz;
> +
>  	ret = xsk_configure_umem(ifobject, umem, bufs, umem_sz);
>  	if (ret)
>  		return ret;

[Severity: Low]
This isn't a bug introduced by this patch, but does this leak the memory
mapped into bufs if xsk_configure_umem() fails?

If xsk_umem__create() fails inside xsk_configure_umem(), umem->buffer is
never assigned. The teardown logic in testapp_clean_xsk_umem() will later
call munmap(NULL, mmap_size) which fails with EINVAL, leaking the memory.

While a test suite memory leak is minor, could repeated failures exhaust
hugepages across test runs?

[ ... ]

> @@ -1706,15 +1711,9 @@ void *worker_testapp_validate_rx(void *arg)
>  static void testapp_clean_xsk_umem(struct ifobject *ifobj)
>  {
>  	struct xsk_umem_info *umem = ifobj->xsk->umem;
> -	u64 umem_sz = umem_size(umem);
> -
> -	if (ifobj->shared_umem)
> -		umem_sz *= 2;
> -
> -	umem_sz = ceil_u64(umem_sz, HUGEPAGE_SIZE) * HUGEPAGE_SIZE;
>  
>  	xsk_umem__delete(umem->umem);
> -	munmap(umem->buffer, umem_sz);
> +	munmap(umem->buffer, umem->mmap_size);
>  }
>  
>  static void handler(int signum)

[Severity: High]
This is a pre-existing issue, but does this async-signal-unsafe call to
pthread_exit() in the SIGUSR1 handler expose the framework to deadlocks?

pthread_exit() is explicitly documented by POSIX as async-signal-unsafe.
The test suite uses SIGUSR1 to asynchronously kill worker threads. If a
thread is interrupted while holding an internal libc lock (e.g., during
malloc(), mmap(), or BPF map ops), the lock is abandoned.

When the main thread subsequently runs clean_sockets() and calls free(),
it could deadlock trying to acquire the same lock.

[ ... ]

> @@ -1857,8 +1856,7 @@ static int __testapp_validate_traffic(struct test_spec *test, struct ifobject *i
>  
>  	if (!ifobj2)
>  		pthread_kill(t0, SIGUSR1);
> -	else
> -		pthread_join(t0, NULL);
> +	pthread_join(t0, NULL);

[Severity: High]
This isn't a bug introduced by this patch, but are single-thread tests being
instantly aborted and bypassing their testing logic?

For single-interface scenarios (ifobj2 == NULL), __testapp_validate_traffic()
spawns t0 and immediately executes pthread_kill(t0, SIGUSR1). The worker
thread is terminated instantly before it can run its test logic (such as
waiting for poll() to time out in testapp_poll_rxq_tmout).

Since test->fail defaults to false, does this cause the test to return a false
positive success, providing zero coverage?

[Severity: High]
This is also a pre-existing issue, but could worker threads call
pthread_barrier_wait() on an uninitialized barrier during single-thread tests?

In __testapp_validate_traffic(), barr is only initialized if (ifobj2).
However, single-thread tests (like testapp_poll_rxq_tmout) invoke
worker_testapp_validate_rx() via t0, which unconditionally calls
pthread_barrier_wait(&barr).

Is waiting on an uninitialized barrier undefined behavior that could cause
memory corruption or deadlocks?

[Severity: Medium]
Another pre-existing issue here: is there a use-after-free race on the
pthread_barrier_destroy() error path higher up in
__testapp_validate_traffic()?

If pthread_barrier_destroy(&barr) fails, the RX thread (t0) is signaled via
pthread_kill() but not joined before clean_sockets() and clean_umem() are
invoked:

	if (pthread_barrier_destroy(&barr)) {
		pthread_kill(t0, SIGUSR1);
		clean_sockets(test, ifobj1);
		clean_umem(test, ifobj1, NULL);
		return TEST_FAILURE;
	}

Could the worker thread still be running or executing its signal handler
while its resources are torn down?

[Severity: Medium]
This is also a pre-existing issue, but does the framework call
pthread_create() for t0 and t1 without checking the return value in
__testapp_validate_traffic()?

If thread creation fails (e.g., due to EAGAIN), the pthread_t variable is
left uninitialized. The code subsequently passes this uninitialized ID to
pthread_kill() and pthread_join().

Could this invoke undefined behavior?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603060327.298389-1-tushar.vyavahare@intel.com?part=4