bpf: Cancel special fields in resizable hashtab on recycle

[kernel-patches-daemon-bpf]

Pull request for series with
subject: bpf: Cancel special fields in resizable hashtab on recycle
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150975

[kernel-patches-daemon-bpf]

Upstream branch: 5e289c5
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150975
version: 1

[kernel-patches-review-bot]

commit 8d1f2c9fcffe8f47040112ef6ea259b5962b1f71
Author: Yuan Chen <chenyuan@kylinos.cn>

bpf: Fix use-after-free of program BTF in mem-alloc destructor

This commit adds reference counting for program BTF in the bpf_mem_alloc
destructor path to prevent use-after-free when the program BTF is freed
before the deferred workqueue runs.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 0df8db27cd8c..b8df2bc9a9a0 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c

[ ... ]

> +/*
> + * bpf_ma_set_dtor() duplicates the map's btf_record. For kptr fields whose
> + * btf is the program BTF (MEM_ALLOC kptrs, e.g. objects allocated with
> + * bpf_obj_new()/bpf_percpu_obj_new()) btf_record_dup() only borrows the
> + * reference, like btf_parse_fields() did for the map's own record. The
> + * duplicated record is released later from the deferred bpf_mem_alloc
> + * destructor workqueue, by which time the program BTF may already have been
> + * freed (the map dropped its own reference in bpf_map_free()), so reading
> + * field->kptr.btf there would be a use-after-free.
> + *
> + * Hold a reference on non-kernel (program) BTF for the lifetime of the
> + * duplicated record and release it before the record is freed. After the
> + * last btf_put() the object is only destroyed after an RCU grace period, so
> + * btf_record_free() can still safely read the field descriptors.
> + */
> +static void htab_record_prog_btf_ref(struct btf_record *rec, bool get)
> +{
> +	int i;
> +
> +	if (IS_ERR_OR_NULL(rec))
> +		return;
> +
> +	for (i = 0; i < rec->cnt; i++) {
> +		const struct btf_field *field = &rec->fields[i];
> +
> +		switch (field->type) {
> +		case BPF_KPTR_UNREF:
> +		case BPF_KPTR_REF:
> +		case BPF_KPTR_PERCPU:
> +		case BPF_UPTR:
> +			if (field->kptr.btf && !btf_is_kernel(field->kptr.btf)) {
> +				if (get)
> +					btf_get(field->kptr.btf);
> +				else
> +					btf_put(field->kptr.btf);
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
>  static void htab_dtor_ctx_free(void *ctx)
>  {
>  	struct htab_btf_record *hrec = ctx;
>
> +	htab_record_prog_btf_ref(hrec->record, false);
>  	btf_record_free(hrec->record);
>  	kfree(ctx);
>  }

Does this ordering still have a use-after-free?

The comment states that "After the last btf_put() the object is only
destroyed after an RCU grace period, so btf_record_free() can still safely
read the field descriptors." But in htab_dtor_ctx_free(), the program-BTF
reference is dropped one statement before the record is freed:

    htab_record_prog_btf_ref(hrec->record, false);   /* btf_put() */
    btf_record_free(hrec->record);                   /* reads field->kptr.btf */

htab_record_prog_btf_ref(..., false) ends in btf_put(field->kptr.btf).
Looking at btf_put() in kernel/bpf/btf.c, it only queues the object for RCU
destruction if this was the last reference:

    void btf_put(struct btf *btf)
    {
            if (btf && refcount_dec_and_test(&btf->refcnt)) {
                    btf_free_id(btf);
                    call_rcu(&btf->rcu, btf_free_rcu);
            }
    }

and btf_free_rcu() -> btf_free() -> kfree(btf). The very next statement,
btf_record_free() in kernel/bpf/syscall.c, does for every kptr field:

            if (btf_is_kernel(rec->fields[i].kptr.btf))
                    btf_put(rec->fields[i].kptr.btf);

and btf_is_kernel() is 'return btf->kernel_btf;' - a dereference of the
object whose kfree() was just queued.

In the deferred path this is the last reference, so the drop queues an RCU
free and the following read can land on freed memory. The reachable path is
exactly the scenario described in the commit message:

  bpf_map_free_deferred()
    -> bpf_map_free()
       -> map->ops->map_free (rhtab_map_free / htab_map_free)
          -> bpf_mem_alloc_destroy()
             -> destroy_mem_alloc(), rcu_in_progress != 0
                -> queue_work(system_dfl_wq, &copy->work)
       -> btf_record_free(rec); btf_put(btf);   /* map drops its own BTF ref */
    ... later, on the workqueue ...
  free_mem_alloc_deferred()
    -> free_mem_alloc()
       -> free_mem_alloc_no_barrier()
          -> ma->dtor_ctx_free(ma->dtor_ctx) == htab_dtor_ctx_free

By then the map's reference is gone, so the btf_put() inside
htab_record_prog_btf_ref() drops the last reference and queues
btf_free_rcu. free_mem_alloc_deferred() is a plain, fully preemptible
workqueue worker; there is no rcu_read_lock() anywhere between the work
callback and htab_dtor_ctx_free() or btf_record_free(). Being preempted
outside an RCU read-side critical section is itself a quiescent state, so
on CONFIG_PREEMPT/PREEMPT_RT the grace period can complete and
btf_free_rcu() can run before the worker resumes in btf_record_free().

Could the reference be held until after btf_record_free() has read the
descriptors, or could the pair be wrapped in rcu_read_lock()/
rcu_read_unlock()?

[ ... ]

---

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

In-Reply-To-Subject: bpf: Fix use-after-free of program BTF in mem-alloc destructor
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32741782570

[kernel-patches-review-bot]

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

bpf: Cancel special fields in resizable hashtab on recycle

rhtab_delete_elem() and rhtab_map_update_existing() eagerly call
bpf_obj_free_fields() when an element is deleted or its value is replaced.
This runs kptr destructors in the caller's execution context, which is unsafe
for BPF programs running in NMI context. The commit switches the resizable
hashtab to use bpf_obj_cancel_fields() for NMI-safe fields only, with
referenced kptrs staying attached until eventual element destruction.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index d40cb5dd446c..0df8db27cd8c 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -2864,14 +2864,56 @@ static int rhtab_map_alloc_check(union bpf_attr *attr)
>  	return htab_map_alloc_check(attr);
>  }
>
> -static void rhtab_check_and_free_fields(struct bpf_rhtab *rhtab,
> -					struct rhtab_elem *elem)
> +static void rhtab_cancel_fields(struct bpf_rhtab *rhtab,
> +				struct rhtab_elem *elem)
>  {
>  	if (IS_ERR_OR_NULL(rhtab->map.record))
>  		return;
>
> -	bpf_obj_free_fields(rhtab->map.record,
> -			    rhtab_elem_value(elem, rhtab->map.key_size));
> +	/*
> +	 * Only cancel NMI-safe fields (timer, workqueue, task_work) here.
> +	 * RHASH values can also carry referenced kptrs (and per-cpu kptrs),
> +	 * whose destructors must not run from arbitrary BPF execution
> +	 * contexts (e.g. NMI); leave them attached to the recycled element
> +	 * and let rhtab_mem_dtor() destroy them once the element is
> +	 * eventually freed. This matches the hash map semantics introduced
> +	 * by a3a81d247651 ("bpf: Cancel special fields on map value
> +	 * recycle").
> +	 */
> +	bpf_map_free_internal_structs(&rhtab->map,
> +				      rhtab_elem_value(elem, rhtab->map.key_size));
> +}

This isn't a bug, but could the explanation live in one place with the call
sites pointing at it? The same rationale appears in five spots throughout
this change: here above rhtab_cancel_fields(), above rhtab_init_map_value(),
and in three more comments in the call sites below. That's roughly 33
comment lines against 25 lines of code, and any future adjustments to the
reasoning need to be synced across all five. For comparison, the equivalent
hash map helpers check_and_cancel_fields() and htab_elem_free() carry no
comment at all.

> +
> +/*
> + * Initialize special fields of a freshly allocated rhtab element, but keep
> + * kptr fields untouched. A recycled element may carry a referenced kptr from
> + * its previous life: the delete path only cancels NMI-safe fields (matching
> + * the hash map semantics), so the kptr reference stays owned by the element
> + * until rhtab_mem_dtor() destroys it. Zeroing it here (as
> + * check_and_init_map_value() would) would drop the reference without
> + * releasing it.
> + */
> +static void rhtab_init_map_value(struct bpf_map *map, void *value)
> +{
> +	struct btf_record *rec = map->record;
> +	int i;
> +
> +	if (IS_ERR_OR_NULL(rec))
> +		return;
> +
> +	for (i = 0; i < rec->cnt; i++) {
> +		struct btf_field *field = &rec->fields[i];
> +		void *field_ptr = value + field->offset;
> +
> +		switch (field->type) {
> +		case BPF_KPTR_UNREF:
> +		case BPF_KPTR_REF:
> +		case BPF_KPTR_PERCPU:
> +			continue;
> +		default:
> +			bpf_obj_init_field(field, field_ptr);
> +		}
> +	}
> +}

This isn't a bug, but could this kptr-filtering init be unified with
bpf_obj_init() rather than duplicated here? rhtab_init_map_value()
re-implements the field walk from bpf_obj_init() with a kptr filter in
front of bpf_obj_init_field(). The hash map path this change says it is
matching takes a different route: alloc_htab_elem() does no special-field
initialization at all on a recycled or fresh element, relying on __GFP_ZERO
for fresh memory and on bpf_obj_cancel_fields() having left the cancelled
fields reusable. Would it be simpler to drop the init call entirely the way
alloc_htab_elem() does, or to add the kptr skip as a flag or variant of
bpf_obj_init() so the field walk lives in one place?

[ ... ]

> @@ -3022,10 +3064,11 @@ static long rhtab_map_update_existing(struct bpf_map *map, struct rhtab_elem *el
>  	 * BPF_F_LOCK, matching arraymap semantics.
>  	 *
>  	 * copy_map_value() skips special-field offsets, so old timers/
> -	 * kptrs/etc. still sit in the slot. Cancel them after the copy
> -	 * to match arraymap's update semantics.
> +	 * kptrs/etc. still sit in the slot. Cancel the NMI-safe ones after
> +	 * the copy to match arraymap's update semantics; referenced kptrs
> +	 * stay attached and are destroyed by rhtab_mem_dtor().
>  	 */
> -	rhtab_check_and_free_fields(rhtab, elem);
> +	rhtab_cancel_fields(rhtab, elem);
>  	return 0;
>  }

A subsystem pattern flags this as potentially concerning: after this change,
rhtab_map_update_existing() overwrites an existing map value in place with
copy_map_value() but then only calls rhtab_cancel_fields(), which now calls
bpf_map_free_internal_structs() instead of bpf_obj_free_fields().
bpf_map_free_internal_structs() handles only BPF_TIMER, BPF_WORKQUEUE and
BPF_TASK_WORK. The BPF_KPTR_REF or BPF_KPTR_PERCPU reference held by the
value the user just overwrote is no longer released at update time; it stays
attached to the live element and is only destroyed when the element is
eventually freed to the slab.

Before the change:

rhtab_check_and_free_fields(rhtab, elem);   /* -> bpf_obj_free_fields() */

After:

rhtab_cancel_fields(rhtab, elem);           /* -> bpf_map_free_internal_structs() */

The change appears intentional and mirrors array_map_update_elem(), which
was itself converted from bpf_obj_free_fields() to bpf_obj_cancel_fields()
by a3a81d247651. The reference is not leaked, only its release is deferred:
the element keeps ownership, and rhtab_mem_dtor() runs bpf_obj_free_fields()
before returning memory to the allocator. The kptr slot holds at most one
reference and copy_map_value() never writes it, so repeated updates cannot
accumulate references. Is this deferred-release semantic for RHASH
intentional, matching the hash map and array behavior?

> @@ -3066,7 +3109,14 @@ static long rhtab_map_update_elem(struct bpf_map *map, void *key, void *value, u
>
>  	memcpy(elem->data, key, map->key_size);
>  	copy_map_value(map, rhtab_elem_value(elem, map->key_size), value);
> -	check_and_init_map_value(map, rhtab_elem_value(elem, map->key_size));
> +	/*
> +	 * Initialize special fields of the (possibly recycled) element, but
> +	 * leave kptr slots alone: a recycled element may still own a
> +	 * referenced kptr that rhtab_mem_dtor() will release, so zeroing it
> +	 * here would leak the reference. Fresh memory from the bpf mem
> +	 * allocator is zeroed, so skipping the kptr init is safe there too.
> +	 */
> +	rhtab_init_map_value(map, rhtab_elem_value(elem, map->key_size));

A subsystem pattern flags this as potentially concerning: after this change,
rhtab_map_update_elem() copies the user value into a newly allocated element
with copy_map_value() and the paired check_and_init_map_value() call is
replaced by rhtab_init_map_value(), which explicitly skips BPF_KPTR_UNREF,
BPF_KPTR_REF and BPF_KPTR_PERCPU instead of calling bpf_obj_init_field().
copy_map_value() also skips special-field offsets via bpf_obj_memcpy(), so
no code writes the kptr slots of the new element. bpf_mem_cache_alloc() may
hand back an element recycled from a previous operation whose kptr slots
were left populated, and the element is then published into the rhashtable
under a new key while still carrying the previous occupant's kptr.

Before the change:

check_and_init_map_value(map, rhtab_elem_value(elem, map->key_size));

After:

rhtab_init_map_value(map, rhtab_elem_value(elem, map->key_size));

Skipping BPF_KPTR_REF and BPF_KPTR_PERCPU init appears required for the
accounting to balance: zeroing the slot with bpf_obj_init_field() would drop
the pointer without running the destructor. The equivalent reference path,
alloc_htab_elem() for non-prealloc hash maps, performs no special-field
initialization at all, so htab inherits recycled kptrs the same way. Fresh
memory from the allocator is verified to be zeroed via __GFP_ZERO. Is the
kptr inheritance across recycled elements under new keys the intended
behavior?

---

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

In-Reply-To-Subject: bpf: Cancel special fields in resizable hashtab on recycle
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32741782570

[kernel-patches-review-bot]

commit 81f409ba449d05e3810819754d1b5b1a686cd654
Author: Yuan Chen <chenyuan@kylinos.cn>

selftests/bpf: Test rhtab kptr recycle from NMI context

This test verifies that rhtab cancel semantics match hash maps when
recycling elements with referenced kptrs. The NMI phase checks that
overwrites don't release the old kptr, and the delete/re-insert loop
checks that recycled elements inherit the kptr without leaking the
reference.

> 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..13158d74cbc1
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c
> @@ -0,0 +1,146 @@

[ ... ]

> +	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"))
> +		goto out;

Does this code pass the correct value size to bpf_map_update_elem()?

The map value type is struct val_t which contains an 8-byte kptr and a
u32 magic field, making sizeof(struct val_t) == 16 bytes (with 4 bytes
of trailing padding). The code passes &zero where zero is a single __u64
(8 bytes).

kernel/bpf/syscall.c:map_update_elem() copies the full 16 bytes from
&zero via kvmemdup_bpfptr(), reading 8 bytes past the end of the stack
variable. This has two consequences: the test performs an out-of-bounds
read of its own stack, and the element's magic field gets initialised
with whatever stack garbage followed zero.

The same undersized buffer is used again for the recreate_elem call
further down. Would declaring a correctly sized buffer fix this?

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

This isn't a bug, but the opening /* of a multi-line comment should sit
on its own line per BPF subsystem style. The file's other block comments
already follow that form.

> +		for (i = 0; i < 20 && read_counter(skel, 1) == 0; i++)
> +			usleep(100000);
> +		ASSERT_GT(read_counter(skel, 1), 0, "nmi_update_ran");

Can this assertion fail for environmental reasons on an otherwise correct
kernel?

The perf event is opened with pid == -1 and cpu == 0, so it only samples
CPU 0. PERF_COUNT_HW_CPU_CYCLES does not advance while a core is in a
deep C-state, and the polling thread is not pinned to CPU 0. On an
otherwise idle host the counter can stay at 0 and the test hard-fails
here, even though nothing is wrong with the kernel.

Contrast lru_lock_nmi.c, which makes the same assertion safe by attaching
on every CPU and running busy threads pinned per CPU during the sampling
window.

> +
> +		bpf_link__destroy(skel->links.nmi_update);
> +		skel->links.nmi_update = NULL;
> +		close(pmu_fd);

Does this code close pmu_fd twice?

bpf_program__attach_perf_event() takes ownership of the perf fd on
success: libbpf stores it in bpf_link_perf::perf_event_fd and
bpf_link_perf_detach() closes it. So bpf_link__destroy() above already
closed pmu_fd, and this explicit close() closes an fd number that is no
longer owned by the test.

The file gets the ownership rule right on the failure path: it closes
pmu_fd only because the attach failed. The neighbouring NMI test
lru_lock_nmi.c documents it explicitly at lines 203-206. Today nothing
allocates a descriptor between the destroy and close so the stray close()
just returns EBADF, but a double close in a test that later grows an
intervening open()/socket()/map creation will silently close an unrelated
descriptor.

[ ... ]

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

Does calling test__skip() in a test that defines no subtests make the
whole test report SKIP even though the second phase still runs?

test_progs.c:print_test_result() reads the subtests_cnt and with
subtest_num == 0, a single test__skip() turns the entire test into
'#N rhtab_kptr:SKIP'. On the common BPF CI setup (QEMU without a vPMU)
perf_event_open() always fails, so this test would permanently print SKIP
while silently exercising the delete/re-insert recycle path - a failure
there is still reported, but a pass is indistinguishable from nothing ran.

The neighbouring lru_lock_nmi.c wraps each phase in test__start_subtest()
so that skips and passes are accounted per phase.

A subsystem pattern flags this as potentially concerning: the NMI phase is
skipped with a bare test__skip() that records no reason, so a reader of
the CI log cannot tell whether the PMU was absent, perf_event_paranoid
blocked the open, or something else failed. errno from the failed
perf_event_open() is discarded. The guide warns about tests that skip
silently with no message, though test__skip() takes no reason argument and
this is the dominant idiom across prog_tests/.

[ ... ]

> +	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),
> +				       "recreate_elem"))
> +				goto out;

Same value-size issue here as the earlier create_elem call.

[ ... ]

> 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..fd6bd63cb405
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/rhtab_kptr.c
> @@ -0,0 +1,132 @@

[ ... ]

> +} counters SEC(".maps");
> +
> +/* 0: init ok, 1: nmi update ok, 2: probe xchg non-NULL, 3: probe xchg NULL,
> + * 4: probe saw expected magic value
> + */

