Re: [PATCH bpf-next 2/2] selftests/bpf: Test rhtab kptr recycle 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
> new file mode 100644
> index 000000000000..79bdcda95555
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c
> @@ -0,0 +1,87 @@

[ ... ]

> +	pmu_fd = syscall(__NR_perf_event_open, &attr, -1, 0, -1, 0);
> +	if (pmu_fd < 0) {
> +		test__skip();
> +		goto out;
> +	}

This isn't a bug, but should the test distinguish between the expected
no-hardware-PMU skip case (ENOENT, EOPNOTSUPP) and unexpected errors
(EACCES, EMFILE)? lru_lock_nmi.c checks errno before skipping:

    if (pmu_fd < 0) {
        if (i == 0 && (errno == ENOENT || errno == EOPNOTSUPP)) {
                test__skip();
                goto cleanup;
        }
        continue;
    }

Right now a real setup failure produces the same SKIP outcome as a
missing PMU, making it harder to debug.

> +	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. */
> +	usleep(100000);
> +
> +	bpf_link__destroy(skel->links.nmi_update);
> +	skel->links.nmi_update = NULL;
> +	close(pmu_fd);

Can this close a stale descriptor?

bpf_program__attach_perf_event() takes ownership of pfd on success: the
returned link stores it in bpf_link_perf->perf_event_fd, and
bpf_link_perf_detach() in libbpf closes it on both attach paths:

    if (perf_link->perf_event_fd != link->fd)
            close(perf_link->perf_event_fd);
    close(link->fd);

On the FEAT_PERF_LINK path link->fd is a separate bpf_link fd, so pfd
is closed by the first close(); on the legacy ioctl path link->fd ==
pfd, so it is closed by the second. Either way, after
bpf_link__destroy() the fd in pmu_fd is already closed, and
close(pmu_fd) operates on a stale descriptor number. Today it only
returns EBADF, but if any code added in between (or any concurrent
thread in the same test_progs worker) allocates an fd, that fd can be
closed out from under it.

The error path close(pmu_fd) after a failed attach is correct — the
attach opts only close link_fd, never pfd — so only this success-path
close is wrong.

Every comparable test in the same directory closes pmu_fd only on
attach failure and never after success: perf_event_stackmap.c:99-104,
stacktrace_build_id_nmi.c:47-52, and lru_lock_nmi.c (which states
"libbpf takes ownership of pfd on success" and only calls
bpf_link__destroy()). Should the close(pmu_fd) be dropped here?

> +	/*
> +	 * The old kptr must still be attached to the element: the NMI update
> +	 * path only cancels NMI-safe fields, mirroring hash map semantics.
> +	 * Before the fix the kptr was released from the NMI context and the
> +	 * probe below would see NULL.
> +	 */
> +	topts.retval = 0;
> +	if (!ASSERT_OK(bpf_prog_test_run_opts(bpf_program__fd(skel->progs.probe_elem),
> +					      &topts), "test_run_probe") ||
> +	    !ASSERT_EQ(topts.retval, 0, "probe_ret"))
> +		goto out;
> +
> +	ASSERT_EQ(read_counter(skel, 2), 1, "xchg_non_null");
> +	ASSERT_EQ(read_counter(skel, 3), 0, "xchg_null");

Does the test actually detect the bug it is meant to catch?

The BPF side bumps counters[1] on every successful NMI update, but the
harness never reads index 1 — it only checks indexes 2 and 3. If the
perf event never overflows during the 100 ms window, no rhtab update
happens at all, the kptr stashed by init_elem is trivially still there,
probe_elem takes the non-NULL branch, and both assertions
(xchg_non_null == 1, xchg_null == 0) pass — on a kernel with or without
the fix. The "no update happened" outcome and the "update happened and
correctly preserved the kptr" outcome are indistinguishable to this
test.

That outcome is plausible: the test generates no work on the monitored
CPU. The event is opened system-wide but for CPU 0 only
(syscall(__NR_perf_event_open, &attr, -1, 0, -1, 0)), the process is
never pinned to CPU 0, and the only thing it does while attached is
usleep(100000) — i.e. it sleeps. A CPU 0 that is idle in a deep C-state
does not advance PERF_COUNT_HW_CPU_CYCLES, so no sample and no NMI.
Whether the test exercises anything then depends on unrelated activity
landing on CPU 0.

