Re: [PATCH 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Userspace stack out-of-bounds read in `bpf_map_update_elem()` due to mismatched value size.
- [Medium] Double close of the perf_event file descriptor (pmu_fd).
- [Low] Multi-line comments do not follow the mandatory BPF subsystem comment style.
--

--- Patch [3]: [PATCH 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context ---
commit ce5b7999a24e0290b526f254441d70dcbf59a6cf
Author: Yuan Chen <[email protected]>

selftests/bpf: Test rhtab kptr recycle from NMI context

This patch introduces a test to verify that the rhtab update and delete
recycle paths do not eagerly destroy referenced kptrs from NMI context.

> diff --git a/tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c b/tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c
> index 0000000000000..13158d74cbc1b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c

[ ... ]

> +void test_rhtab_kptr(void)
> +{
> +	struct perf_event_attr attr = {
> +		.type = PERF_TYPE_HARDWARE,
> +		.config = PERF_COUNT_HW_CPU_CYCLES,
> +		.freq = 1,
> +		.sample_freq = read_perf_max_sample_freq(),
> +		.size = sizeof(struct perf_event_attr),
> +	};
> +	LIBBPF_OPTS(bpf_test_run_opts, topts);
> +	struct rhtab_kptr *skel;
> +	__u32 key = 0;
> +	__u64 zero = 0;
> +	__u64 nonnull_before;
> +	int pmu_fd, i, err;
> +
> +	skel = rhtab_kptr__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +		return;
> +
> +	/* Create the element and stash a referenced task kptr in it. */
> +	if (!ASSERT_OK(bpf_map_update_elem(bpf_map__fd(skel->maps.rhtab),
> +					   &key, &zero, BPF_ANY), "create_elem"))

[Severity: Medium]
Does this code cause an out-of-bounds stack read?

The rhtab map expects a value of type struct val_t, which is 16 bytes in size
on 64-bit systems (an 8-byte pointer plus a 4-byte u32 and padding).

However, zero is defined as an 8-byte __u64 local variable. When
bpf_map_update_elem() copies 16 bytes from this address, it will read 8 bytes
of uninitialized or unrelated stack memory adjacent to zero.

> +		goto out;
> +	if (!ASSERT_OK(bpf_prog_test_run_opts(bpf_program__fd(skel->progs.init_elem),
> +					      &topts), "test_run_init") ||
> +	    !ASSERT_EQ(topts.retval, 0, "init_ret"))
> +		goto out;
> +
> +	pmu_fd = syscall(__NR_perf_event_open, &attr, -1, 0, -1, 0);
> +	if (pmu_fd >= 0) {
> +		skel->links.nmi_update = bpf_program__attach_perf_event(skel->progs.nmi_update,
> +									pmu_fd);
> +		if (!ASSERT_OK_PTR(skel->links.nmi_update, "attach_perf_event")) {
> +			close(pmu_fd);
> +			goto out;
> +		}
> +
> +		/* Let the NMI handler overwrite the element, and make sure it

[Severity: Low]
This isn't a bug, but does this comment violate the BPF subsystem multi-line
comment style?

The BPF subsystem requires multi-line comments to have the opening /* on its
own line.

> +		 * actually ran before probing (otherwise the probe would pass
> +		 * vacuously even on an unfixed kernel).
> +		 */
> +		for (i = 0; i < 20 && read_counter(skel, 1) == 0; i++)
> +			usleep(100000);
> +		ASSERT_GT(read_counter(skel, 1), 0, "nmi_update_ran");
> +
> +		bpf_link__destroy(skel->links.nmi_update);
> +		skel->links.nmi_update = NULL;
> +		close(pmu_fd);

[Severity: Medium]
Does this result in a double close of pmu_fd?

The function bpf_program__attach_perf_event() takes ownership of the passed
perf_event file descriptor upon successful attachment. When bpf_link__destroy()
is called, it automatically closes the file descriptor.

Calling close(pmu_fd) here explicitly could accidentally close an unrelated file
descriptor opened by another thread.

> +
> +		/*
> +		 * The old kptr must still be attached to the element: the

[ ... ]

> +	for (i = 0; i < 2000; i++) {
> +		topts.retval = 0;
> +		err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.init_elem),
> +					     &topts);
> +		if (err || topts.retval) {
> +			/* Element may be gone; recreate and retry once. */
> +			if (!ASSERT_OK(bpf_map_update_elem(bpf_map__fd(skel->maps.rhtab),
> +							   &key, &zero, BPF_ANY),

[Severity: Medium]
Can this also cause an out-of-bounds stack read?

This is the same issue as above where zero is 8 bytes but the map expects
16 bytes.

> +				       "recreate_elem"))
> +				goto out;

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/rhtab_kptr.c b/tools/testing/selftests/bpf/progs/rhtab_kptr.c
> index 0000000000000..fd6bd63cb405e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/rhtab_kptr.c

[ ... ]

> +struct {
> +	__uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
> +	__uint(max_entries, 5);
> +	__type(key, __u32);
> +	__type(value, __u64);
> +} counters SEC(".maps");
> +
> +/* 0: init ok, 1: nmi update ok, 2: probe xchg non-NULL, 3: probe xchg NULL,

[Severity: Low]
This isn't a bug, but should the opening /* be on its own line to match the BPF
subsystem multi-line comment style?

> + * 4: probe saw expected magic value
> + */
> +static __always_inline void bump(u32 idx)
> +{

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3