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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Double close of `pmu_fd` in the success path.
- [Low] Missing FD validation for manually looked up BPF program.
- [Low] Multi-line comments do not follow the mandatory BPF subsystem style.
- [Low] Calling test__skip() marks the entire test as skipped, but execution continues to the recycle phase, masking its success on systems without a PMU.
--

commit 2b5822e0c1417c4403ffbea06426d15e411775a3
Author: Yuan Chen <chenyuan@kylinos.cn>

selftests/bpf: Test rhtab kptr recycle from NMI context

A perf_event program running in NMI context overwrites a rhtab element whose
value holds a referenced task kptr to verify it stays attached on recycle. A
second phase deletes and re-inserts the element 2000 times, requiring at
least one recycle to inherit the kptr. The NMI phase is skipped when no
hardware PMU is available.

> diff --git a/tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c b/tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c
> index 0000000000000..4bdcc9ce5500f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c
[ ... ]
> +/* Userspace mirror of the BPF-side struct val_t (progs/rhtab_kptr.c). The
> + * update syscall copies map->value_size bytes from the buffer, so it must
> + * be at least that large; special fields are skipped by the value copy but
> + * the kernel still reads the full value_size from userspace.
> + */
> +struct val_t_user {

[Severity: Low]
This isn't a bug, but does this comment violate the BPF subsystem multi-line
comment style? The guidelines mandate that multi-line comments must have the
opening /* on its own line, with text starting on the subsequent line.

[ ... ]
> +/* Run @name via BPF_PROG_TEST_RUN, asserting both the syscall status and
> + * that the program exited 0. Returns 0 on success.
> + */
> +static int run_prog_ok(struct rhtab_kptr *skel, const char *name)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, topts);
> +	struct bpf_program *prog;
> +	int err;
> +
> +	prog = bpf_object__find_program_by_name(skel->obj, name);
> +	if (!ASSERT_OK_PTR(prog, name))
> +		return -1;
> +	err = bpf_prog_test_run_opts(bpf_program__fd(prog), &topts);

[Severity: Low]
Is it safe to use the file descriptor returned by bpf_program__fd() here
without checking it?

The BPF Skeleton API guidelines require that file descriptors returned by
bpf_program__fd() when using manual lookup APIs like
bpf_object__find_program_by_name() must be validated (e.g., using ASSERT_GE)
before use.

[ ... ]
> +	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
> +		 * actually ran before probing (otherwise the probe would pass
> +		 * vacuously even on an unfixed kernel).
> +		 */

[Severity: Low]
This isn't a bug, but there's another instance of the multi-line comment
style violation here.

> +		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]
Will this result in a double close of pmu_fd?

When bpf_link__destroy() is called on this link, it invokes
bpf_link_perf_detach(), which internally closes the pmu_fd. Calling
close(pmu_fd) immediately after could inadvertently close an unrelated file
descriptor opened by a concurrent test thread.

[ ... ]
> +		ASSERT_EQ(read_counter(skel, 2), 1, "xchg_non_null");
> +		ASSERT_EQ(read_counter(skel, 3), 0, "xchg_null");
> +	} else {
> +		test__skip();
> +	}

[Severity: Low]
When test__skip() is called here, execution continues to the recycle phase
below rather than returning early. Could this hide the success of the recycle
phase under a SKIPPED test status on systems without a PMU?

> +
> +	/*
> +	 * Now exercise the delete/re-insert recycle path. The delete only
> +	 * cancels NMI-safe fields, so the freed element still owns the kptr.
[ ... ]
> +		if (run_prog_ok(skel, "init_elem") != 0) {
> +			/* init_elem fails only if the element is missing,
> +			 * which must not happen in this single-threaded
> +			 * loop; count it so a rhtab bug cannot be absorbed
> +			 * silently.
> +			 */

[Severity: Low]
This isn't a bug, but this comment also violates the BPF subsystem multi-line
comment style.

> +			retries++;
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/rhtab_kptr.c b/tools/testing/selftests/bpf/progs/rhtab_kptr.c
> index 0000000000000..c96cf7f2d799b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/rhtab_kptr.c
[ ... ]
> +/* 0: init ok, 1: nmi update ok, 2: probe xchg non-NULL, 3: probe xchg NULL,
> + * 4: probe saw expected magic value
> + */
> +static __always_inline void bump(u32 idx)

[Severity: Low]
This isn't a bug, but this comment also violates the BPF subsystem multi-line
comment style.

[ ... ]
> +SEC("syscall")
> +int del_elem(void *ctx)
> +{
> +	u32 key = 0;
> +
> +	if (bpf_map_delete_elem(&rhtab, &key))
> +		return 1;
> +	/* The element must really be gone: otherwise the following upd_elem()
> +	 * is an in-place update on the surviving element and the kptr that
> +	 * probe_elem() observes never went through a recycle.
> +	 */

[Severity: Low]
This isn't a bug, but this comment also violates the BPF subsystem multi-line
comment style.

> +	if (bpf_map_lookup_elem(&rhtab, &key))
> +		return 2;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901062845.1379760-1-chenyuan_fl@163.com?part=3