Re: [PATCH bpf 2/2] selftests/bpf: Test that a drained task_vma iterator keeps returning NULL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] selftests/bpf: short bpf_loop() spin in
  iter_task_vma_next_after_drain()
- [Low] selftests/bpf: missing acquire/release ordering in map_new_vma()
- [Low] selftests/bpf: mmap() at last_end in map_new_vma() fails at
  TASK_SIZE
- [Low] selftests/bpf: standalone task_vma_iter test instead of
  extending iters.c
--

commit 6861abec39a496e31db7f94849dcb9afd420ad9b
Author: Kang Chen <k.chen@smail.nju.edu.cn>

selftests/bpf: Test that a drained task_vma iterator keeps returning NULL

This commit adds a BPF selftest verifying that calling
bpf_iter_task_vma_next() on an already drained task_vma iterator continues
to return NULL even after a new VMA is mapped at the end of the address
space.

> diff --git a/tools/testing/selftests/bpf/prog_tests/task_vma_iter.c b/tools/testing/selftests/bpf/prog_tests/task_vma_iter.c
> new file mode 100644
> index 0000000000000..bb9feb840a277
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/task_vma_iter.c
> @@ -0,0 +1,89 @@

[ ... ]

> +static void *map_new_vma(void *arg)
> +{
> +	struct map_ctx *ctx = arg;
> +	struct task_vma_iter *skel = ctx->skel;
> +	void *addr;
> +
> +	while (!__atomic_load_n(&skel->bss->drain_done, __ATOMIC_RELAXED))
> +		sched_yield();
> +
> +	if (!skel->bss->new_ret) {
> +		addr = (void *)(unsigned long)skel->bss->last_end;

[Severity: Low]
Can map_new_vma() observe drain_done == 1 before the stores to last_end
and new_ret are visible on weakly-ordered architectures?

iter_task_vma_next_after_drain() writes last_end, new_ret, and drain_done
with plain stores, while map_new_vma() polls drain_done with
__ATOMIC_RELAXED instead of __ATOMIC_ACQUIRE.  Similarly, map_new_vma()
sets go with __ATOMIC_RELEASE, but wait_for_go() reads go with a plain
load.  If the stores are reordered, mmap() with MAP_FIXED_NOREPLACE can
use a stale last_end address and fail the test.

> +		ctx->addr = mmap(addr, getpagesize(), PROT_READ | PROT_WRITE,
> +				 MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED_NOREPLACE,
> +				 -1, 0);
> +		if (ctx->addr == MAP_FAILED)
> +			ctx->err = errno;
> +	}

[Severity: Low]
Can mmap() at last_end fail with ENOMEM when the highest VMA ends at
TASK_SIZE?

iter_task_vma_next_after_drain() records vm_end of the final VMA in the
process.  On architectures where STACK_TOP equals TASK_SIZE, the stack
VMA's vm_end is TASK_SIZE whenever ASLR is disabled (such as
kernel.randomize_va_space=0 or under gdb) or when randomize_stack_top()
picks a zero offset, leaving no unmapped user address space at last_end.

[ ... ]

> +void test_task_vma_iter(void)
> +{
> +	if (test__start_subtest("next_after_drain"))
> +		test_task_vma_iter_next_after_drain();
> +}

[Severity: Low]
This isn't a bug, but tools/testing/selftests/bpf/prog_tests/iters.c and
tools/testing/selftests/bpf/progs/iters_task_vma.c already test the
open-coded bpf_iter_task_vma iterator.  Could this subtest be added there
instead of introducing standalone task_vma_iter files?

> diff --git a/tools/testing/selftests/bpf/progs/task_vma_iter.c b/tools/testing/selftests/bpf/progs/task_vma_iter.c
> new file mode 100644
> index 0000000000000..9159aa7fc48e4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/task_vma_iter.c
> @@ -0,0 +1,53 @@

[ ... ]

> +	/* Let user space map a new VMA at last_end before the extra next(). */
> +	drain_done = 1;
> +	bpf_loop(1000000, wait_for_go, NULL, 0);
> +	go_seen = go;

[Severity: Medium]
Can bpf_loop(1000000, wait_for_go, NULL, 0) finish before map_new_vma()
runs and sets go?

wait_for_go() is inlined into a tight loop of 1,000,000 reads that
completes in roughly 1 to 3 ms inside bpf_prog_test_run_syscall() with
migration disabled.  test_task_vma_iter_next_after_drain() starts the
syscall program immediately after pthread_create() without waiting for
map_new_vma() to start.  On a single-CPU system (especially under
!CONFIG_PREEMPT where the kernel loop is not preempted) or under parallel
test_progs load where thread startup and mmap() take more than a few
milliseconds, bpf_loop() exhausts its iterations before go is set and fails
the go_seen assertion.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113024500.922824-1-k.chen@smail.nju.edu.cn?part=2