On 8/24/26 3:36 PM, [email protected] wrote:
> From: Yuan Chen <[email protected]>
>
> 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 <[email protected]>
> ---
> 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();
