Re: [PATCH bpf-next 9/9] selftests/bpf: Test uprobes and fork() with busy mmap_lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/bpf: integer overflow of AREA_MAX on 32-bit in hold_mmap_lock()
--

commit 27a041a03328fba58bd21d5e62e3d149445f0523
Author: Alexei Starovoitov <ast@kernel.org>

selftests/bpf: Test uprobes and fork() with busy mmap_lock

This commit adds a BPF selftest that exercises uprobe attach and detach
concurrently with fork() while a process holds its mmap_lock via
MADV_POPULATE_READ.

> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c b/tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c
> new file mode 100644
> index 0000000000000..30614e8b92cd5
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c
> @@ -0,0 +1,447 @@

[ ... ]

> +#define AREA_MAX	(64UL << 30)
> +#define AREA_MIN	(64UL << 20)

[ ... ]

> +/*
> + * MADV_POPULATE_READ holds mmap_lock for reading while it maps zero page
> + * all over the area. It costs page tables only.
> + */
> +static void *hold_mmap_lock(void *arg)
> +{
> +	size_t size = AREA_MIN;
> +	void *area;
> +	__u64 t;
> +
> +	area = mmap(NULL, AREA_MAX, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0);
> +	if (area == MAP_FAILED)
> +		goto err;

[Severity: Medium]
In tools/testing/selftests/bpf/prog_tests/uprobe_busy_mm.c, does AREA_MAX
overflow on 32-bit architectures when passed to mmap() in hold_mmap_lock()?

When serial_test_uprobe_busy_mm() runs on a 32-bit system via:

  serial_test_uprobe_busy_mm()
    -> start_busy_process()
      -> busy_process()
        -> hold_mmap_lock()

unsigned long and size_t are 32 bits wide, so (64UL << 30) overflows to 0
(and a 64 GiB area exceeds the 32-bit virtual address space). Passing 0 as
the length to mmap() causes it to return MAP_FAILED with EINVAL, jumping to
err and failing the test.

Should AREA_MAX be sized appropriately for 32-bit architectures as well?

> +	/* EINVAL without CONFIG_TRANSPARENT_HUGEPAGE */
> +	madvise(area, AREA_MAX, MADV_NOHUGEPAGE);
> +
> +	t = now_ns();
> +	if (madvise(area, size, MADV_POPULATE_READ))
> +		goto err;
> +	t = now_ns() - t;
> +	size *= sh->hold_ms * 1000000ULL / t + 1;
> +	if (size > AREA_MAX)
> +		size = AREA_MAX;
> +
> +	while (!LOAD(sh->stop)) {
> +		STORE(sh->in_hold, 1);
> +		t = now_ns();
> +		madvise(area, size, MADV_POPULATE_READ);
> +		/* before anything that can fault and wait for fork() */
> +		INC(sh->hold_seq);
> +		STORE(sh->hold_ns, now_ns() - t);
> +		STORE(sh->in_hold, 0);
> +		madvise(area, size, MADV_DONTNEED);
> +		STORE(sh->ready, 1);
> +	}
> +	return NULL;
> +err:
> +	INC(sh->errors);
> +	STORE(sh->ready, 1);
> +	return NULL;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022015712.1297331-1-alexei.starovoitov@gmail.com?part=9