* [PATCH bpf-next v2 0/4] bpf: Cancel special fields in resizable hashtab on recycle [not found] <DKM6K95EN9OF.3O9XNYWVLYHDE@gmail.com> @ 2026-08-24 14:36 ` chenyuan_fl 2026-08-24 14:36 ` [PATCH 1/4] " chenyuan_fl ` (3 more replies) 0 siblings, 4 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> Posted in reply to Kumar Kartikeya Dwivedi's review [1] of Nuoqi Gui's overlapping series [2]: switching rhtab to bpf_obj_cancel_fields() alone leaks referenced kptrs, because rhtab_map_update_elem() zeroes the kptr slot of a recycled element via check_and_init_map_value() before the allocator destructor can release it. The resizable hashtab still eagerly calls bpf_obj_free_fields() on element delete/replace, which runs kptr destructors from the caller's execution context (unsafe in NMI). Patch 1 applies the cancel semantics like hash/array maps (a3a81d247651) and fixes the recycle-path kptr leak with rhtab_init_map_value(); patch 2 fixes a program-BTF use-after-free in the mem-alloc destructor found while testing; patches 3-4 add regression tests (NMI update, and per-field delete/re-insert cycles). Verified in QEMU (KASAN, current bpf-next): rhtab_kptr, rhtab_fields (4/4) and rhash pass with the fix; on the unfixed kernel the kptr subtests fail at the recycle assertions and the program-BTF UAF reproduces. Changes since v1: fix the recycle-path kptr leak, extend the selftests with delete/re-insert recycle coverage, add per-field combination tests, fix the program-BTF UAF (patch 2). [1] https://lore.kernel.org/bpf/DKEVG8ZVJDDQ.2G40FTOIZKU57@gmail.com/ [2] https://lore.kernel.org/bpf/20260726-f01-23-rhash-cancel-bpf-next-v1-0-6e5e1131d885@mails.tsinghua.edu.cn/ Yuan Chen (4): bpf: Cancel special fields in resizable hashtab on recycle bpf: Fix use-after-free of program BTF in mem-alloc destructor selftests/bpf: Test rhtab kptr recycle from NMI context selftests/bpf: Test rhtab special-field combinations kernel/bpf/hashtab.c | 115 ++++++- .../selftests/bpf/prog_tests/rhtab_fields.c | 213 ++++++++++++ .../selftests/bpf/prog_tests/rhtab_kptr.c | 146 +++++++++ .../selftests/bpf/progs/rhtab_fields.c | 305 ++++++++++++++++++ .../testing/selftests/bpf/progs/rhtab_kptr.c | 132 ++++++++ 5 files changed, 901 insertions(+), 10 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/rhtab_fields.c create mode 100644 tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c create mode 100644 tools/testing/selftests/bpf/progs/rhtab_fields.c create mode 100644 tools/testing/selftests/bpf/progs/rhtab_kptr.c -- 2.54.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/4] bpf: Cancel special fields in resizable hashtab on recycle 2026-08-24 14:36 ` [PATCH bpf-next v2 0/4] bpf: Cancel special fields in resizable hashtab on recycle chenyuan_fl @ 2026-08-24 14:36 ` chenyuan_fl 2026-08-24 15:42 ` bot+bpf-ci 2026-08-24 16:15 ` Mykyta Yatsenko 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, 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> 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:42 ` bot+bpf-ci 2026-08-24 16:15 ` Mykyta Yatsenko 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: 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:42 ` bot+bpf-ci @ 2026-08-24 16:15 ` Mykyta Yatsenko 2026-09-01 6:28 ` [PATCH bpf-next v3 0/4] " chenyuan_fl 1 sibling, 1 reply; 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 bpf-next v3 0/4] bpf: Cancel special fields in resizable hashtab on recycle 2026-08-24 16:15 ` Mykyta Yatsenko @ 2026-09-01 6:28 ` chenyuan_fl 2026-09-01 6:28 ` [PATCH bpf-next v3 1/4] " chenyuan_fl ` (3 more replies) 0 siblings, 4 replies; 20+ messages in thread From: chenyuan_fl @ 2026-09-01 6:28 UTC (permalink / raw) To: bpf Cc: linux-kernel, linux-kselftest, Mykyta Yatsenko, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, Martin KaFai Lau, Emil Tsalapatis, Ihor Solodrai, Shuah Khan, Nuoqi Gui, Yuan Chen From: Yuan Chen <chenyuan@kylinos.cn> v3 addresses Mykyta's review of v2 [3]. Both comments concern patch 1: 1. "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." -> Correct: bpf_obj_cancel_fields() resets the timer/workqueue/ task_work slots in place (xchg to NULL in bpf_async_cancel_and_free() and bpf_task_work_cancel_and_free()), so no re-initialization is needed on reuse. rhtab_init_map_value() is dropped entirely; the element alloc path now matches htab's non-prealloc path (alloc_htab_elem()), which also performs no explicit init. 2. "Let's directly call bpf_obj_cancel_fields() here and below, so it is consistent with htab." -> Done: the rhtab_cancel_fields() wrapper is removed and rhtab_delete_elem()/rhtab_map_update_existing() call bpf_obj_cancel_fields() directly. The resizable hashtab still eagerly calls bpf_obj_free_fields() on element delete/replace, which runs kptr destructors from the caller's execution context (unsafe in NMI). This follows the existing discussion on RHash special-field recycling (Nuoqi Gui's series [2] and the review [1]): patch 1 applies the cancel semantics like hash/array maps (a3a81d247651); patch 2 fixes a program-BTF use-after-free in the mem-alloc destructor found while testing; patches 3-4 add regression tests (NMI update, and per-field delete/re-insert cycles). Changes since v2 (all from Mykyta's review [3]): * Drop rhtab_init_map_value(): bpf_obj_cancel_fields() resets the timer/workqueue/task_work slots to zero on delete, fresh elements come zeroed from the bpf mem allocator, and kptr slots must stay untouched, so no re-initialization is needed (matching htab's non-prealloc path). * Call bpf_obj_cancel_fields() directly instead of a rhtab-specific wrapper, consistent with htab. [1] https://lore.kernel.org/bpf/DKEVG8ZVJDDQ.2G40FTOIZKU57@gmail.com/ [2] https://lore.kernel.org/bpf/20260726-f01-23-rhash-cancel-bpf-next-v1-0-6e5e1131d885@mails.tsinghua.edu.cn/ [3] https://lore.kernel.org/bpf/0560a24d-2cf9-4e5b-aa61-580af1e56de1@gmail.com/ Yuan Chen (4): bpf: Cancel special fields in resizable hashtab on recycle bpf: Fix use-after-free of program BTF in mem-alloc destructor selftests/bpf: Test rhtab kptr recycle from NMI context selftests/bpf: Test rhtab special-field combinations kernel/bpf/hashtab.c | 98 +++++- .../selftests/bpf/prog_tests/rhtab_fields.c | 337 ++++++++++++++++++ .../testing/selftests/bpf/prog_tests/rhtab_kptr.c | 184 ++++++++++ tools/testing/selftests/bpf/progs/rhtab_fields.c | 378 +++++++++++++++++++++ tools/testing/selftests/bpf/progs/rhtab_kptr.c | 146 ++++++++ tools/testing/selftests/bpf/rhtab_fields_common.h | 19 ++ tools/testing/selftests/bpf/rhtab_kptr_common.h | 6 + 7 files changed, 1152 insertions(+), 16 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/rhtab_fields.c create mode 100644 tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c create mode 100644 tools/testing/selftests/bpf/progs/rhtab_fields.c create mode 100644 tools/testing/selftests/bpf/progs/rhtab_kptr.c create mode 100644 tools/testing/selftests/bpf/rhtab_fields_common.h create mode 100644 tools/testing/selftests/bpf/rhtab_kptr_common.h -- 2.54.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH bpf-next v3 1/4] bpf: Cancel special fields in resizable hashtab on recycle 2026-09-01 6:28 ` [PATCH bpf-next v3 0/4] " chenyuan_fl @ 2026-09-01 6:28 ` chenyuan_fl 2026-09-01 7:37 ` bot+bpf-ci 2026-09-01 16:57 ` Mykyta Yatsenko 2026-09-01 6:28 ` [PATCH bpf-next v3 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor chenyuan_fl ` (2 subsequent siblings) 3 siblings, 2 replies; 20+ messages in thread From: chenyuan_fl @ 2026-09-01 6:28 UTC (permalink / raw) To: bpf Cc: linux-kernel, linux-kselftest, Mykyta Yatsenko, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, Martin KaFai Lau, 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: call bpf_obj_cancel_fields() on delete and in-place update, matching htab. 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. No special-field initialization is added to the element alloc path: fresh elements come zeroed from the bpf mem allocator, recycled elements already had their timer/workqueue/task_work slots reset by bpf_obj_cancel_fields(), and check_and_init_map_value() would zero the kptr slot of a recycled element, dropping the reference without releasing it. 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") Suggested-by: Mykyta Yatsenko <mykyta.yatsenko5@gmail.com> Signed-off-by: Yuan Chen <chenyuan@kylinos.cn> --- kernel/bpf/hashtab.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c index d40cb5dd446c..aaedda3730f3 100644 --- a/kernel/bpf/hashtab.c +++ b/kernel/bpf/hashtab.c @@ -2864,16 +2864,6 @@ 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) -{ - if (IS_ERR_OR_NULL(rhtab->map.record)) - return; - - bpf_obj_free_fields(rhtab->map.record, - rhtab_elem_value(elem, rhtab->map.key_size)); -} - static void rhtab_mem_dtor(void *obj, void *ctx) { struct htab_btf_record *hrec = ctx; @@ -2963,8 +2953,9 @@ 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 */ + bpf_obj_cancel_fields(&rhtab->map, + rhtab_elem_value(elem, rhtab->map.key_size)); bpf_mem_cache_free_rcu(&rhtab->ma, elem); return 0; } @@ -3022,10 +3013,12 @@ 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); + bpf_obj_cancel_fields(&rhtab->map, + rhtab_elem_value(elem, rhtab->map.key_size)); return 0; } @@ -3066,7 +3059,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)); + /* + * No explicit special-field initialization, matching the hash map's + * non-prealloc path: fresh elements come zeroed from the bpf mem + * allocator, and recycled elements had their timer/workqueue/task_work + * slots reset by bpf_obj_cancel_fields() on delete. kptr slots are + * left untouched so a recycled element keeps owning its reference + * until rhtab_mem_dtor() releases it. + */ /* 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 bpf-next v3 1/4] bpf: Cancel special fields in resizable hashtab on recycle 2026-09-01 6:28 ` [PATCH bpf-next v3 1/4] " chenyuan_fl @ 2026-09-01 7:37 ` bot+bpf-ci 2026-09-01 16:57 ` Mykyta Yatsenko 1 sibling, 0 replies; 20+ messages in thread From: bot+bpf-ci @ 2026-09-01 7:37 UTC (permalink / raw) To: chenyuan_fl, bpf Cc: linux-kernel, linux-kselftest, mykyta.yatsenko5, ast, daniel, andrii, eddyz87, memxor, martin.lau, emil, ihor.solodrai, shuah, gnq25, chenyuan, ast, andrii, daniel, martin.lau, eddyz87, yonghong.song, mason, ihor.solodrai [-- Attachment #1: Type: text/plain, Size: 2865 bytes --] > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c > index d40cb5dd446ca..aaedda3730f37 100644 > --- a/kernel/bpf/hashtab.c > +++ b/kernel/bpf/hashtab.c [ ... ] > @@ -3022,10 +3013,12 @@ 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); > + bpf_obj_cancel_fields(&rhtab->map, > + rhtab_elem_value(elem, rhtab->map.key_size)); This isn't a bug, but could the second argument use old_val here? It already holds rhtab_elem_value(elem, map->key_size) from the top of the function and is used by copy_map_value_locked() two lines above. > @@ -3066,7 +3059,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)); > + /* > + * No explicit special-field initialization, matching the hash map's > + * non-prealloc path: fresh elements come zeroed from the bpf mem > + * allocator, and recycled elements had their timer/workqueue/task_work > + * slots reset by bpf_obj_cancel_fields() on delete. kptr slots are > + * left untouched so a recycled element keeps owning its reference > + * until rhtab_mem_dtor() releases it. > + */ A subsystem pattern flags this as potentially concerning: the copy_map_value() call on a field-capable map type is no longer followed by check_and_init_map_value(). Looking at the non-prealloc hash map path in alloc_htab_elem(), it appears this pattern has never called check_and_init_map_value() for fresh elements, and the bpf mem allocator does return zeroed memory. For recycled elements, the kptr slot staying intact is the intended behavior since rhtab_mem_dtor() handles final cleanup. Could you confirm this removal is safe for all special field types that BPF_MAP_TYPE_RHASH supports? This isn't a bug, but does the seven-line comment block add clarity over the briefer explanations already present in rhtab_delete_elem() and rhtab_map_update_existing()? The same rationale appears three times within about 110 lines. --- 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/33478386254 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next v3 1/4] bpf: Cancel special fields in resizable hashtab on recycle 2026-09-01 6:28 ` [PATCH bpf-next v3 1/4] " chenyuan_fl 2026-09-01 7:37 ` bot+bpf-ci @ 2026-09-01 16:57 ` Mykyta Yatsenko 1 sibling, 0 replies; 20+ messages in thread From: Mykyta Yatsenko @ 2026-09-01 16:57 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, Emil Tsalapatis, Ihor Solodrai, Shuah Khan, Nuoqi Gui, Yuan Chen On 9/1/26 7:28 AM, 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: call bpf_obj_cancel_fields() > on delete and in-place update, matching htab. 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. No special-field initialization is added to the element > alloc path: fresh elements come zeroed from the bpf mem allocator, > recycled elements already had their timer/workqueue/task_work slots > reset by bpf_obj_cancel_fields(), and check_and_init_map_value() would > zero the kptr slot of a recycled element, dropping the reference > without releasing it. > > 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") > Suggested-by: Mykyta Yatsenko <mykyta.yatsenko5@gmail.com> > Signed-off-by: Yuan Chen <chenyuan@kylinos.cn> > --- > kernel/bpf/hashtab.c | 32 ++++++++++++++++---------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c > index d40cb5dd446c..aaedda3730f3 100644 > --- a/kernel/bpf/hashtab.c > +++ b/kernel/bpf/hashtab.c > @@ -2864,16 +2864,6 @@ 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) > -{ > - if (IS_ERR_OR_NULL(rhtab->map.record)) > - return; > - > - bpf_obj_free_fields(rhtab->map.record, > - rhtab_elem_value(elem, rhtab->map.key_size)); > -} > - > static void rhtab_mem_dtor(void *obj, void *ctx) > { > struct htab_btf_record *hrec = ctx; > @@ -2963,8 +2953,9 @@ 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 */ > + bpf_obj_cancel_fields(&rhtab->map, > + rhtab_elem_value(elem, rhtab->map.key_size)); > bpf_mem_cache_free_rcu(&rhtab->ma, elem); > return 0; > } > @@ -3022,10 +3013,12 @@ 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); > + bpf_obj_cancel_fields(&rhtab->map, > + rhtab_elem_value(elem, rhtab->map.key_size)); > return 0; > } > > @@ -3066,7 +3059,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)); > + /* > + * No explicit special-field initialization, matching the hash map's > + * non-prealloc path: fresh elements come zeroed from the bpf mem > + * allocator, and recycled elements had their timer/workqueue/task_work > + * slots reset by bpf_obj_cancel_fields() on delete. kptr slots are > + * left untouched so a recycled element keeps owning its reference > + * until rhtab_mem_dtor() releases it. > + */ I'm not sure this comment is useful, we don't comment on why we are not zeroing special fields in htab, so why here. Please address the finding of the bot regarding the old_val variable and for the next respin send the patch series independently, not as a response to an old thread. > > /* Prevent deadlock for NMI programs attempting to take bucket lock */ > bpf_disable_instrumentation(); ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH bpf-next v3 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor 2026-09-01 6:28 ` [PATCH bpf-next v3 0/4] " chenyuan_fl 2026-09-01 6:28 ` [PATCH bpf-next v3 1/4] " chenyuan_fl @ 2026-09-01 6:28 ` chenyuan_fl 2026-09-01 17:10 ` Mykyta Yatsenko 2026-09-01 6:28 ` [PATCH bpf-next v3 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context chenyuan_fl 2026-09-01 6:28 ` [PATCH bpf-next v3 4/4] selftests/bpf: Test rhtab special-field combinations chenyuan_fl 3 siblings, 1 reply; 20+ messages in thread From: chenyuan_fl @ 2026-09-01 6:28 UTC (permalink / raw) To: bpf Cc: linux-kernel, linux-kselftest, Mykyta Yatsenko, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, Martin KaFai Lau, 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. btf_record_dup() only borrows the BTF references held by the fields: kptrs and list_head/rb_root fields point at the program BTF, whose lifetime is independent of the map. The duplicated record is used from the deferred mem-alloc destructor workqueue, which can run after the program BTF is gone (bpf_map_free() drops the map's reference, and the RCU callback may run first). Reading the borrowed descriptors there is a use-after-free, detected by KASAN as "slab-use-after-free in btf_is_kernel". Keep a reference on the borrowed program BTFs for the lifetime of the duplicated record. The record is freed from a preemptible worker, so the last btf_put() (which only schedules RCU destruction) does not make the field descriptors safe to read; snapshot the borrowed BTFs, free the record, then drop the references. 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c index aaedda3730f3..30bcc573772e 100644 --- a/kernel/bpf/hashtab.c +++ b/kernel/bpf/hashtab.c @@ -493,11 +493,76 @@ static void htab_pcpu_mem_dtor(void *obj, void *ctx) bpf_obj_free_fields(hrec->record, per_cpu_ptr(pptr, cpu)); } +/* + * The duplicated record borrows program BTFs, but is freed from a deferred + * workqueue that may run after the program BTF is gone. Hold a reference for + * the record's lifetime and snapshot the borrowed BTFs, since + * btf_record_free() frees the record. + */ + +/* Program BTF borrowed by the field, or NULL. */ +static struct btf *htab_field_borrowed_btf(const struct btf_field *field) +{ + struct btf *btf = NULL; + + switch (field->type) { + case BPF_KPTR_UNREF: + case BPF_KPTR_REF: + case BPF_KPTR_PERCPU: + case BPF_UPTR: + btf = field->kptr.btf; + break; + case BPF_LIST_HEAD: + case BPF_RB_ROOT: + btf = field->graph_root.btf; + break; + default: + break; + } + + if (btf && !btf_is_kernel(btf)) + return btf; + return NULL; +} + +/* Snapshot the program BTFs @rec borrows into @btfs; returns their count. */ +static int htab_record_prog_btfs_snapshot(struct btf_record *rec, + struct btf **btfs) +{ + int i, n = 0; + + if (IS_ERR_OR_NULL(rec)) + return 0; + + for (i = 0; i < rec->cnt; i++) { + struct btf *btf = htab_field_borrowed_btf(&rec->fields[i]); + + if (btf) + btfs[n++] = btf; + } + return n; +} + +static void htab_record_prog_btf_get(struct btf_record *rec) +{ + struct btf *btfs[BTF_FIELDS_MAX]; + int i, n; + + n = htab_record_prog_btfs_snapshot(rec, btfs); + for (i = 0; i < n; i++) + btf_get(btfs[i]); +} + static void htab_dtor_ctx_free(void *ctx) { struct htab_btf_record *hrec = ctx; + struct btf *btfs[BTF_FIELDS_MAX]; + int i, n; + n = htab_record_prog_btfs_snapshot(hrec->record, btfs); btf_record_free(hrec->record); + for (i = 0; i < n; i++) + btf_put(btfs[i]); kfree(ctx); } @@ -521,6 +586,7 @@ static int bpf_ma_set_dtor(struct bpf_map *map, struct bpf_mem_alloc *ma, kfree(hrec); return err; } + htab_record_prog_btf_get(hrec->record); 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 bpf-next v3 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor 2026-09-01 6:28 ` [PATCH bpf-next v3 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor chenyuan_fl @ 2026-09-01 17:10 ` Mykyta Yatsenko 0 siblings, 0 replies; 20+ messages in thread From: Mykyta Yatsenko @ 2026-09-01 17:10 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, Emil Tsalapatis, Ihor Solodrai, Shuah Khan, Nuoqi Gui, Yuan Chen On 9/1/26 7:28 AM, chenyuan_fl@163.com wrote: > From: Yuan Chen <chenyuan@kylinos.cn> > > bpf_ma_set_dtor() duplicates the map's btf_record for the bpf_mem_alloc > destructor. btf_record_dup() only borrows the BTF references held by > the fields: kptrs and list_head/rb_root fields point at the program > BTF, whose lifetime is independent of the map. The duplicated record is > used from the deferred mem-alloc destructor workqueue, which can run > after the program BTF is gone (bpf_map_free() drops the map's > reference, and the RCU callback may run first). Reading the borrowed > descriptors there is a use-after-free, detected by KASAN as > "slab-use-after-free in btf_is_kernel". > > Keep a reference on the borrowed program BTFs for the lifetime of the > duplicated record. The record is freed from a preemptible worker, so > the last btf_put() (which only schedules RCU destruction) does not make > the field descriptors safe to read; snapshot the borrowed BTFs, free > the record, then drop the references. > > The rhtab kptr selftests exercise this path on every map teardown and > triggered the bug under KASAN; with this fix they pass cleanly. Does htab trigger the same kasan? > > Fixes: 1df97a7453ee ("bpf: Register dtor for freeing special fields") > Signed-off-by: Yuan Chen <chenyuan@kylinos.cn> > --- > kernel/bpf/hashtab.c | 66 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c > index aaedda3730f3..30bcc573772e 100644 > --- a/kernel/bpf/hashtab.c > +++ b/kernel/bpf/hashtab.c > @@ -493,11 +493,76 @@ static void htab_pcpu_mem_dtor(void *obj, void *ctx) > bpf_obj_free_fields(hrec->record, per_cpu_ptr(pptr, cpu)); > } > > +/* > + * The duplicated record borrows program BTFs, but is freed from a deferred > + * workqueue that may run after the program BTF is gone. Hold a reference for > + * the record's lifetime and snapshot the borrowed BTFs, since > + * btf_record_free() frees the record. > + */ > + > +/* Program BTF borrowed by the field, or NULL. */ > +static struct btf *htab_field_borrowed_btf(const struct btf_field *field) > +{ > + struct btf *btf = NULL; > + > + switch (field->type) { > + case BPF_KPTR_UNREF: > + case BPF_KPTR_REF: > + case BPF_KPTR_PERCPU: > + case BPF_UPTR: > + btf = field->kptr.btf; > + break; > + case BPF_LIST_HEAD: > + case BPF_RB_ROOT: > + btf = field->graph_root.btf; > + break; > + default: > + break; > + } > + > + if (btf && !btf_is_kernel(btf)) > + return btf; > + return NULL; > +} > + > +/* Snapshot the program BTFs @rec borrows into @btfs; returns their count. */ > +static int htab_record_prog_btfs_snapshot(struct btf_record *rec, > + struct btf **btfs) > +{ > + int i, n = 0; > + > + if (IS_ERR_OR_NULL(rec)) > + return 0; > + > + for (i = 0; i < rec->cnt; i++) { > + struct btf *btf = htab_field_borrowed_btf(&rec->fields[i]); > + > + if (btf) > + btfs[n++] = btf; > + } > + return n; > +} > + > +static void htab_record_prog_btf_get(struct btf_record *rec) > +{ > + struct btf *btfs[BTF_FIELDS_MAX]; it looks like temporary array and htab_record_prog_btfs_snapshot() are unnecessary: for (i = 0; i < rec->cnt; i++) { struct btf *btf = htab_field_borrowed_btf(&rec->fields[i]); if (btf) btf_get(btf); } > + int i, n; > + > + n = htab_record_prog_btfs_snapshot(rec, btfs); > + for (i = 0; i < n; i++) > + btf_get(btfs[i]); > +} > + > static void htab_dtor_ctx_free(void *ctx) > { > struct htab_btf_record *hrec = ctx; > + struct btf *btfs[BTF_FIELDS_MAX]; > + int i, n; > > + n = htab_record_prog_btfs_snapshot(hrec->record, btfs); > btf_record_free(hrec->record); > + for (i = 0; i < n; i++) > + btf_put(btfs[i]); > kfree(ctx); > } > > @@ -521,6 +586,7 @@ static int bpf_ma_set_dtor(struct bpf_map *map, struct bpf_mem_alloc *ma, > kfree(hrec); > return err; > } > + htab_record_prog_btf_get(hrec->record); > bpf_mem_alloc_set_dtor(ma, dtor, htab_dtor_ctx_free, hrec); > return 0; > } ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH bpf-next v3 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context 2026-09-01 6:28 ` [PATCH bpf-next v3 0/4] " chenyuan_fl 2026-09-01 6:28 ` [PATCH bpf-next v3 1/4] " chenyuan_fl 2026-09-01 6:28 ` [PATCH bpf-next v3 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor chenyuan_fl @ 2026-09-01 6:28 ` chenyuan_fl 2026-09-01 7:37 ` bot+bpf-ci 2026-09-01 6:28 ` [PATCH bpf-next v3 4/4] selftests/bpf: Test rhtab special-field combinations chenyuan_fl 3 siblings, 1 reply; 20+ messages in thread From: chenyuan_fl @ 2026-09-01 6:28 UTC (permalink / raw) To: bpf Cc: linux-kernel, linux-kselftest, Mykyta Yatsenko, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, Martin KaFai Lau, 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 | 184 ++++++++++++++++++ .../testing/selftests/bpf/progs/rhtab_kptr.c | 146 ++++++++++++++ .../testing/selftests/bpf/rhtab_kptr_common.h | 6 + 3 files changed, 336 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c create mode 100644 tools/testing/selftests/bpf/progs/rhtab_kptr.c create mode 100644 tools/testing/selftests/bpf/rhtab_kptr_common.h 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..4bdcc9ce5500 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_kptr.c @@ -0,0 +1,184 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2026 KylinSoft Co., Ltd. */ + +#include <test_progs.h> +#include <linux/perf_event.h> +#include <stddef.h> +#include <sys/syscall.h> +#include <unistd.h> +#include "rhtab_kptr.skel.h" + +/* Userspace mirror of the BPF-side struct val_t (progs/rhtab_kptr.c). The + * update syscall copies map->value_size bytes from the buffer, so it must + * be at least that large; special fields are skipped by the value copy but + * the kernel still reads the full value_size from userspace. + */ +struct val_t_user { + __u64 tsk; + __u32 magic; + __u32 pad; +}; + +_Static_assert(sizeof(struct val_t_user) == 16, "val_t layout drift"); +_Static_assert(offsetof(struct val_t_user, magic) == 8, "val_t magic offset drift"); + +/* Zeroed value for creating/recreating elements; BSS is zero-filled. */ +static struct val_t_user zero; + +/* Cached CPU count and scratch buffer for percpu counter summation. */ +static __u64 *cpu_vals; +static int ncpu = -1; + +static __u64 read_counter(struct rhtab_kptr *skel, u32 idx) +{ + __u64 sum = 0; + int i, err; + + if (!cpu_vals) + return 0; + err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.counters), &idx, + cpu_vals); + if (!ASSERT_OK(err, "lookup_counter")) + return 0; + for (i = 0; i < ncpu; i++) + sum += cpu_vals[i]; + return sum; +} + +/* Run @name via BPF_PROG_TEST_RUN, asserting both the syscall status and + * that the program exited 0. Returns 0 on success. + */ +static int run_prog_ok(struct rhtab_kptr *skel, const char *name) +{ + LIBBPF_OPTS(bpf_test_run_opts, topts); + struct bpf_program *prog; + int err; + + prog = bpf_object__find_program_by_name(skel->obj, name); + if (!ASSERT_OK_PTR(prog, name)) + return -1; + err = bpf_prog_test_run_opts(bpf_program__fd(prog), &topts); + if (!ASSERT_OK(err, name)) + return -1; + if (!ASSERT_EQ(topts.retval, 0, name)) + return -1; + return 0; +} + +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), + }; + struct rhtab_kptr *skel; + __u64 init_before, nonnull_before; + __u32 key = 0; + int pmu_fd, i, retries = 0; + + ncpu = libbpf_num_possible_cpus(); + if (!ASSERT_GT(ncpu, 0, "num_possible_cpus")) + return; + cpu_vals = calloc(ncpu, sizeof(*cpu_vals)); + if (!ASSERT_OK_PTR(cpu_vals, "calloc_cpu_vals")) + return; + + skel = rhtab_kptr__open_and_load(); + if (!ASSERT_OK_PTR(skel, "open_and_load")) + goto out_free; + + /* 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 (run_prog_ok(skel, "init_elem") != 0) + 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. + */ + if (run_prog_ok(skel, "probe_elem") != 0) + 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. Every iteration runs exactly one probe, so the + * counters must add up to the loop count. + */ + init_before = read_counter(skel, 0); + nonnull_before = read_counter(skel, 2); + for (i = 0; i < 2000; i++) { + if (run_prog_ok(skel, "init_elem") != 0) { + /* init_elem fails only if the element is missing, + * which must not happen in this single-threaded + * loop; count it so a rhtab bug cannot be absorbed + * silently. + */ + retries++; + if (!ASSERT_OK(bpf_map_update_elem(bpf_map__fd(skel->maps.rhtab), + &key, &zero, BPF_ANY), + "recreate_elem")) + goto out; + if (run_prog_ok(skel, "init_elem") != 0) + goto out; + } + if (run_prog_ok(skel, "del_elem") != 0 || + run_prog_ok(skel, "upd_elem") != 0 || + run_prog_ok(skel, "probe_elem") != 0) + 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(retries, 0, "no_unexpected_recreate"); + ASSERT_EQ(read_counter(skel, 0) - init_before, 2000, "init_loop_count"); + 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); +out_free: + free(cpu_vals); +} 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..c96cf7f2d799 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/rhtab_kptr.c @@ -0,0 +1,146 @@ +// 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. + * + * The delete program checks that the element really disappeared, otherwise + * the following update would be an in-place update whose value copy skips + * the special fields, and the surviving kptr would prove nothing about the + * recycle path. + */ +#include <vmlinux.h> +#include <bpf/bpf_helpers.h> +#include "rhtab_kptr_common.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; + + if (bpf_map_delete_elem(&rhtab, &key)) + return 1; + /* The element must really be gone: otherwise the following upd_elem() + * is an in-place update on the surviving element and the kptr that + * probe_elem() observes never went through a recycle. + */ + if (bpf_map_lookup_elem(&rhtab, &key)) + return 2; + return 0; +} + +SEC("syscall") +int upd_elem(void *ctx) +{ + struct val_t val = { .magic = RHTAB_MAGIC }; + u32 key = 0; + + if (bpf_map_update_elem(&rhtab, &key, &val, BPF_ANY)) + return 1; + 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 == RHTAB_MAGIC) + bump(4); + return 0; +} diff --git a/tools/testing/selftests/bpf/rhtab_kptr_common.h b/tools/testing/selftests/bpf/rhtab_kptr_common.h new file mode 100644 index 000000000000..1b629fcb6742 --- /dev/null +++ b/tools/testing/selftests/bpf/rhtab_kptr_common.h @@ -0,0 +1,6 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (c) 2026 KylinSoft Co., Ltd. */ +#pragma once + +/* Magic value stored in the plain bytes of rhtab values ("RHAS"). */ +#define RHTAB_MAGIC 0x52484153 -- 2.54.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next v3 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context 2026-09-01 6:28 ` [PATCH bpf-next v3 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context chenyuan_fl @ 2026-09-01 7:37 ` bot+bpf-ci 0 siblings, 0 replies; 20+ messages in thread From: bot+bpf-ci @ 2026-09-01 7:37 UTC (permalink / raw) To: chenyuan_fl, bpf Cc: linux-kernel, linux-kselftest, mykyta.yatsenko5, ast, daniel, andrii, eddyz87, memxor, martin.lau, emil, ihor.solodrai, shuah, gnq25, chenyuan, ast, andrii, daniel, martin.lau, eddyz87, yonghong.song, mason, ihor.solodrai [-- Attachment #1: Type: text/plain, Size: 2234 bytes --] > 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 0000000000000..c96cf7f2d799b > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/rhtab_kptr.c > @@ -0,0 +1,146 @@ > +// 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. [ ... ] > +#include <vmlinux.h> > +#include <bpf/bpf_helpers.h> > +#include "rhtab_kptr_common.h" [ ... ] > diff --git a/tools/testing/selftests/bpf/rhtab_kptr_common.h b/tools/testing/selftests/bpf/rhtab_kptr_common.h > new file mode 100644 > index 0000000000000..1b629fcb6742d > --- /dev/null > +++ b/tools/testing/selftests/bpf/rhtab_kptr_common.h > @@ -0,0 +1,6 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright (c) 2026 KylinSoft Co., Ltd. */ > +#pragma once > + > +/* Magic value stored in the plain bytes of rhtab values ("RHAS"). */ > +#define RHTAB_MAGIC 0x52484153 This isn't a bug, but since only progs/rhtab_kptr.c uses RHTAB_MAGIC, could the define live in that file instead of a separate _common.h header? The *_common.h idiom at the selftests root is for constants that genuinely have to be identical on both the BPF and the userspace side. Here the userspace side never includes the header and never uses RHTAB_MAGIC. Moving the define into progs/rhtab_kptr.c next to struct val_t would avoid relying on the root directory being on the include path for .bpf.o builds, and root-level headers are not listed in the .bpf.o prerequisites, so edits to the header do not trigger a rebuild. --- 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/33478386254 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH bpf-next v3 4/4] selftests/bpf: Test rhtab special-field combinations 2026-09-01 6:28 ` [PATCH bpf-next v3 0/4] " chenyuan_fl ` (2 preceding siblings ...) 2026-09-01 6:28 ` [PATCH bpf-next v3 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context chenyuan_fl @ 2026-09-01 6:28 ` chenyuan_fl 2026-09-01 7:37 ` bot+bpf-ci 3 siblings, 1 reply; 20+ messages in thread From: chenyuan_fl @ 2026-09-01 6:28 UTC (permalink / raw) To: bpf Cc: linux-kernel, linux-kselftest, Mykyta Yatsenko, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi, Martin KaFai Lau, 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 | 337 ++++++++++++++++ .../selftests/bpf/progs/rhtab_fields.c | 378 ++++++++++++++++++ .../selftests/bpf/rhtab_fields_common.h | 19 + 3 files changed, 734 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/rhtab_fields.c create mode 100644 tools/testing/selftests/bpf/progs/rhtab_fields.c create mode 100644 tools/testing/selftests/bpf/rhtab_fields_common.h 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..93e8cbacca65 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c @@ -0,0 +1,337 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2026 KylinSoft Co., Ltd. */ + +#include <stddef.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <test_progs.h> +#include "rhtab_fields.skel.h" +#include "rhtab_fields_common.h" + +#define RECYCLE_LOOPS 2000 + +/* Userspace view of the lkmap value. The BPF side owns the real layout; + * the spin lock and the kptr are special fields that value copies skip, + * so only the plain bytes actually matter here. + */ +struct lock_kptr_val_user { + __u32 lock; + __u32 pad; + __u64 tsk; + __u32 magic; + __u32 pad2; +}; + +_Static_assert(sizeof(struct lock_kptr_val_user) == 24, "lkmap layout drift"); +_Static_assert(offsetof(struct lock_kptr_val_user, magic) == 16, + "lkmap magic offset drift"); + +/* + * Zeroed value buffer shared by every create/update issued from userspace. + * The update syscall copies map->value_size bytes from this buffer (special + * fields among them), so a short stack variable would be read past its end; + * BSS is zero-filled and 64 bytes cover every map in the skeleton. Each + * caller re-checks the size to keep that true as maps are added. + */ +static __u8 zero_val[64]; + +/* Cached CPU count and scratch buffer for percpu counter summation. */ +static __u64 *cpu_vals; +static int ncpu; + +static __u64 read_counter(struct rhtab_fields *skel, __u32 idx) +{ + __u64 sum = 0; + int i, err; + + err = bpf_map_lookup_elem(bpf_map__fd(skel->maps.counters), &idx, + cpu_vals); + if (!ASSERT_OK(err, "lookup_counter")) + return 0; + for (i = 0; i < ncpu; i++) + sum += cpu_vals[i]; + return sum; +} + +/* + * Run @name through BPF_PROG_TEST_RUN. Returns 0 on success and reports the + * program retval through @retval, so callers can tell a syscall failure, + * a prog that exited nonzero, and a prog that exited 0 apart. + */ +static int run_prog(struct rhtab_fields *skel, const char *name, int *retval) +{ + 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; + if (retval) + *retval = topts.retval; + return 0; +} + +/* run_prog() plus the assertion that the program exited 0. */ +static int run_prog_ok(struct rhtab_fields *skel, const char *name) +{ + int retval = -1; + + if (!ASSERT_OK(run_prog(skel, name, &retval), name)) + return -1; + if (!ASSERT_EQ(retval, 0, name)) + return -1; + return 0; +} + +/* Create one element filled with zero_val in @map. */ +static int create_zero_elem(struct bpf_map *map, const char *name) +{ + __u32 key = 0; + int fd; + + if (!ASSERT_LE(bpf_map__value_size(map), sizeof(zero_val), + "value_size_fits")) + return -1; + fd = bpf_map__fd(map); + return bpf_map_update_elem(fd, &key, zero_val, BPF_ANY); +} + +static void recycle_loop(struct rhtab_fields *skel, int map_fd, + const char *init, const char *del, + const char *upd, const char *probe, + int *retries) +{ + __u32 key = 0; + int i; + + for (i = 0; i < RECYCLE_LOOPS; i++) { + if (run_prog_ok(skel, init) != 0) { + /* init fails only if the element is missing, which + * must not happen in this single-threaded loop. Count + * it so a rhtab bug cannot be absorbed silently; the + * caller asserts the count is zero. + */ + (*retries)++; + if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, + zero_val, BPF_ANY), + "recreate_elem")) + return; + if (run_prog_ok(skel, init) != 0) + return; + } + if (run_prog_ok(skel, del) != 0) + return; + if (run_prog_ok(skel, upd) != 0) + return; + if (run_prog_ok(skel, probe) != 0) + 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, total; + int map_fd, retries = 0; + __u32 key = 0; + + map_fd = bpf_map__fd(skel->maps.lkmap); + + if (!ASSERT_OK(create_zero_elem(skel->maps.lkmap, "create_elem"), + "create_elem")) + return; + + /* The 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. + * Every iteration runs exactly one probe, so the two probe counters + * must add up to the loop count; the magic check must hold on every + * single probe. + */ + nonnull_before = read_counter(skel, 1); + recycle_loop(skel, map_fd, "lk_init", "lk_del", "lk_upd", "lk_probe", + &retries); + ASSERT_EQ(retries, 0, "no_unexpected_recreate"); + ASSERT_EQ(read_counter(skel, 0), RECYCLE_LOOPS, "lk_init_count"); + total = read_counter(skel, 1) + read_counter(skel, 2); + ASSERT_EQ(total, RECYCLE_LOOPS, "lk_probe_count"); + ASSERT_EQ(read_counter(skel, 3), RECYCLE_LOOPS, + "recycle_magic_roundtrip"); + ASSERT_GT(read_counter(skel, 1), nonnull_before, + "recycle_xchg_non_null"); + + /* 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) +{ + int fired, map_fd; + __u32 key = 0; + + map_fd = bpf_map__fd(skel->maps.tmap); + + if (!ASSERT_OK(create_zero_elem(skel->maps.tmap, "create_elem"), + "create_elem")) + return; + + /* 1. A short-delay timer must fire. */ + skel->data->timer_delay_ns = 50000; + if (run_prog_ok(skel, "arm_timer") != 0) + return; + usleep(300000); + if (!ASSERT_GT(skel->bss->timer_fired, 0, "timer_fired_first")) + return; + + /* + * 2. The real cancellation test: arm a long-delay timer on a freshly + * recycled element and delete the element while the timer is still + * pending. If the delete failed to cancel it, the callback would run + * before the sleep below ends. The element must be deleted and + * recreated between arms: bpf_timer_init() returns -EBUSY on an + * element whose timer has not been cancelled and freed yet. + */ + fired = skel->bss->timer_fired; + if (!ASSERT_OK(bpf_map_delete_elem(map_fd, &key), + "delete_before_rearm")) + return; + if (!ASSERT_OK(create_zero_elem(skel->maps.tmap, "recreate_elem"), + "recreate_before_rearm")) + return; + skel->data->timer_delay_ns = 200000000; + if (run_prog_ok(skel, "arm_timer") != 0) + return; + if (!ASSERT_OK(bpf_map_delete_elem(map_fd, &key), + "delete_cancels_timer")) + return; + usleep(300000); + ASSERT_EQ(skel->bss->timer_fired, fired, "timer_cancelled_after_delete"); + + /* 3. A recycled element can arm and fire a fresh timer again. */ + skel->data->timer_delay_ns = 50000; + if (!ASSERT_OK(create_zero_elem(skel->maps.tmap, "recreate_elem"), + "recreate_second")) + return; + if (run_prog_ok(skel, "arm_timer") != 0) + return; + usleep(300000); + ASSERT_GT(skel->bss->timer_fired, fired, "timer_fired_second"); +} + +struct kptr_recycle_case { + const char *name; + struct bpf_map **map; + const char *init; + const char *del; + const char *upd; + const char *probe; + __u32 init_idx; /* counter bumped on every successful init */ + __u32 nonnull_idx; /* counter bumped when the kptr was inherited */ + __u32 null_idx; /* counter bumped when the kptr was not inherited */ + __u32 marker_idx; /* percpu data roundtrip counter, 0 if none */ +}; + +static void subtest_kptr_recycle(struct rhtab_fields *skel, + const struct kptr_recycle_case *c) +{ + __u64 nonnull_before, total; + struct bpf_map *map; + int map_fd, retries = 0; + + map = *c->map; + map_fd = bpf_map__fd(map); + + if (!ASSERT_OK(create_zero_elem(map, "create_elem"), "create_elem")) + return; + + /* The kptr must survive the recycle without leaking its reference, + * exactly like the referenced kptr in the lkmap subtest. + */ + nonnull_before = read_counter(skel, c->nonnull_idx); + recycle_loop(skel, map_fd, c->init, c->del, c->upd, c->probe, + &retries); + ASSERT_EQ(retries, 0, "no_unexpected_recreate"); + ASSERT_EQ(read_counter(skel, c->init_idx), RECYCLE_LOOPS, + "init_count"); + total = read_counter(skel, c->nonnull_idx) + + read_counter(skel, c->null_idx); + ASSERT_EQ(total, RECYCLE_LOOPS, "probe_count"); + ASSERT_GT(read_counter(skel, c->nonnull_idx), nonnull_before, + "recycle_non_null"); + /* The marker read is CPU-local, so assert at least one hit. */ + if (c->marker_idx) + ASSERT_GT(read_counter(skel, c->marker_idx), 0, + "recycle_data_roundtrip"); +} + +static void subtest_kptr_recycles(struct rhtab_fields *skel) +{ + const struct kptr_recycle_case cases[] = { + { .name = "kptr_untrusted", .map = &skel->maps.umap, + .init = "u_init", .del = "u_del", .upd = "u_upd", + .probe = "u_probe", .init_idx = 4, .nonnull_idx = 5, + .null_idx = 6 }, + { .name = "kptr_percpu", .map = &skel->maps.pcmap, + .init = "pc_init", .del = "pc_del", .upd = "pc_upd", + .probe = "pc_probe", .init_idx = 7, .nonnull_idx = 8, + .null_idx = 9, .marker_idx = 10 }, + }; + int i; + + for (i = 0; i < ARRAY_SIZE(cases); i++) { + if (test__start_subtest(cases[i].name)) + subtest_kptr_recycle(skel, &cases[i]); + } +} + +void test_rhtab_fields(void) +{ + struct rhtab_fields *skel; + + ncpu = libbpf_num_possible_cpus(); + if (!ASSERT_GT(ncpu, 0, "num_possible_cpus")) + return; + cpu_vals = calloc(ncpu, sizeof(*cpu_vals)); + if (!ASSERT_OK_PTR(cpu_vals, "calloc_cpu_vals")) + return; + + skel = rhtab_fields__open_and_load(); + if (!ASSERT_OK_PTR(skel, "open_and_load")) { + free(cpu_vals); + return; + } + + if (test__start_subtest("lock_kptr")) + subtest_lock_kptr(skel); + if (test__start_subtest("timer")) + subtest_timer(skel); + subtest_kptr_recycles(skel); + + rhtab_fields__destroy(skel); + free(cpu_vals); +} 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..f8bcd88b7f34 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/rhtab_fields.c @@ -0,0 +1,378 @@ +// 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 a timer that is still + * pending (the delay is tunable through timer_delay_ns so userspace can + * arm a long timer and delete the element before it fires), 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, and the object's contents must + * survive the recycle round-trip. + * + * The delete programs check that the element really disappeared, otherwise + * the following update would be an in-place update whose value copy skips + * the special fields, and the surviving kptr would prove nothing about the + * recycle path. + */ + +#include <vmlinux.h> +#include <bpf/bpf_helpers.h> +#include "bpf_experimental.h" +#include "rhtab_fields_common.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; +}; + +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, 11); + __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: u probe ptr NULL, + * 7: pc init ok, 8: pc probe xchg non-NULL, 9: pc probe xchg NULL, + * 10: pc probe data roundtrip + */ +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; +extern void bpf_rcu_read_lock(void) __ksym; +extern void bpf_rcu_read_unlock(void) __ksym; + +/* Tunable from userspace (an initialized global lands in .data, so the + * driver writes it through skel->data, not skel->bss). + */ +int timer_delay_ns = 50000; +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; + + if (bpf_map_delete_elem(&lkmap, &key)) + return 1; + /* The element must really be gone: otherwise the following lk_upd() + * is an in-place update on the surviving element and the kptr that + * lk_probe() then observes never went through a recycle. + */ + if (bpf_map_lookup_elem(&lkmap, &key)) + return 2; + return 0; +} + +SEC("syscall") +int lk_upd(void *ctx) +{ + struct lock_kptr_val val = { .magic = LK_MAGIC }; + u32 key = 0; + + /* BPF_ANY is safe even though the value holds a spin lock: value + * copies skip special fields, so the lock word is never written and + * the prog side does not need to take the lock for an update. + */ + 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 magic; + u32 key = 0; + + val = bpf_map_lookup_elem(&lkmap, &key); + if (!val) + return 1; + /* Take the lock directly: a recycled element that came back with a + * corrupted lock word deadlocks here instead of passing. Helpers are + * forbidden while the lock is held, so the xchg stays outside. + */ + bpf_spin_lock(&val->lock); + magic = val->magic; + bpf_spin_unlock(&val->lock); + old = bpf_kptr_xchg(&val->tsk, NULL); + if (old) { + bpf_task_release(old); + bump(1); + } else { + bump(2); + } + if (magic == LK_MAGIC) + 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, timer_delay_ns, 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; + + if (bpf_map_delete_elem(&umap, &key)) + return 1; + if (bpf_map_lookup_elem(&umap, &key)) + return 2; + 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); + else + bump(6); + val->tsk = NULL; + return 0; +} + +/* Map 4: per-cpu kptr. */ + +SEC("syscall") +int pc_init(void *ctx) +{ + struct pcpu_val *val; + struct pcval *p, *cp, *q, *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); + /* After the xchg the slot holds p, so q aliases p. Syscall progs are + * sleepable, so the kptr field load only yields a trusted per-cpu + * view under an explicit RCU read lock. + */ + bpf_rcu_read_lock(); + q = val->pc; + if (q) { + cp = bpf_this_cpu_ptr(q); + cp->v = PC_MAGIC; + } + bpf_rcu_read_unlock(); + bump(7); + return 0; +} + +SEC("syscall") +int pc_del(void *ctx) +{ + u32 key = 0; + + if (bpf_map_delete_elem(&pcmap, &key)) + return 1; + if (bpf_map_lookup_elem(&pcmap, &key)) + return 2; + 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 *cp, *q, *old; + u32 key = 0; + int marker = 0; + + val = bpf_map_lookup_elem(&pcmap, &key); + if (!val) + return 1; + /* The object was freshly allocated and marked by pc_init() within + * the same iteration, so an inherited object must still carry the + * marker; q aliases old until the drop. Read the marker under the + * RCU read lock (which makes the field load trusted) and before the + * xchg NULLs the field. The read is CPU-local and the loop may + * migrate between CPUs, so userspace only asserts that this fired + * at least once. + */ + bpf_rcu_read_lock(); + q = val->pc; + if (q) { + cp = bpf_this_cpu_ptr(q); + if (cp->v == PC_MAGIC) + marker = 1; + } + bpf_rcu_read_unlock(); + old = bpf_kptr_xchg(&val->pc, NULL); + if (old) { + if (marker) + bump(10); + bpf_percpu_obj_drop(old); + bump(8); + } else { + bump(9); + } + return 0; +} diff --git a/tools/testing/selftests/bpf/rhtab_fields_common.h b/tools/testing/selftests/bpf/rhtab_fields_common.h new file mode 100644 index 000000000000..b52fb5e62807 --- /dev/null +++ b/tools/testing/selftests/bpf/rhtab_fields_common.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (c) 2026 KylinSoft Co., Ltd. */ +#pragma once + +/* + * Constants shared between the rhtab_fields BPF program (progs/) and its + * userspace driver (prog_tests/) so the two sides cannot drift apart + * silently. + */ + +/* Magic value stored in the plain bytes of lkmap values ("RHAB"). */ +#define LK_MAGIC 0x52484142 + +/* + * Marker written into freshly allocated per-cpu objects of pcmap. Distinct + * from LK_MAGIC on purpose: a hit proves the data came from our own + * pc_init(), not from anything else. + */ +#define PC_MAGIC 0x43504d47 -- 2.54.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH bpf-next v3 4/4] selftests/bpf: Test rhtab special-field combinations 2026-09-01 6:28 ` [PATCH bpf-next v3 4/4] selftests/bpf: Test rhtab special-field combinations chenyuan_fl @ 2026-09-01 7:37 ` bot+bpf-ci 0 siblings, 0 replies; 20+ messages in thread From: bot+bpf-ci @ 2026-09-01 7:37 UTC (permalink / raw) To: chenyuan_fl, bpf Cc: linux-kernel, linux-kselftest, mykyta.yatsenko5, ast, daniel, andrii, eddyz87, memxor, martin.lau, emil, ihor.solodrai, shuah, gnq25, chenyuan, ast, andrii, daniel, martin.lau, eddyz87, yonghong.song, mason, ihor.solodrai [-- Attachment #1: Type: text/plain, Size: 14171 bytes --] > 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: The commit message says it verifies "each field combination" from the enumerated list of seven special-field kinds (spin locks, timers, workqueues, task_work, kptrs, refcounts). However, test_rhtab_fields() only dispatches four subtests covering five field types: BPF_SPIN_LOCK, BPF_TIMER, BPF_KPTR_REF, BPF_KPTR_UNREF and BPF_KPTR_PERCPU. Three field kinds listed in the message are never exercised: BPF_WORKQUEUE (struct bpf_wq), BPF_TASK_WORK (struct bpf_task_work) and BPF_REFCOUNT (struct bpf_refcount). These are supported configurations that the recycle path must handle - map_check_btf() explicitly whitelists BPF_MAP_TYPE_RHASH for all three in kernel/bpf/syscall.c. Could the commit message be narrowed to match the actual coverage (e.g. "verify the spin-lock, timer and kptr combinations")? > 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..93e8cbacca656 > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/rhtab_fields.c > @@ -0,0 +1,337 @@ [ ... ] > +/* Userspace view of the lkmap value. The BPF side owns the real layout; > + * the spin lock and the kptr are special fields that value copies skip, > + * so only the plain bytes actually matter here. > + */ Three multi-line comments in this file start their text on the opening '/*' line instead of putting '/*' on its own line (here at line 14, line 114, and line 272). The BPF subsystem's comment style guide requires the opening '/*' on its own line even in selftests, and the same file already uses this style correctly elsewhere (lines 30, 57, 160, 211). Could these three be adjusted to match? [ ... ] > +static void recycle_loop(struct rhtab_fields *skel, int map_fd, > + const char *init, const char *del, > + const char *upd, const char *probe, > + int *retries) > +{ [ ... ] > + for (i = 0; i < RECYCLE_LOOPS; i++) { > + if (run_prog_ok(skel, init) != 0) { > + /* init fails only if the element is missing, which > + * must not happen in this single-threaded loop. Count > + * it so a rhtab bug cannot be absorbed silently; the > + * caller asserts the count is zero. > + */ > + (*retries)++; The comment states "init fails only if the element is missing" and says the caller asserts the count is zero, but run_prog_ok() has already reported the failure through ASSERT_OK()/ASSERT_EQ() which call test__fail(), so by the time (*retries)++ executes the subtest is already marked failed. The subsequent ASSERT_EQ(retries, 0, "no_unexpected_recreate") can only add a second failure line for an already-failed test, and the recreate+retry recovery merely lets the loop continue after a hard failure. Also, run_prog_ok() returns non-zero not only when the element is missing, but also when bpf_object__find_program_by_name() returns NULL, when bpf_prog_test_run_opts() fails with any errno, or when the BPF program returns any non-zero value. The BPF-side init programs return 2 on bpf_task_acquire()/bpf_percpu_obj_new() failure, not only on a missing element. Should this branch be dropped and the loop return on first failure (as the neighboring prog_tests/rhtab_kptr.c effectively does), or should the failure path not assert before deciding to retry? [ ... ] > +static void subtest_lock_kptr(struct rhtab_fields *skel) > +{ > + struct lock_kptr_val_user val = {}; > + struct lock_kptr_val_user out = {}; [ ... ] > + val.magic = LK_MAGIC; > + if (!ASSERT_OK(bpf_map_update_elem(map_fd, &key, &val, BPF_F_LOCK), > + "locked_update")) The syscall value buffers are sized by the userspace mirror struct (24 bytes) while the kernel writes map->value_size bytes. If the BPF-side value struct in progs/rhtab_fields.c ever grows, the test will overrun its own stack rather than failing cleanly. create_zero_elem() has the right guard - ASSERT_LE(bpf_map__value_size(map), sizeof(zero_val), "value_size_fits") - but only for the shared 64-byte zero_val buffer, not for val/out. The two _Static_assert()s at lines 26-27 only check the mirror against itself. Would adding an ASSERT_LE(bpf_map__value_size(skel->maps.lkmap), sizeof(val), ...) make the contract explicit? [ ... ] > +static void subtest_timer(struct rhtab_fields *skel) > +{ [ ... ] > + /* > + * 2. The real cancellation test: arm a long-delay timer on a freshly > + * recycled element and delete the element while the timer is still > + * pending. If the delete failed to cancel it, the callback would run The timer subtest documents itself as exercising the recycle path (the comment says "arm a long-delay timer on a freshly recycled element", and step 3's comment at line 235 says "A recycled element can arm and fire a fresh timer again"), but the element can never be recycled. rhtab_delete_elem() frees via bpf_mem_cache_free_rcu() -> unit_free_rcu(), which puts the object on c->free_by_rcu then c->waiting_for_gp. unit_alloc() pops only from c->free_llist, and alloc_bulk() refills only from c->free_by_rcu_ttrace / c->waiting_for_gp_ttrace - an object reaches free_by_rcu_ttrace only after a full RCU grace period. The subtest deletes and immediately re-creates the element with no grace period in between, and each rhtab map owns its own bpf_mem_alloc, so tmap has only ever freed one or two elements. The re-inserted element is therefore always a fresh allocation, not a recycled one. This means steps 2 and 3 verify only that delete cancels a pending timer and that a new element can arm a timer - both useful, but neither is the recycle behaviour the comments and the commit message claim. Unlike the kptr subtests, which loop RECYCLE_LOOPS=2000 times and therefore do get recycled elements, the timer subtest has no loop and no mechanism to force reuse. Should the timer subtest either loop like the kptr tests do, or have its comments and the commit message adjusted to reflect that it does not actually test recycle? [ ... ] The immediately preceding commit in the same series (b51e79dfbe300) added prog_tests/rhtab_kptr.c with largely identical infrastructure. A subsystem pattern flags adding a new selftest file for a narrow variant of behavior an existing test already covers: this file's percpu-counter summation helper read_counter(), its run_prog_ok() wrapper, and its 2000-iteration recycle loop with the identical retries+recreate_elem recovery block all appear in rhtab_kptr.c with the same assertion strings ("no_unexpected_recreate", "recycle_magic_roundtrip", "recycle_xchg_non_null"). The new kptr_untrusted and kptr_percpu cases are variants of coverage that rhtab_kptr.c provides for referenced kptrs, and the lock/timer cases are field variants of the same map type the pre-existing rhash.c in this directory already covers. Worth asking whether these should be additional subtests and BPF programs in rhtab_kptr.c or rhash.c, with one shared helper set and one common header, rather than a third parallel file? The current arrangement means any change to the recycle-loop helper has to be made in two places. > 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..f8bcd88b7f345 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/rhtab_fields.c > @@ -0,0 +1,378 @@ [ ... ] > + * 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 The comment says the spin lock of a recycled element is "initialized by the alloc path". Nothing in the recycle path initializes it. rhtab_map_update_elem() performs no special-field initialization - commit 8244b62c3a5ed in this same series removed check_and_init_map_value() and documents that fresh elements come zeroed from the allocator but recycled elements are not re-initialized. copy_map_value() skips every special-field offset. bpf_obj_cancel_fields() only touches timer, workqueue and task_work, not BPF_SPIN_LOCK. __alloc() in kernel/bpf/memalloc.c uses __GFP_ZERO only for fresh slab memory, not for objects from the per-cpu free list. So for "after every recycle" - the case this sentence is about - the alloc path contributes nothing. The lock word is unlocked on a recycled element only because no one held it when the element was deleted, not because anything initialized it. The parenthetical asserts an invariant enforced by initialization when it is only a consequence of the delete-time state. Could the parenthetical be reworded to say the lock word is left untouched by the recycle, so it must still be unlocked? [ ... ] > +/* 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: u probe ptr NULL, > + * 7: pc init ok, 8: pc probe xchg non-NULL, 9: pc probe xchg NULL, > + * 10: pc probe data roundtrip > + */ Seven new multi-line comments in this file start their text on the opening '/*' line instead of on the following line: here at line 98 (counter legend), line 117 (timer_delay_ns), line 152 (lk_del), line 167 (lk_upd), line 186 (lk_probe), line 304 (pc_init), and line 352 (pc_probe). The file's own top-of-file comment (lines 4-28) and the new shared header rhtab_fields_common.h both already use the required style, so the seven sites are inconsistent even within this patch. The BPF subsystem comment style guide explicitly extends the "opening /* on its own line" rule to selftests even if surrounding code uses the old style. Could these seven be adjusted to match the file's own top comment? [ ... ] The immediately preceding commit in this series added progs/rhtab_kptr.c with nearly identical init/delete/update/probe program quartets. lk_init/ lk_del/lk_upd/lk_probe here are near-verbatim re-implementations of init_elem/del_elem/upd_elem/probe_elem there, down to the comment prose. For example rhtab_kptr.c has upd_elem() with the same body structure as lk_upd() here, just a different map and struct. The genuinely new coverage in this patch is the spin-lock/timer/ untrusted-kptr/percpu-kptr field combinations; the plumbing around them is duplicated. A subsystem pattern flags this: a new selftest file added for a narrow variant of behavior an existing test already covers more generally. Worth asking whether rhtab_fields should have been additional map/prog cases inside the existing rhtab_kptr test (or whether the two files should share a helper/macro) rather than a second parallel test binary? [ ... ] > +SEC("syscall") > +int lk_upd(void *ctx) > +{ > + struct lock_kptr_val val = { .magic = LK_MAGIC }; > + u32 key = 0; > + > + /* BPF_ANY is safe even though the value holds a spin lock: value > + * copies skip special fields, so the lock word is never written and > + * the prog side does not need to take the lock for an update. > + */ > + bpf_map_update_elem(&lkmap, &key, &val, BPF_ANY); > + return 0; > +} All three update programs (lk_upd at line 171, u_upd at line 265, pc_upd at line 337) discard the bpf_map_update_elem() return value and unconditionally return 0. The error is reachable - rhtab_map_update_elem() allocates the element through bpf_mem_cache_alloc() and returns -ENOMEM on allocation failure (and -E2BIG when the map is full). Because the prog returns 0 anyway, the userspace run_prog_ok() wrapper cannot see the failure; it only surfaces one step later as the following *_probe() returning 1 from its lookup NULL check, i.e. as a misattributed failure in a different program. The equivalent function added by the preceding commit in this series does check it (progs/rhtab_kptr.c upd_elem(): 'if (bpf_map_update_elem(&rhtab, &key, &val, BPF_ANY)) return 1;'). Could these three match upd_elem() and return non-zero on failure? [ ... ] > +SEC("syscall") > +int lk_probe(void *ctx) > +{ > + struct lock_kptr_val *val; > + struct task_struct *old; > + __u32 magic; > + u32 key = 0; > + > + val = bpf_map_lookup_elem(&lkmap, &key); > + if (!val) > + return 1; > + /* Take the lock directly: a recycled element that came back with a > + * corrupted lock word deadlocks here instead of passing. Helpers are > + * forbidden while the lock is held, so the xchg stays outside. > + */ > + bpf_spin_lock(&val->lock); The comment accurately describes the intended behaviour on failure: "a recycled element that came back with a corrupted lock word deadlocks here instead of passing". That makes a hard lockup the test's designed failure signal. bpf_spin_lock() resolves to __bpf_spin_lock(), which disables preemption and then spins in arch_spin_lock() with no timeout, no trylock fallback and no other wake condition. A recycled element whose lock word is non-zero hangs that CPU inside a BPF program with preemption disabled - the machine wedges (hard lockup watchdog at best) rather than the selftest reporting a failure. RECYCLE_LOOPS is 2000, so a bad iteration will be hit. This matters specifically because, as the FILE-3-CHANGE-1-R1 issue documents, nothing in the recycle path re-initializes the lock word: bpf_obj_cancel_ fields() does not touch BPF_SPIN_LOCK, copy_map_value() skips it, and only fresh mem-allocator memory is __GFP_ZERO'd. So the exact regression this subtest is meant to detect is the one that hangs the box instead of failing. Would a bpf_spin_lock-free check (reading val->magic plainly, or driving the locked read from userspace via BPF_F_LOCK as subtest_lock_kptr() already does) report a failure instead of potentially taking the machine down? [ ... ] --- 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/33478386254 ^ 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] bpf: Cancel special fields in resizable hashtab on recycle chenyuan_fl 2026-08-24 14:36 ` [PATCH 1/4] " chenyuan_fl @ 2026-08-24 14:36 ` chenyuan_fl 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, 1 reply; 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:42 ` bot+bpf-ci 0 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: 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] bpf: Cancel special fields in resizable hashtab on recycle 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:42 ` bot+bpf-ci 2026-08-24 14:36 ` [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations chenyuan_fl 3 siblings, 1 reply; 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:42 ` bot+bpf-ci 0 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: 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] bpf: Cancel special fields in resizable hashtab on recycle 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:42 ` bot+bpf-ci 3 siblings, 1 reply; 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:42 ` bot+bpf-ci 0 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: 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
end of thread, other threads:[~2026-09-01 17:10 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <DKM6K95EN9OF.3O9XNYWVLYHDE@gmail.com>
2026-08-24 14:36 ` [PATCH bpf-next v2 0/4] bpf: Cancel special fields in resizable hashtab on recycle chenyuan_fl
2026-08-24 14:36 ` [PATCH 1/4] " chenyuan_fl
2026-08-24 15:42 ` bot+bpf-ci
2026-08-24 16:15 ` Mykyta Yatsenko
2026-09-01 6:28 ` [PATCH bpf-next v3 0/4] " chenyuan_fl
2026-09-01 6:28 ` [PATCH bpf-next v3 1/4] " chenyuan_fl
2026-09-01 7:37 ` bot+bpf-ci
2026-09-01 16:57 ` Mykyta Yatsenko
2026-09-01 6:28 ` [PATCH bpf-next v3 2/4] bpf: Fix use-after-free of program BTF in mem-alloc destructor chenyuan_fl
2026-09-01 17:10 ` Mykyta Yatsenko
2026-09-01 6:28 ` [PATCH bpf-next v3 3/4] selftests/bpf: Test rhtab kptr recycle from NMI context chenyuan_fl
2026-09-01 7:37 ` bot+bpf-ci
2026-09-01 6:28 ` [PATCH bpf-next v3 4/4] selftests/bpf: Test rhtab special-field combinations chenyuan_fl
2026-09-01 7:37 ` bot+bpf-ci
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: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 15:42 ` bot+bpf-ci
2026-08-24 14:36 ` [PATCH 4/4] selftests/bpf: Test rhtab special-field combinations chenyuan_fl
2026-08-24 15:42 ` bot+bpf-ci
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox