Re: [PATCH RFC bpf-next 3/3] selftests/bpf: Stress LRU rqspinlock recovery paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `drain_and_refill` function fails to detect partial node leaks because LRU map eviction transparently hides lost capacity.
- [High] Stack buffer overflow when using `CPU_SET` on systems with a large number of CPUs.
- [Medium] Using glibc's `rand()` in concurrent userspace threads causes severe lock contention, serializing the stress threads and defeating the test's purpose.
- [Medium] Unchecked return value from `libbpf_num_possible_cpus()` is used to allocate Variable Length Arrays (VLAs).
- [Medium] Double close of the PMU perf_event file descriptors due to missing ownership transfer accounting.
--

commit de08c9fe543d53ce17c858bad1ef0cdae6fb41af
Author: Mykyta Yatsenko <yatsenko@meta.com>

selftests/bpf: Stress LRU rqspinlock recovery paths

This commit replaces the single-CPU NMI re-entry reproducer with a
parameterized stress harness to exercise LRU rqspinlock recovery paths.
It runs subtests against common LRU, per-CPU LRU lists, and per-CPU LRU
maps using multiple userspace threads and perf_event NMI BPF programs.

> diff --git a/tools/testing/selftests/bpf/prog_tests/lru_lock_nmi.c b/tools/testing/selftests/bpf/prog_tests/lru_lock_nmi.c
> new file mode 100644
> index 0000000000000..15b6606fef4d4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/lru_lock_nmi.c

[ ... ]

> +static void *hammer_thread(void *p)
> +{
> +	struct hammer_arg *a = p;
> +	__u64 val[a->nr_cpus];
> +	cpu_set_t cs;
> +	__u32 key;
> +
> +	memset(val, 0, sizeof(val));
> +	CPU_ZERO(&cs);
> +	CPU_SET(a->cpu, &cs);

[Severity: High]
Could this CPU_SET call write out of bounds on the stack?

The cpu_set_t structure is statically sized and often defaults to supporting
1024 CPUs. On machines with 1024 or more CPUs, setting a bit beyond
CPU_SETSIZE might corrupt memory. Would it be safer to use dynamically
allocated CPU sets via CPU_ALLOC and CPU_SET_S here?

> +	pthread_setaffinity_np(pthread_self(), sizeof(cs), &cs);
> +
> +	while (get_time_ns() < a->deadline_ns) {
> +		bool do_update = rand() & 1;
> +
> +		key = rand() % KEY_RANGE;

[Severity: Medium]
Will the use of glibc rand serialize these userspace threads?

Because rand uses a global internal lock to protect its PRNG state, calling
it in a tight loop across multiple threads pinned to different CPUs could
cause massive userspace contention. This might serialize the threads before
they enter the kernel and defeat the test's goal of stressing cross-CPU LRU
lock contention. Could a thread-local PRNG like rand_r be used instead?

> +		if (do_update)
> +			bpf_map_update_elem(a->map_fd, &key, val, BPF_ANY);
> +		else
> +			bpf_map_delete_elem(a->map_fd, &key);
> +	}
> +	return NULL;
> +}

[ ... ]

> +static int drain_and_refill(int map_fd, int nr_cpus)
> +{
> +	__u64 val[nr_cpus];
> +	__u32 key;
> +
> +	memset(val, 0, sizeof(val));
> +	for (key = 0; key < KEY_RANGE; key++)
> +		bpf_map_delete_elem(map_fd, &key);
> +
> +	for (key = 0; key < MAP_ENTRIES; key++)
> +		if (bpf_map_update_elem(map_fd, &key, val, BPF_ANY))
> +			return -ENOMEM;

[Severity: High]
Does this check reliably detect partial leaks in an LRU map?

Since this is an LRU map, if the usable capacity shrinks (for example, to 63
due to a leaked node), the 64th insertion will likely not fail with -ENOMEM.
Instead, it will evict the first inserted element and succeed.

Should the test perform lookups for all inserted keys after this loop finishes
to ensure none were prematurely evicted, in order to accurately verify that no
capacity was lost?

> +	return 0;
> +}
> +
> +static void run_variant(enum bpf_map_type type, __u32 map_flags,
> +			const char *name)
> +{
> +	struct perf_event_attr attr = {
> +		.size = sizeof(attr),
> +		.type = PERF_TYPE_HARDWARE,
> +		.config = PERF_COUNT_HW_CPU_CYCLES,
> +		.freq = 1,
> +	};
> +	int nr_cpus = libbpf_num_possible_cpus();
> +	int pmu_fds[nr_cpus];
> +	struct bpf_link *links[nr_cpus];

[Severity: Medium]
What happens if libbpf_num_possible_cpus returns a negative error code?

Passing a negative size to these variable length arrays would result in
undefined behavior and immediate stack pointer corruption. Should we check
if nr_cpus is positive before using it to declare these arrays?

[ ... ]

> +	for (i = 0; i < nr_cpus; i++) {
> +		pmu_fds[i] = syscall(__NR_perf_event_open, &attr, -1, i,
> +				     -1, 0);
> +		if (pmu_fds[i] < 0) {
> +			if (i == 0 &&
> +			    (errno == ENOENT || errno == EOPNOTSUPP)) {
> +				test__skip();
> +				goto cleanup;
> +			}
> +			continue;
> +		}
> +		links[i] = bpf_program__attach_perf_event(skel->progs.oncpu,
> +							  pmu_fds[i]);
> +		if (!links[i]) {
> +			close(pmu_fds[i]);
> +			pmu_fds[i] = -1;
> +		}
> +	}

[Severity: Medium]
Could this result in a double close of the PMU file descriptors?

When bpf_program__attach_perf_event successfully attaches, the returned
bpf_link takes ownership of the perf event file descriptor and will close it
automatically during bpf_link__destroy.

Since pmu_fds[i] isn't set to -1 on a successful attachment, the cleanup loop
later in the function will call close on pmu_fds[i] after bpf_link__destroy
has already closed it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528-lru_map_spin-v1-0-4f52223170cf@meta.com?part=3