* [PATCH 1/4] bpf: Cancel special fields in resizable hashtab on recycle
2026-08-24 14:36 ` [PATCH bpf-next v2 0/4] " chenyuan_fl
@ 2026-08-24 14:36 ` chenyuan_fl
2026-08-24 15:00 ` sashiko-bot
` (2 more replies)
2026-08-24 14:36 ` [PATCH 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor chenyuan_fl
` (2 subsequent siblings)
3 siblings, 3 replies; 20+ messages in thread
From: chenyuan_fl @ 2026-08-24 14:36 UTC (permalink / raw)
To: bpf
Cc: linux-kernel, linux-kselftest, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Martin KaFai Lau, Song Liu,
Yonghong Song, Jiri Olsa, Emil Tsalapatis, Ihor Solodrai,
Shuah Khan, Nuoqi Gui, Yuan Chen
From: Yuan Chen <chenyuan@kylinos.cn>
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
(e.g. perf_event programs attached to hardware PMU overflows):
referenced kptr destructors may take locks or otherwise cannot run
in NMI.
Commit a3a81d247651 ("bpf: Cancel special fields on map value
recycle") switched the hash map and array recycle paths to
bpf_obj_cancel_fields(), which only cancels NMI-safe fields (timer,
workqueue, task_work), but it missed the resizable hashtab.
rhtab_map_update_existing() even documents the intended "cancel"
semantics while still calling bpf_obj_free_fields().
Fix the resizable hashtab the same way:
* rhtab_delete_elem() and rhtab_map_update_existing() now cancel
only NMI-safe fields. Referenced kptrs stay attached to the
recycled element and are destroyed by rhtab_mem_dtor() once the
element is eventually freed, keeping the reference accounting
balanced.
* rhtab_map_update_elem() initializes the special fields of a
freshly allocated element. The bpf memory allocator may return a
recycled element that still owns a referenced kptr, and
check_and_init_map_value() would zero that slot, dropping the
reference without releasing it. rhtab_init_map_value()
initializes the remaining fields (spin lock, timer, workqueue,
task_work, refcount) but leaves kptr slots untouched, matching
the hash map semantics.
Verified with a selftest: a perf_event (NMI) program overwrites a
rhtab element that holds a referenced task kptr, and a second phase
deletes and re-inserts the element to exercise the recycle path.
Before the patch the NMI update eagerly released the kptr and the
recycle path zeroed the inherited slot; after the patch the kptr is
inherited on both paths and the probe observes it non-NULL.
Fixes: a3a81d247651 ("bpf: Cancel special fields on map value recycle")
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
---
kernel/bpf/hashtab.c | 70 +++++++++++++++++++++++++++++++++++++-------
1 file changed, 60 insertions(+), 10 deletions(-)
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));
+}
+
+/*
+ * 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);
+ }
+ }
}
static void rhtab_mem_dtor(void *obj, void *ctx)
@@ -2963,8 +3005,8 @@ static int rhtab_delete_elem(struct bpf_rhtab *rhtab, struct rhtab_elem *elem, v
rhtab_read_elem_value(&rhtab->map, copy, elem, flags);
check_and_init_map_value(&rhtab->map, copy);
}
- /* Release internal structs: kptr, bpf_timer, task_work, wq */
- rhtab_check_and_free_fields(rhtab, elem);
+ /* Cancel NMI-safe fields; full destruction happens in rhtab_mem_dtor */
+ rhtab_cancel_fields(rhtab, elem);
bpf_mem_cache_free_rcu(&rhtab->ma, elem);
return 0;
}
@@ -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;
}
@@ -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));
/* Prevent deadlock for NMI programs attempting to take bucket lock */
bpf_disable_instrumentation();
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 1/4] bpf: Cancel special fields in resizable hashtab on recycle
2026-08-24 14:36 ` [PATCH 1/4] " chenyuan_fl
@ 2026-08-24 15:00 ` sashiko-bot
2026-08-24 15:42 ` bot+bpf-ci
2026-08-24 16:15 ` Mykyta Yatsenko
2 siblings, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-08-24 15:00 UTC (permalink / raw)
To: chenyuan_fl; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing `bpf_obj_free_fields()` in map updates, directly violating BPF-001 guidelines.
--
--- Patch [1]: [PATCH 1/4] bpf: Cancel special fields in resizable hashtab on recycle ---
commit 6f12e4dc235993c2415d956e168783cb133ed13a
Author: Yuan Chen <chenyuan@kylinos.cn>
bpf: Cancel special fields in resizable hashtab on recycle
This commit updates the resizable hashtab recycle paths to cancel only
NMI-safe fields instead of fully freeing all fields. This is intended to
prevent kptr destructors from running in NMI contexts during map updates or
deletions, delaying their destruction until the element is actually freed.
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
[ ... ]
> @@ -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);
[Severity: Medium]
Does this bypass the required resource cleanup for overwritten map values?
The BPF subsystem guidelines mandate that when overwriting an existing map
value using copy_map_value(), bpf_obj_free_fields() must be called to release
resources held by the old value before or after the copy overwrites them.
Since rhtab_cancel_fields() is used here instead of bpf_obj_free_fields(),
could this leak kptrs or other resources when the old value is overwritten
during a map update?
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260824143621.2098856-1-chenyuan_fl@163.com?part=1
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] bpf: Cancel special fields in resizable hashtab on recycle
2026-08-24 14:36 ` [PATCH 1/4] " chenyuan_fl
2026-08-24 15:00 ` sashiko-bot
@ 2026-08-24 15:42 ` bot+bpf-ci
2026-08-24 16:15 ` Mykyta Yatsenko
2 siblings, 0 replies; 20+ messages in thread
From: bot+bpf-ci @ 2026-08-24 15:42 UTC (permalink / raw)
To: chenyuan_fl, bpf
Cc: linux-kernel, linux-kselftest, ast, daniel, andrii, eddyz87,
memxor, martin.lau, song, yonghong.song, jolsa, emil,
ihor.solodrai, shuah, gnq25, chenyuan, ast, andrii, daniel,
martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 7977 bytes --]
> 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
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32741782570
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 1/4] bpf: Cancel special fields in resizable hashtab on recycle
2026-08-24 14:36 ` [PATCH 1/4] " chenyuan_fl
2026-08-24 15:00 ` sashiko-bot
2026-08-24 15:42 ` bot+bpf-ci
@ 2026-08-24 16:15 ` Mykyta Yatsenko
2 siblings, 0 replies; 20+ messages in thread
From: Mykyta Yatsenko @ 2026-08-24 16:15 UTC (permalink / raw)
To: chenyuan_fl, bpf
Cc: linux-kernel, linux-kselftest, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Martin KaFai Lau, Song Liu,
Yonghong Song, Jiri Olsa, Emil Tsalapatis, Ihor Solodrai,
Shuah Khan, Nuoqi Gui, Yuan Chen
On 8/24/26 3:36 PM, chenyuan_fl@163.com wrote:
> From: Yuan Chen <chenyuan@kylinos.cn>
>
> 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
> (e.g. perf_event programs attached to hardware PMU overflows):
> referenced kptr destructors may take locks or otherwise cannot run
> in NMI.
>
> Commit a3a81d247651 ("bpf: Cancel special fields on map value
> recycle") switched the hash map and array recycle paths to
> bpf_obj_cancel_fields(), which only cancels NMI-safe fields (timer,
> workqueue, task_work), but it missed the resizable hashtab.
> rhtab_map_update_existing() even documents the intended "cancel"
> semantics while still calling bpf_obj_free_fields().
>
> Fix the resizable hashtab the same way:
>
> * rhtab_delete_elem() and rhtab_map_update_existing() now cancel
> only NMI-safe fields. Referenced kptrs stay attached to the
> recycled element and are destroyed by rhtab_mem_dtor() once the
> element is eventually freed, keeping the reference accounting
> balanced.
>
> * rhtab_map_update_elem() initializes the special fields of a
> freshly allocated element. The bpf memory allocator may return a
> recycled element that still owns a referenced kptr, and
> check_and_init_map_value() would zero that slot, dropping the
> reference without releasing it. rhtab_init_map_value()
> initializes the remaining fields (spin lock, timer, workqueue,
> task_work, refcount) but leaves kptr slots untouched, matching
> the hash map semantics.
>
> Verified with a selftest: a perf_event (NMI) program overwrites a
> rhtab element that holds a referenced task kptr, and a second phase
> deletes and re-inserts the element to exercise the recycle path.
> Before the patch the NMI update eagerly released the kptr and the
> recycle path zeroed the inherited slot; after the patch the kptr is
> inherited on both paths and the probe observes it non-NULL.
>
> Fixes: a3a81d247651 ("bpf: Cancel special fields on map value recycle")
> Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
> ---
> kernel/bpf/hashtab.c | 70 +++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 60 insertions(+), 10 deletions(-)
>
> 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));
> +}
> +
> +/*
> + * 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)
Could you please double check if this is needed at all?
I think bpf_map_free_internal_structs() going to reset
special fields to 0, so immediate reuse by __bpf_async_init(),
bpf_task_work_schedule() correctly identifies fresh fields.
> +{
> + 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);
> + }
> + }
> }
>
> static void rhtab_mem_dtor(void *obj, void *ctx)
> @@ -2963,8 +3005,8 @@ static int rhtab_delete_elem(struct bpf_rhtab *rhtab, struct rhtab_elem *elem, v
> rhtab_read_elem_value(&rhtab->map, copy, elem, flags);
> check_and_init_map_value(&rhtab->map, copy);
> }
> - /* Release internal structs: kptr, bpf_timer, task_work, wq */
> - rhtab_check_and_free_fields(rhtab, elem);
> + /* Cancel NMI-safe fields; full destruction happens in rhtab_mem_dtor */
> + rhtab_cancel_fields(rhtab, elem);
Let's directly call bpf_obj_cancel_fields() here and below,
so it is consistent with htab.
> bpf_mem_cache_free_rcu(&rhtab->ma, elem);
> return 0;
> }
> @@ -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;
> }
>
> @@ -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));
>
> /* Prevent deadlock for NMI programs attempting to take bucket lock */
> bpf_disable_instrumentation();
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor
2026-08-24 14:36 ` [PATCH bpf-next v2 0/4] " chenyuan_fl
2026-08-24 14:36 ` [PATCH 1/4] " chenyuan_fl
@ 2026-08-24 14:36 ` chenyuan_fl
2026-08-24 15:17 ` sashiko-bot
2026-08-24 15:42 ` bot+bpf-ci
2026-08-24 14:36 ` [PATCH 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context chenyuan_fl
2026-08-24 14:36 ` [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations chenyuan_fl
3 siblings, 2 replies; 20+ messages in thread
From: chenyuan_fl @ 2026-08-24 14:36 UTC (permalink / raw)
To: bpf
Cc: linux-kernel, linux-kselftest, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Martin KaFai Lau, Song Liu,
Yonghong Song, Jiri Olsa, Emil Tsalapatis, Ihor Solodrai,
Shuah Khan, Nuoqi Gui, Yuan Chen
From: Yuan Chen <chenyuan@kylinos.cn>
bpf_ma_set_dtor() duplicates the map's btf_record for the bpf_mem_alloc
destructor. For kptr fields backed by 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, matching what
btf_parse_fields() did for the map's own record.
The duplicated record, however, is released later from the deferred
bpf_mem_alloc destructor workqueue (free_mem_alloc_deferred), by which
time the program BTF may already have been freed: bpf_map_free() drops
the map's own reference, and the RCU callback can run before the
workqueue. Reading field->kptr.btf in btf_record_free() (via
btf_is_kernel()) is then a use-after-free, detected by KASAN as
"slab-use-after-free in btf_is_kernel" when a map with a MEM_ALLOC kptr
field is destroyed.
Hold a reference on program BTF for the lifetime of the duplicated
record and drop it right before the record is freed. The last btf_put()
only schedules the object for RCU destruction, so btf_record_free() can
still safely read the field descriptors.
The rhtab kptr selftests exercise this path on every map teardown and
triggered the bug under KASAN; with this fix they pass cleanly.
Fixes: 1df97a7453ee ("bpf: Register dtor for freeing special fields")
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
---
kernel/bpf/hashtab.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
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
@@ -493,10 +493,54 @@ static void htab_pcpu_mem_dtor(void *obj, void *ctx)
bpf_obj_free_fields(hrec->record, per_cpu_ptr(pptr, cpu));
}
+/*
+ * 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);
}
@@ -521,6 +565,7 @@ static int bpf_ma_set_dtor(struct bpf_map *map, struct bpf_mem_alloc *ma,
kfree(hrec);
return err;
}
+ htab_record_prog_btf_ref(hrec->record, true);
bpf_mem_alloc_set_dtor(ma, dtor, htab_dtor_ctx_free, hrec);
return 0;
}
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor
2026-08-24 14:36 ` [PATCH 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor chenyuan_fl
@ 2026-08-24 15:17 ` sashiko-bot
2026-08-24 15:42 ` bot+bpf-ci
1 sibling, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-08-24 15:17 UTC (permalink / raw)
To: chenyuan_fl; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] Race condition in `htab_dtor_ctx_free` leading to a use-after-free of the program BTF.
Pre-existing issues:
- [Critical] The patch fails to fix the use-after-free for BPF_LIST_HEAD and BPF_RB_ROOT fields, which also rely on the program BTF.
--
--- Patch [2]: [PATCH 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor ---
commit 67cf446d3408eefa2548db164b549cbc9a9e17a2
Author: Yuan Chen <chenyuan@kylinos.cn>
Subject: bpf: Fix use-after-free of program BTF in mem-alloc destructor
This commit addresses a use-after-free issue of the program BTF in the
bpf_mem_alloc destructor. It holds a reference to the program BTF for the
lifetime of the duplicated record and drops it right before the record is
freed.
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 0df8db27cd8cc..b8df2bc9a9a00 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
[ ... ]
> @@ -493,10 +493,54 @@ static void htab_pcpu_mem_dtor(void *obj, void *ctx)
> bpf_obj_free_fields(hrec->record, per_cpu_ptr(pptr, cpu));
> }
>
> +/*
> + * 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:
[Severity: Critical]
This is a pre-existing issue, but does this switch statement also need to
handle BPF_LIST_HEAD and BPF_RB_ROOT fields?
These field types also rely on the program BTF. When a map containing them
is destroyed, the deferred mem-alloc destructor is scheduled. Because we
don't retain a reference to the program BTF for these types, the program
BTF can be freed when the map drops its reference.
Later, when the deferred destructor calls bpf_obj_free_fields(),
bpf_list_head_free() could dereference the dangling
field->graph_root.value_rec pointer (which points into the freed program
BTF's struct_meta_tab):
kernel/bpf/helpers.c:bpf_list_head_free() {
...
__bpf_obj_drop_impl((char *)pos - field->graph_root.node_offset,
field->graph_root.value_rec, false);
...
}
> + 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);
[Severity: High]
Can this sequence introduce a race condition leading to a use-after-free of
the program BTF?
This code executes in a preemptible workqueue context
(free_mem_alloc_deferred) without RCU read protection. When
htab_record_prog_btf_ref() is called with get=false, it drops the program
BTF reference. If this is the last reference, the BTF object is scheduled
for RCU destruction.
If the thread is preempted immediately after this call and an RCU grace
period elapses, the BTF memory could be freed. Then the next line calls
btf_record_free(), which iterates over the fields and dereferences the
potentially freed kptr.btf inside btf_is_kernel():
kernel/bpf/syscall.c:btf_record_free() {
...
if (btf_is_kernel(rec->fields[i].kptr.btf))
btf_put(rec->fields[i].kptr.btf);
...
}
> kfree(ctx);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260824143621.2098856-1-chenyuan_fl@163.com?part=2
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor
2026-08-24 14:36 ` [PATCH 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor chenyuan_fl
2026-08-24 15:17 ` sashiko-bot
@ 2026-08-24 15:42 ` bot+bpf-ci
1 sibling, 0 replies; 20+ messages in thread
From: bot+bpf-ci @ 2026-08-24 15:42 UTC (permalink / raw)
To: chenyuan_fl, bpf
Cc: linux-kernel, linux-kselftest, ast, daniel, andrii, eddyz87,
memxor, martin.lau, song, yonghong.song, jolsa, emil,
ihor.solodrai, shuah, gnq25, chenyuan, ast, andrii, daniel,
martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 4819 bytes --]
> 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, ©->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
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32741782570
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context
2026-08-24 14:36 ` [PATCH bpf-next v2 0/4] " chenyuan_fl
2026-08-24 14:36 ` [PATCH 1/4] " chenyuan_fl
2026-08-24 14:36 ` [PATCH 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor chenyuan_fl
@ 2026-08-24 14:36 ` chenyuan_fl
2026-08-24 15:28 ` sashiko-bot
2026-08-24 15:42 ` bot+bpf-ci
2026-08-24 14:36 ` [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations chenyuan_fl
3 siblings, 2 replies; 20+ messages in thread
From: chenyuan_fl @ 2026-08-24 14:36 UTC (permalink / raw)
To: bpf
Cc: linux-kernel, linux-kselftest, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Martin KaFai Lau, Song Liu,
Yonghong Song, Jiri Olsa, Emil Tsalapatis, Ihor Solodrai,
Shuah Khan, Nuoqi Gui, Yuan Chen
From: Yuan Chen <chenyuan@kylinos.cn>
A perf_event program running in NMI context overwrites a rhtab
element whose value holds a referenced task kptr. The old kptr must
stay attached to the element (cancel semantics, matching hash maps);
before the rhtab recycle fix the NMI update eagerly released it and
the probe observed NULL. The test asserts the NMI program actually
ran, so the probe result is meaningful.
A second phase deletes and re-inserts the element 2000 times. The
re-insertion may recycle the freed element, which still owns the
kptr; before the fix the alloc path zeroed the inherited slot via
check_and_init_map_value(), leaking the reference, and the probe
never observed a non-NULL pointer. The test requires at least one
recycle to inherit the kptr, and also verifies that plain
(non-special) value bytes still round-trip through the recycled
element on every iteration.
The NMI phase is skipped when no hardware PMU is available.
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
---
.../selftests/bpf/prog_tests/rhtab_kptr.c | 146 ++++++++++++++++++
.../testing/selftests/bpf/progs/rhtab_kptr.c | 132 ++++++++++++++++
2 files changed, 278 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c
create mode 100644 tools/testing/selftests/bpf/progs/rhtab_kptr.c
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 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2026 KylinSoft Co., Ltd. */
+
+#include <linux/perf_event.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+#include <test_progs.h>
+#include "rhtab_kptr.skel.h"
+
+static __u64 read_counter(struct rhtab_kptr *skel, u32 idx)
+{
+ __u64 vals[libbpf_num_possible_cpus()];
+ __u64 sum = 0;
+ int i, err;
+
+ err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.counters), &idx, vals);
+ if (!ASSERT_OK(err, "lookup_counter"))
+ return 0;
+ for (i = 0; i < libbpf_num_possible_cpus(); i++)
+ sum += vals[i];
+ return sum;
+}
+
+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"))
+ 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
+ * 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);
+
+ /*
+ * 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");
+ } else {
+ test__skip();
+ }
+
+ /*
+ * Now exercise the delete/re-insert recycle path. The delete only
+ * cancels NMI-safe fields, so the freed element still owns the kptr.
+ * If the re-insertion recycles that element, the kptr must be
+ * inherited; zeroing it (as check_and_init_map_value() did before
+ * the fix) leaks the reference and probe_elem() observes NULL.
+ * Fresh memory handed out by the allocator is zeroed, so NULL probes
+ * are expected too; only require that the inherited kptr survives at
+ * least one recycle.
+ */
+ nonnull_before = read_counter(skel, 2);
+ 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;
+ topts.retval = 0;
+ err = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.init_elem),
+ &topts);
+ }
+ if (!ASSERT_OK(err, "test_run_init_loop") ||
+ !ASSERT_EQ(topts.retval, 0, "init_loop_ret"))
+ goto out;
+
+ topts.retval = 0;
+ if (!ASSERT_OK(bpf_prog_test_run_opts(bpf_program__fd(skel->progs.del_elem),
+ &topts), "test_run_del"))
+ goto out;
+ topts.retval = 0;
+ if (!ASSERT_OK(bpf_prog_test_run_opts(bpf_program__fd(skel->progs.upd_elem),
+ &topts), "test_run_upd"))
+ goto out;
+ topts.retval = 0;
+ if (!ASSERT_OK(bpf_prog_test_run_opts(bpf_program__fd(skel->progs.probe_elem),
+ &topts), "test_run_probe"))
+ goto out;
+ }
+
+ /*
+ * Plain (non-special) value bytes must survive the recycle path:
+ * every probe must observe the magic value written by upd_elem() in
+ * the same iteration, regardless of whether the element memory was
+ * recycled or freshly allocated.
+ */
+ ASSERT_EQ(read_counter(skel, 4), 2000, "recycle_magic_roundtrip");
+
+ ASSERT_GT(read_counter(skel, 2), nonnull_before, "recycle_xchg_non_null");
+out:
+ rhtab_kptr__destroy(skel);
+}
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 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2026 KylinSoft Co., Ltd. */
+
+/*
+ * Verify that the rhtab update/delete recycle paths do not eagerly destroy
+ * referenced kptrs. rhtab must match the hash map semantics introduced by
+ * commit a3a81d247651 ("bpf: Cancel special fields on map value recycle"):
+ * only NMI-safe fields (timer, workqueue, task_work) are cancelled on
+ * update/delete, while kptrs stay attached to the recycled element until it
+ * is eventually freed.
+ *
+ * Two paths are exercised:
+ * 1. a perf_event (NMI) program overwrites an existing element; without the
+ * fix the NMI update releases the old kptr and probe_elem() observes
+ * NULL;
+ * 2. the element is deleted and re-inserted; the re-insertion may recycle
+ * the freed element, and zeroing the inherited kptr slot (as
+ * check_and_init_map_value() did before the fix) would drop the
+ * reference without releasing it. probe_elem() must observe the
+ * inherited non-NULL pointer, and plain (non-special) value bytes must
+ * still round-trip through the recycled element.
+ */
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+
+char LICENSE[] SEC("license") = "GPL";
+
+struct val_t {
+ struct task_struct __kptr * tsk;
+ __u32 magic;
+};
+
+struct {
+ __uint(type, BPF_MAP_TYPE_RHASH);
+ __uint(max_entries, 16);
+ __uint(map_flags, BPF_F_NO_PREALLOC);
+ __type(key, __u32);
+ __type(value, struct val_t);
+} rhtab SEC(".maps");
+
+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,
+ * 4: probe saw expected magic value
+ */
+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;
+}
+
+SEC("syscall")
+int init_elem(void *ctx)
+{
+ struct val_t *val;
+ struct task_struct *task, *old;
+ u32 key = 0;
+
+ val = bpf_map_lookup_elem(&rhtab, &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 del_elem(void *ctx)
+{
+ u32 key = 0;
+
+ bpf_map_delete_elem(&rhtab, &key);
+ return 0;
+}
+
+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;
+}
+
+SEC("syscall")
+int probe_elem(void *ctx)
+{
+ struct val_t *val;
+ struct task_struct *old;
+ u32 key = 0;
+
+ val = bpf_map_lookup_elem(&rhtab, &key);
+ if (!val)
+ return 1;
+ old = bpf_kptr_xchg(&val->tsk, NULL);
+ if (old) {
+ bpf_task_release(old);
+ bump(2);
+ } else {
+ bump(3);
+ }
+ if (val->magic == 0x52484153)
+ bump(4);
+ return 0;
+}
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context
2026-08-24 14:36 ` [PATCH 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context chenyuan_fl
@ 2026-08-24 15:28 ` sashiko-bot
2026-08-24 15:42 ` bot+bpf-ci
1 sibling, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-08-24 15:28 UTC (permalink / raw)
To: chenyuan_fl; +Cc: bpf
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 <chenyuan@kylinos.cn>
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/20260824143621.2098856-1-chenyuan_fl@163.com?part=3
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context
2026-08-24 14:36 ` [PATCH 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context chenyuan_fl
2026-08-24 15:28 ` sashiko-bot
@ 2026-08-24 15:42 ` bot+bpf-ci
1 sibling, 0 replies; 20+ messages in thread
From: bot+bpf-ci @ 2026-08-24 15:42 UTC (permalink / raw)
To: chenyuan_fl, bpf
Cc: linux-kernel, linux-kselftest, ast, daniel, andrii, eddyz87,
memxor, martin.lau, song, yonghong.song, jolsa, emil,
ihor.solodrai, shuah, gnq25, chenyuan, ast, andrii, daniel,
martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 7887 bytes --]
> 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
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32741782570
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations
2026-08-24 14:36 ` [PATCH bpf-next v2 0/4] " chenyuan_fl
` (2 preceding siblings ...)
2026-08-24 14:36 ` [PATCH 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context chenyuan_fl
@ 2026-08-24 14:36 ` chenyuan_fl
2026-08-24 15:40 ` sashiko-bot
2026-08-24 15:42 ` bot+bpf-ci
3 siblings, 2 replies; 20+ messages in thread
From: chenyuan_fl @ 2026-08-24 14:36 UTC (permalink / raw)
To: bpf
Cc: linux-kernel, linux-kselftest, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Martin KaFai Lau, Song Liu,
Yonghong Song, Jiri Olsa, Emil Tsalapatis, Ihor Solodrai,
Shuah Khan, Nuoqi Gui, Yuan Chen
From: Yuan Chen <chenyuan@kylinos.cn>
BPF_MAP_TYPE_RHASH allows spin locks, timers, workqueues, task_work,
kptrs (referenced, untrusted, per-cpu) and refcounts in map values.
The recycle fix only changes kptr slot handling, so verify each field
combination end to end:
* lock_kptr: bpf_spin_lock + referenced kptr + plain data in one
value. BPF_F_LOCK syscall updates/lookups must work before and
after many delete/re-insert recycle cycles, the referenced kptr
must be inherited on recycled elements (zeroing it would leak the
reference), and the plain bytes must round-trip every iteration.
* timer: arm a bpf_timer and verify it fires, delete the element and
verify the timer is cancelled, then re-insert (possibly recycling
the freed element) and arm a fresh timer again.
* kptr_untrusted: the untrusted kptr must survive the recycle like a
referenced one.
* kptr_percpu: the per-cpu kptr reference must survive the recycle
(zeroing it would leak the reference).
On the unfixed kernel the three kptr subtests fail at the recycle
assertions while the lock and timer paths still pass, isolating the
behavior change to kptr slots only.
Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>
---
.../selftests/bpf/prog_tests/rhtab_fields.c | 213 ++++++++++++
.../selftests/bpf/progs/rhtab_fields.c | 305 ++++++++++++++++++
2 files changed, 518 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/rhtab_fields.c
create mode 100644 tools/testing/selftests/bpf/progs/rhtab_fields.c
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
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2026 KylinSoft Co., Ltd. */
+
+#include <unistd.h>
+#include <test_progs.h>
+#include "rhtab_fields.skel.h"
+
+#define RECYCLE_LOOPS 2000
+#define LK_MAGIC 0x52484142
+
+/* Userspace view of the BPF value types (layouts must match the progs). */
+struct lock_kptr_val_user {
+ __u32 lock;
+ __u32 pad;
+ __u64 tsk;
+ __u32 magic;
+ __u32 pad2;
+};
+
+static __u64 read_counter(struct rhtab_fields *skel, u32 idx)
+{
+ __u64 vals[libbpf_num_possible_cpus()];
+ __u64 sum = 0;
+ int i, err;
+
+ err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.counters), &idx, vals);
+ if (!ASSERT_OK(err, "lookup_counter"))
+ return 0;
+ for (i = 0; i < libbpf_num_possible_cpus(); i++)
+ sum += vals[i];
+ return sum;
+}
+
+/* Returns the program retval; asserts the test_run itself succeeded. */
+static int run_prog(struct rhtab_fields *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);
+ if (!ASSERT_OK(err, name))
+ return -1;
+ return topts.retval;
+}
+
+static void recycle_loop(struct rhtab_fields *skel, int map_fd,
+ const char *init, const char *del,
+ const char *upd, const char *probe)
+{
+ u64 zero = 0;
+ u32 key = 0;
+ int i;
+
+ for (i = 0; i < RECYCLE_LOOPS; i++) {
+ if (run_prog(skel, init) != 0) {
+ /* Element may be gone; recreate and retry once. */
+ if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &zero, BPF_ANY),
+ "recreate_elem"))
+ return;
+ if (!ASSERT_OK(run_prog(skel, init), init))
+ return;
+ }
+ if (!ASSERT_OK(run_prog(skel, del), del))
+ return;
+ if (!ASSERT_OK(run_prog(skel, upd), upd))
+ return;
+ if (!ASSERT_OK(run_prog(skel, probe), probe))
+ return;
+ }
+}
+
+static void subtest_lock_kptr(struct rhtab_fields *skel)
+{
+ struct lock_kptr_val_user val = {};
+ struct lock_kptr_val_user out = {};
+ u64 nonnull_before;
+ u32 key = 0;
+ int map_fd;
+
+ map_fd = bpf_map__fd(skel->maps.lkmap);
+
+ if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &val, BPF_ANY),
+ "create_elem"))
+ return;
+
+ /* Spin lock must be usable from the syscall path (BPF_F_LOCK). */
+ val.magic = LK_MAGIC;
+ if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &val, BPF_F_LOCK),
+ "locked_update"))
+ return;
+ if (!ASSERT_OK(bpf_map_lookup_elem_flags(map_fd, &key, &out, BPF_F_LOCK),
+ "locked_lookup"))
+ return;
+ ASSERT_EQ(out.magic, LK_MAGIC, "locked_lookup_magic");
+
+ /*
+ * Delete/re-insert recycle cycles: the referenced kptr must be
+ * inherited on recycled elements (zeroing it would leak the
+ * reference) and the plain magic bytes must round-trip every time.
+ */
+ nonnull_before = read_counter(skel, 1);
+ recycle_loop(skel, map_fd, "lk_init", "lk_del", "lk_upd", "lk_probe");
+ ASSERT_GT(read_counter(skel, 1), nonnull_before, "recycle_xchg_non_null");
+ ASSERT_EQ(read_counter(skel, 3), RECYCLE_LOOPS, "recycle_magic_roundtrip");
+
+ /* The spin lock must still work after many recycles. */
+ val.magic = LK_MAGIC + 1;
+ if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &val, BPF_F_LOCK),
+ "post_recycle_locked_update"))
+ return;
+ memset(&out, 0, sizeof(out));
+ if (!ASSERT_OK(bpf_map_lookup_elem_flags(map_fd, &key, &out, BPF_F_LOCK),
+ "post_recycle_locked_lookup"))
+ return;
+ ASSERT_EQ(out.magic, LK_MAGIC + 1, "post_recycle_locked_magic");
+}
+
+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;
+
+ 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");
+
+ /*
+ * Re-insert (may recycle the freed element): the timer field must be
+ * re-initialized so a fresh timer can be armed again.
+ */
+ if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &zero, BPF_ANY),
+ "recreate_elem"))
+ return;
+ if (!ASSERT_OK(run_prog(skel, "arm_timer"), "arm_timer_second"))
+ return;
+ usleep(300000);
+ ASSERT_GT(skel->bss->timer_fired, fired, "timer_fired_second");
+}
+
+static void subtest_kptr_untrusted(struct rhtab_fields *skel)
+{
+ u64 nonnull_before;
+ u64 zero = 0;
+ u32 key = 0;
+ int map_fd;
+
+ map_fd = bpf_map__fd(skel->maps.umap);
+ if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &zero, BPF_ANY),
+ "create_elem"))
+ return;
+
+ /* The untrusted kptr must survive the recycle like a referenced one. */
+ nonnull_before = read_counter(skel, 5);
+ recycle_loop(skel, map_fd, "u_init", "u_del", "u_upd", "u_probe");
+ ASSERT_GT(read_counter(skel, 5), nonnull_before, "recycle_unref_non_null");
+}
+
+static void subtest_kptr_percpu(struct rhtab_fields *skel)
+{
+ u64 nonnull_before;
+ u64 zero = 0;
+ u32 key = 0;
+ int map_fd;
+
+ map_fd = bpf_map__fd(skel->maps.pcmap);
+ if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &zero, BPF_ANY),
+ "create_elem"))
+ return;
+
+ /* The per-cpu kptr reference must survive the recycle (no leak). */
+ nonnull_before = read_counter(skel, 7);
+ recycle_loop(skel, map_fd, "pc_init", "pc_del", "pc_upd", "pc_probe");
+ ASSERT_GT(read_counter(skel, 7), nonnull_before, "recycle_pcpu_non_null");
+}
+
+void test_rhtab_fields(void)
+{
+ struct rhtab_fields *skel;
+
+ skel = rhtab_fields__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "open_and_load"))
+ return;
+
+ if (test__start_subtest("lock_kptr"))
+ subtest_lock_kptr(skel);
+ if (test__start_subtest("timer"))
+ subtest_timer(skel);
+ if (test__start_subtest("kptr_untrusted"))
+ subtest_kptr_untrusted(skel);
+ if (test__start_subtest("kptr_percpu"))
+ subtest_kptr_percpu(skel);
+
+ rhtab_fields__destroy(skel);
+}
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
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2026 KylinSoft Co., Ltd. */
+
+/*
+ * Combined special-field tests for BPF_MAP_TYPE_RHASH. Each map carries a
+ * different field combination and is exercised through delete/re-insert
+ * cycles so the bpf memory allocator recycles element memory:
+ *
+ * 1. lkmap: bpf_spin_lock + referenced kptr + plain data in one value.
+ * After every recycle the spin lock must still be usable (initialized by
+ * the alloc path), the referenced kptr must be inherited instead of
+ * zeroed (zeroing would leak the reference), and the plain bytes must
+ * round-trip.
+ * 2. tmap: bpf_timer. The delete path must cancel the timer, and a recycled
+ * element must be able to arm a fresh timer again.
+ * 3. umap: untrusted (unreferenced) kptr. The inherited pointer must be
+ * preserved on recycle, matching hash map behavior.
+ * 4. pcmap: per-cpu kptr. Like the referenced kptr, the per-cpu reference
+ * must not be dropped on recycle.
+ */
+
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_experimental.h"
+
+char LICENSE[] SEC("license") = "GPL";
+
+struct lock_kptr_val {
+ struct bpf_spin_lock lock;
+ struct task_struct __kptr * tsk;
+ __u32 magic;
+};
+
+struct {
+ __uint(type, BPF_MAP_TYPE_RHASH);
+ __uint(max_entries, 16);
+ __uint(map_flags, BPF_F_NO_PREALLOC);
+ __type(key, __u32);
+ __type(value, struct lock_kptr_val);
+} lkmap SEC(".maps");
+
+struct timer_val {
+ struct bpf_timer timer;
+ __u64 data;
+};
+
+struct {
+ __uint(type, BPF_MAP_TYPE_RHASH);
+ __uint(max_entries, 16);
+ __uint(map_flags, BPF_F_NO_PREALLOC);
+ __type(key, __u32);
+ __type(value, struct timer_val);
+} tmap SEC(".maps");
+
+struct unref_val {
+ struct task_struct __kptr_untrusted * tsk;
+};
+
+struct {
+ __uint(type, BPF_MAP_TYPE_RHASH);
+ __uint(max_entries, 16);
+ __uint(map_flags, BPF_F_NO_PREALLOC);
+ __type(key, __u32);
+ __type(value, struct unref_val);
+} umap SEC(".maps");
+
+struct pcval {
+ __u64 v;
+};
+
+struct pcpu_val {
+ struct pcval __percpu_kptr * pc;
+};
+
+struct {
+ __uint(type, BPF_MAP_TYPE_RHASH);
+ __uint(max_entries, 16);
+ __uint(map_flags, BPF_F_NO_PREALLOC);
+ __type(key, __u32);
+ __type(value, struct pcpu_val);
+} pcmap SEC(".maps");
+
+struct {
+ __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
+ __uint(max_entries, 9);
+ __type(key, __u32);
+ __type(value, __u64);
+} 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
+ */
+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)
+{
+ u32 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;
+}
+
+/* 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;
+}
+
+/* Map 3: untrusted kptr. */
+
+SEC("syscall")
+int u_init(void *ctx)
+{
+ struct unref_val *val;
+ u32 key = 0;
+
+ val = bpf_map_lookup_elem(&umap, &key);
+ if (!val)
+ return 1;
+ val->tsk = bpf_get_current_task_btf();
+ bump(4);
+ return 0;
+}
+
+SEC("syscall")
+int u_del(void *ctx)
+{
+ u32 key = 0;
+
+ bpf_map_delete_elem(&umap, &key);
+ return 0;
+}
+
+SEC("syscall")
+int u_upd(void *ctx)
+{
+ struct unref_val val = {};
+ u32 key = 0;
+
+ bpf_map_update_elem(&umap, &key, &val, BPF_ANY);
+ return 0;
+}
+
+SEC("syscall")
+int u_probe(void *ctx)
+{
+ struct unref_val *val;
+ u32 key = 0;
+
+ val = bpf_map_lookup_elem(&umap, &key);
+ if (!val)
+ return 1;
+ if (val->tsk)
+ bump(5);
+ val->tsk = NULL;
+ return 0;
+}
+
+/* Map 4: per-cpu kptr. */
+
+SEC("syscall")
+int pc_init(void *ctx)
+{
+ struct pcpu_val *val;
+ struct pcval *p, *old;
+ u32 key = 0;
+
+ val = bpf_map_lookup_elem(&pcmap, &key);
+ if (!val)
+ return 1;
+ p = bpf_percpu_obj_new(struct pcval);
+ if (!p)
+ return 2;
+ old = bpf_kptr_xchg(&val->pc, p);
+ if (old)
+ bpf_percpu_obj_drop(old);
+ bump(6);
+ return 0;
+}
+
+SEC("syscall")
+int pc_del(void *ctx)
+{
+ u32 key = 0;
+
+ bpf_map_delete_elem(&pcmap, &key);
+ return 0;
+}
+
+SEC("syscall")
+int pc_upd(void *ctx)
+{
+ struct pcpu_val val = {};
+ u32 key = 0;
+
+ bpf_map_update_elem(&pcmap, &key, &val, BPF_ANY);
+ return 0;
+}
+
+SEC("syscall")
+int pc_probe(void *ctx)
+{
+ struct pcpu_val *val;
+ struct pcval *old;
+ u32 key = 0;
+
+ val = bpf_map_lookup_elem(&pcmap, &key);
+ if (!val)
+ return 1;
+ old = bpf_kptr_xchg(&val->pc, NULL);
+ if (old) {
+ bpf_percpu_obj_drop(old);
+ bump(7);
+ } else {
+ bump(8);
+ }
+ return 0;
+}
--
2.54.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations
2026-08-24 14:36 ` [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations chenyuan_fl
@ 2026-08-24 15:40 ` sashiko-bot
2026-08-24 15:42 ` bot+bpf-ci
1 sibling, 0 replies; 20+ messages in thread
From: sashiko-bot @ 2026-08-24 15:40 UTC (permalink / raw)
To: chenyuan_fl; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Passing an undersized buffer to `bpf_map_update_elem` causes an out-of-bounds read on the userspace stack.
- [Medium] Missing file descriptor validation after manual BPF program lookup.
- [Medium] The timer cancellation test is ineffective because it attempts to cancel a one-shot timer that has already expired.
- [Low] Multi-line comment does not follow the mandatory BPF subsystem style.
--
--- Patch [4]: [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations ---
commit 97a820b266ff0381e757c9ee521aee4aca7caef2
Author: Yuan Chen <chenyuan@kylinos.cn>
selftests/bpf: Test rhtab special-field combinations
This commit adds selftests for BPF_MAP_TYPE_RHASH special-field combinations,
verifying the recycling of memory for various field types like spin locks,
timers, and kptrs.
> 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 0000000000000..29de05bcbd4b0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c
[ ... ]
> @@ -28,34 +28,45 @@ static int run_prog(struct rhtab_fields *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: Medium]
Is it necessary to validate the file descriptor returned by bpf_program__fd()
before using it? According to BPF selftest patterns, file descriptors from
manual lookups should be verified (>= 0) before use, as they can return a
negative value if the program is invalid.
> if (!ASSERT_OK(err, name))
> return -1;
> return topts.retval;
> }
>
> static void recycle_loop(struct rhtab_fields *skel, int map_fd,
> const char *init, const char *del,
> const char *upd, const char *probe)
> {
> u64 zero = 0;
> u32 key = 0;
> int i;
>
> for (i = 0; i < RECYCLE_LOOPS; i++) {
> if (run_prog(skel, init) != 0) {
> /* Element may be gone; recreate and retry once. */
> if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &zero, BPF_ANY),
[Severity: Medium]
Could this result in an out-of-bounds read on the userspace stack? The zero
variable is 8 bytes, but this function is called for maps like lkmap which
expects a 24-byte value structure. bpf_map_update_elem() will read
map->value_size bytes from this pointer.
> "recreate_elem"))
> return;
[ ... ]
> @@ -119,25 +130,24 @@ 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),
[Severity: Medium]
Could this also lead to an out-of-bounds read? The tmap map value
(struct timer_val) is 24 bytes in size, but it's being updated from the
8-byte zero variable.
> "create_elem"))
> return;
>
> 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");
[Severity: Medium]
Does this assertion actually test the cancellation of the timer? Since
arm_timer() sets a 50us one-shot timer and this code sleeps for 300ms
before calling bpf_map_delete_elem(), the timer has already fired by the
time the element is deleted.
[ ... ]
> 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 0000000000000..85335f19f1722
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/rhtab_fields.c
[ ... ]
> @@ -89,20 +89,20 @@ struct {
> __uint(max_entries, 9);
> __type(key, __u32);
> __type(value, __u64);
> } 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
> + */
[Severity: Low]
This isn't a bug, but does this multi-line comment format violate the
BPF subsystem style? BPF subsystem multi-line comments should have the opening
/* on its own line.
> static __always_inline void bump(u32 idx)
> {
> u64 *v = bpf_map_lookup_elem(&counters, &idx);
>
> if (v)
> (*v)++;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260824143621.2098856-1-chenyuan_fl@163.com?part=4
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations
2026-08-24 14:36 ` [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations chenyuan_fl
2026-08-24 15:40 ` sashiko-bot
@ 2026-08-24 15:42 ` bot+bpf-ci
1 sibling, 0 replies; 20+ messages in thread
From: bot+bpf-ci @ 2026-08-24 15:42 UTC (permalink / raw)
To: chenyuan_fl, bpf
Cc: linux-kernel, linux-kselftest, ast, daniel, andrii, eddyz87,
memxor, martin.lau, song, yonghong.song, jolsa, emil,
ihor.solodrai, shuah, gnq25, chenyuan, ast, andrii, daniel,
martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 7951 bytes --]
> 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
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32741782570
^ permalink raw reply [flat|nested] 20+ messages in thread