Both existing NMI perf_event tests in the same directory do the two
things this one omits:

  - lru_lock_nmi.c:230 asserts the program actually ran:
    ASSERT_GT(skel->bss->hits, 0, "nmi_bpf_ran"), and drives load with
    per-CPU hammer threads;
  - stacktrace_build_id_nmi.c:61 pins its workload onto the monitored
    CPU: system("taskset 0x1 ./urandom_read 100000").

Should the test assert read_counter(skel, 1) > 0 before the kptr
assertions, and generate cycles on the monitored CPU rather than
sleeping?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/rhtab_kptr.c b/tools/testing/selftests/bpf/progs/rhtab_kptr.c
> new file mode 100644
> index 000000000000..25800a2e24e6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/rhtab_kptr.c
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2026 KylinSoft Co., Ltd. */

The commit adds a new BPF-program file progs/rhtab_kptr.c plus a new
prog_tests/rhtab_kptr.c for a single narrow rhtab behaviour (kptr
retention across an NMI-context value update). The same directory
already contains progs/rhash.c / prog_tests/rhash.c, which is the
general BPF_MAP_TYPE_RHASH test and already carries a
test__start_subtest() table for rhtab update/delete/exist semantics
(test_rhash_lookup_update, test_rhash_update_delete,
test_rhash_update_exist, test_rhash_update_any, ...), all driven from
SEC("syscall") programs against an RHASH map. Would adding one more
subtest plus one more program to that existing pair avoid a second
skeleton, a second map set and duplicated setup?

Counter-argument: progs/rhash.c has no kptr coverage at all (no __kptr
field, no map record) and prog_tests/map_kptr.c has no RHASH coverage,
so neither existing test covers this behaviour today, and the new test
additionally needs a SEC("perf_event") program which the rhash.c
skeleton does not have. On that reading the new file is a genuinely new
behaviour rather than a narrow variant of existing coverage, and a
separate file is defensible.

> +/* 0: init ok, 1: nmi update ok, 2: probe xchg non-NULL, 3: probe xchg NULL */
> +static __always_inline void bump(u32 idx)
> +{
> +	u64 *v = bpf_map_lookup_elem(&counters, &idx);
> +
> +	if (v)
> +		(*v)++;
> +}
> +
> +extern struct task_struct *bpf_task_acquire(struct task_struct *p) __ksym;
> +extern void bpf_task_release(struct task_struct *p) __ksym;
> +
> +SEC("perf_event")
> +int nmi_update(struct bpf_perf_event_data *ctx)
> +{
> +	struct val_t val = {};
> +	u32 key = 0;
> +
> +	if (bpf_map_update_elem(&rhtab, &key, &val, BPF_ANY) == 0)
> +		bump(1);
> +	return 0;
> +}

The counters map is documented as

  /* 0: init ok, 1: nmi update ok, 2: probe xchg non-NULL, 3: probe xchg NULL */

but only slots 2 and 3 are ever consumed. The userspace side
(prog_tests/rhtab_kptr.c) reads exactly read_counter(skel, 2) and
read_counter(skel, 3); slots 0 and 1 are written and never read.

Slot 1 is the only evidence that nmi_update() ran at all, and dropping
it makes the test unable to distinguish "the NMI update ran and the
kptr survived" from "the PMU never overflowed, so nothing happened".
The userspace side only waits usleep(100000) on a system-wide
PERF_TYPE_HARDWARE/PERF_COUNT_HW_CPU_CYCLES event pinned to CPU 0 while
the test thread is asleep; if CPU 0 stays idle (or the event is
virtualised/throttled) no sample is delivered. In that case probe_elem()
still finds the kptr installed by init_elem(), bump(2) fires once, and
both assertions

    ASSERT_EQ(read_counter(skel, 2), 1, "xchg_non_null");
    ASSERT_EQ(read_counter(skel, 3), 0, "xchg_null");

pass on a kernel that still has the eager bpf_obj_free_fields() in
rhtab_map_update_existing() — i.e. the test silently passes on the very
kernel it is meant to catch.

Would asserting read_counter(skel, 1) > 0 (and dropping the unused slot
0, whose information is already covered by topts.retval) close the gap
and turn a non-firing PMU into an explicit failure or skip instead of a
false pass?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31481833569