This isn't a bug, but the opening /* of a multi-line comment should sit on
its own line. The file's own header comment already uses the correct form,
so this is inconsistent with its own neighbours as well as with the
subsystem style.

> +static __always_inline void bump(u32 idx)
> +{
> +	u64 *v = bpf_map_lookup_elem(&counters, &idx);
> +
> +	if (v)
> +		(*v)++;
> +}

[ ... ]

> +SEC("syscall")
> +int upd_elem(void *ctx)
> +{
> +	struct val_t val = { .magic = 0x52484153 }; /* "RHAS" */
> +	u32 key = 0;
> +
> +	bpf_map_update_elem(&rhtab, &key, &val, BPF_ANY);
> +	return 0;
> +}

Can upd_elem() and del_elem() communicate map operation failures to the
userspace loop?

Both discard the return value of bpf_map_update_elem() and
bpf_map_delete_elem() and unconditionally return 0. The userspace loop
only checks bpf_prog_test_run_opts() and never inspects topts.retval for
these two programs.

rhtab_map_update_elem() has several real failure returns reachable from
this loop - -ENOMEM from bpf_mem_cache_alloc(), -EBUSY when
rhtab->freeing_internal is set, and -E2BIG/insert errors - and
rhtab_map_delete_elem() returns -ENOENT. If any of them fires, the
following probe_elem() returns 1 early, so neither counter 2/3 nor counter
4 is bumped, and the only symptom is the final ASSERT_EQ for
recycle_magic_roundtrip reporting an off-by-N count with no indication of
which operation failed or why.

The sibling programs init_elem()/probe_elem() already use the return value
as a status code, so propagating the map-op error here would turn a silent
count mismatch into a diagnosable failure.

---

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

In-Reply-To-Subject: selftests/bpf: Test rhtab kptr recycle from NMI context
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32741782570

[kernel-patches-review-bot]

commit 71e35bf31c5ee48ec055b230748d10d17de36e16
Author: Yuan Chen <chenyuan@kylinos.cn>

selftests/bpf: Test rhtab special-field combinations

This commit adds tests for BPF_MAP_TYPE_RHASH with various special field
combinations (spin locks, timers, kptrs) to verify correct behavior
through delete/re-insert recycle cycles.

> diff --git a/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c b/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c
> new file mode 100644
> index 000000000000..29de05bcbd4b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c

[ ... ]

> +static void subtest_timer(struct rhtab_fields *skel)
> +{
> +	u64 zero = 0;
> +	u32 key = 0;
> +	int fired, map_fd;
> +
> +	map_fd = bpf_map__fd(skel->maps.tmap);
> +	if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &zero, BPF_ANY),
> +		       "create_elem"))
> +		return;

Does this read past the end of the zero variable?  The tmap value type
is struct timer_val which is 24 bytes (16-byte bpf_timer plus 8-byte
data field), but zero is only 8 bytes.  The syscall copies value_size
bytes from the user pointer in kernel/bpf/syscall.c:

    value_size = bpf_map_value_size(map);
    value = kvmemdup_bpfptr(uvalue, value_size);

so it appears 16 bytes above zero on the stack would be read.  The same
pattern is also used later when recreating the element.

> +
> +	if (!ASSERT_OK(run_prog(skel, "arm_timer"), "arm_timer_first"))
> +		return;
> +	usleep(300000);
> +	if (!ASSERT_GT(skel->bss->timer_fired, 0, "timer_fired_first"))
> +		return;
> +
> +	/* Deleting the element must cancel the timer. */
> +	fired = skel->bss->timer_fired;
> +	if (!ASSERT_OK(bpf_map_delete_elem(map_fd, &key), "delete_elem"))
> +		return;
> +	usleep(300000);
> +	ASSERT_EQ(skel->bss->timer_fired, fired, "timer_cancelled_after_delete");

Can this assertion actually verify timer cancellation?  Looking at
arm_timer() in progs/rhtab_fields.c, it arms the timer with a 50us
expiry (50000ns), and the callback doesn't re-arm.  The test waits
300ms before the delete, and the preceding ASSERT_GT confirms the timer
has already fired.  So by the time bpf_map_delete_elem() runs, the
timer has been inactive for ~300ms.  Whether or not the delete path
cancels the timer field, timer_fired cannot advance during the second
sleep, so the assertion would pass even on a kernel that doesn't cancel
the timer at all.

The commit message says "delete the element and verify the timer is
cancelled" - to actually test cancellation, wouldn't the timer need to
still be pending at delete time?  For example, arming it with a delay
longer than the deletion (say 200ms), deleting immediately, then
sleeping past the original expiry and checking that timer_fired did not
change.

> diff --git a/tools/testing/selftests/bpf/progs/rhtab_fields.c b/tools/testing/selftests/bpf/progs/rhtab_fields.c
> new file mode 100644
> index 000000000000..85335f19f172
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/rhtab_fields.c

[ ... ]

> +struct timer_val {
> +	struct bpf_timer timer;
> +	__u64 data;
> +};

Is the data field used anywhere?  I don't see it read or written in
any of the programs or in the test harness.  Compare struct
lock_kptr_val::magic which exists for a similar purpose (plain bytes
alongside a special field) and is actually exercised by lk_upd/lk_probe
to verify plain bytes round-trip through recycled elements.

The field does affect the value_size (making it 24 bytes instead of
16), which creates a mismatch with prog_tests/rhtab_fields.c where
subtest_timer() seeds the map from an 8-byte local (u64 zero).

> +} counters SEC(".maps");
> +
> +/* 0: lk init ok, 1: lk probe xchg non-NULL, 2: lk probe xchg NULL,
> + * 3: lk probe magic ok, 4: u init ok, 5: u probe ptr non-NULL,
> + * 6: pc init ok, 7: pc probe xchg non-NULL, 8: pc probe xchg NULL
> + */

This isn't a bug, but the comment style here differs from the file
header above.  The BPF subsystem prefers the opening /* on its own
line:

    /*
     * 0: lk init ok, 1: lk probe xchg non-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;
> +
> +int timer_fired;
> +
> +/* Map 1: spin lock + referenced kptr + plain data. */
> +
> +SEC("syscall")
> +int lk_init(void *ctx)
> +{
> +	struct lock_kptr_val *val;
> +	struct task_struct *task, *old;
> +	u32 key = 0;
> +
> +	val = bpf_map_lookup_elem(&lkmap, &key);
> +	if (!val)
> +		return 1;
> +	task = bpf_task_acquire(bpf_get_current_task_btf());
> +	if (!task)
> +		return 2;
> +	old = bpf_kptr_xchg(&val->tsk, task);
> +	if (old)
> +		bpf_task_release(old);
> +	bump(0);
> +	return 0;
> +}
> +
> +SEC("syscall")
> +int lk_del(void *ctx)
> +{
> +	u64 key = 0;
> +
> +	bpf_map_delete_elem(&lkmap, &key);
> +	return 0;
> +}
> +
> +SEC("syscall")
> +int lk_upd(void *ctx)
> +{
> +	struct lock_kptr_val val = { .magic = 0x52484142 };
> +	u32 key = 0;
> +
> +	bpf_map_update_elem(&lkmap, &key, &val, BPF_ANY);
> +	return 0;
> +}
> +
> +SEC("syscall")
> +int lk_probe(void *ctx)
> +{
> +	struct lock_kptr_val *val;
> +	struct task_struct *old;
> +	u32 key = 0;
> +
> +	val = bpf_map_lookup_elem(&lkmap, &key);
> +	if (!val)
> +		return 1;
> +	old = bpf_kptr_xchg(&val->tsk, NULL);
> +	if (old) {
> +		bpf_task_release(old);
> +		bump(1);
> +	} else {
> +		bump(2);
> +	}
> +	if (val->magic == 0x52484142)
> +		bump(3);
> +	return 0;
> +}

A subsystem pattern flags this as potentially concerning: the
lk_init/lk_del/lk_upd/lk_probe quartet appears to re-implement a test
that the immediately preceding commit in this same series (81f409ba449d)
just added to the same directory.  progs/rhtab_kptr.c already covers
the rhtab referenced-kptr recycle path with an identical four-program
template over the same map type.

The shared scaffolding is duplicated as well: bump() and the extern
bpf_task_acquire/bpf_task_release declarations are byte-identical
between the two files.

Given that rhtab_fields.c adds the spin-lock, timer, untrusted-kptr,
and per-cpu-kptr combinations that rhtab_kptr.c doesn't cover, while
rhtab_kptr.c covers the NMI (perf_event) update path that
rhtab_fields.c doesn't, neither file strictly subsumes the other.
Should the two files share a header for the common helpers, or would it
make sense to merge them into one test that covers all field
combinations including the NMI case?

> +
> +/* Map 2: bpf_timer. */
> +
> +static int timer_cb(void *map, void *key, struct timer_val *value)
> +{
> +	timer_fired++;
> +	return 0;
> +}
> +
> +SEC("syscall")
> +int arm_timer(void *ctx)
> +{
> +	struct timer_val *val;
> +	u32 key = 0;
> +
> +	val = bpf_map_lookup_elem(&tmap, &key);
> +	if (!val)
> +		return 1;
> +	/* 1 == CLOCK_MONOTONIC */
> +	if (bpf_timer_init(&val->timer, &tmap, 1))
> +		return 2;
> +	bpf_timer_set_callback(&val->timer, timer_cb);
> +	if (bpf_timer_start(&val->timer, 50000, 0))
> +		return 3;
> +	return 0;
> +}

Does arming a one-shot timer with a 50us expiry make the timer subtest's
delete-cancellation assertion unfalsifiable?  The callback doesn't
re-arm, so the timer fires exactly once.  The consumer in
prog_tests/rhtab_fields.c waits 300ms (6000x the expiry) and confirms
the timer has already fired before calling bpf_map_delete_elem().  The
hrtimer is therefore inactive at delete time, so timer_fired cannot
change during the second sleep regardless of whether the delete path
actually cancels anything.

The existing convention in this directory is to arm a long timer so
it is still pending at delete time - progs/timer_start_delete_race.c
uses bpf_timer_start(&value->timer, 100000000, 0) (100ms) for exactly
this delete-vs-pending-timer scenario, and progs/timer.c uses
1ull << 35 (~34s) as its 'must not fire' expiry.  Would arming with an
expiry longer than the arm-to-delete window make the assertion able to
fail?

[ ... ]

---

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

In-Reply-To-Subject: selftests/bpf: Test rhtab special-field combinations
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32741782570

[kernel-patches-daemon-bpf]

Forwarding comment 5397478581 via email
In-Reply-To: 20260824143621.2098856-3-chenyuan_fl@163.com
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/20260824143621.2098856-3-chenyuan_fl@163.com/

[kernel-patches-daemon-bpf]

Forwarding comment 5397558898 via email
In-Reply-To: 20260824143621.2098856-2-chenyuan_fl@163.com
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/20260824143621.2098856-2-chenyuan_fl@163.com/

[kernel-patches-daemon-bpf]

Forwarding comment 5397565336 via email
In-Reply-To: 20260824143621.2098856-4-chenyuan_fl@163.com
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/20260824143621.2098856-4-chenyuan_fl@163.com/

[kernel-patches-daemon-bpf]

Forwarding comment 5397588881 via email
In-Reply-To: 20260824143621.2098856-5-chenyuan_fl@163.com
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/20260824143621.2098856-5-chenyuan_fl@163.com/

[kernel-patches-daemon-bpf]

Upstream branch: d83fba2
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150975
version: 1

[kernel-patches-daemon-bpf]

Upstream branch: ce36e38
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150975
version: 1

[kernel-patches-daemon-bpf]

Upstream branch: 05ea1b6
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150975
version: 1

[kernel-patches-daemon-bpf]

Upstream branch: 1555de3
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1150975
version: 1