Re: [PATCH bpf-next 1/2] bpf: Cancel special fields in resizable hashtab on recycle

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 9f394e1aa2e8..70fa1e1b4b00 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -2865,14 +2865,23 @@ 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.
> +	 * kptr/uptr/list/rbtree destruction must not run from arbitrary BPF
> +	 * execution contexts (e.g. NMI), so leave those fields 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 the comment lists field types that BPF_MAP_TYPE_RHASH
values cannot contain. In map_check_btf(), BPF_UPTR is restricted to
BPF_MAP_TYPE_TASK_STORAGE, and BPF_LIST_HEAD/BPF_RB_ROOT are allowed only
in BPF_MAP_TYPE_HASH, BPF_MAP_TYPE_LRU_HASH and BPF_MAP_TYPE_ARRAY.
BPF_MAP_TYPE_RHASH is absent from both lists.

The only field types that bpf_obj_free_fields() actually handles beyond
the cancelled ones are BPF_KPTR_REF and BPF_KPTR_PERCPU. Could the
comment be more precise about what this code path actually affects?

>
>  static void rhtab_mem_dtor(void *obj, void *ctx)

[ ... ]

> @@ -2964,8 +2973,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;
>  }

Does this leak referenced kptrs when a deleted rhtab element is recycled
by the bpf memory allocator?

Unlike htab, rhtab re-initialises special fields of every freshly
allocated element. In rhtab_map_update_elem():

    elem = bpf_mem_cache_alloc(&rhtab->ma);
    ...
    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));

check_and_init_map_value() calls through to bpf_obj_init_field(), which
does memset(addr, 0, field->size) for BPF_KPTR_REF fields, overwriting
the pointer without releasing the reference.

Before this patch that was safe: rhtab_delete_elem() called
rhtab_check_and_free_fields(), which ran bpf_obj_free_fields() to xchg
the kptr out and drop it, so the recycled element arrived with the field
already zeroed. After the patch rhtab_cancel_fields() only touches
BPF_TIMER/BPF_WORKQUEUE/BPF_TASK_WORK via bpf_map_free_internal_structs(),
so the live kptr stays in the element.

Can this sequence occur?

  1. BPF prog stores a referenced kptr in an RHASH element (allowed:
     map_check_btf() lists BPF_MAP_TYPE_RHASH in the BPF_KPTR_REF
     allowlist).
  2. bpf_map_delete_elem()->rhtab_map_delete_elem()->rhtab_delete_elem()
     cancels only NMI-safe fields, then bpf_mem_cache_free_rcu().
  3. unit_free_rcu() puts the element on c->free_by_rcu; after a grace
     period it lands on c->free_llist.
  4. Next insert: rhtab_map_update_elem()->bpf_mem_cache_alloc()->
     unit_alloc() pops that element off c->free_llist.
  5. check_and_init_map_value() memsets the kptr field to 0. The
     reference is now unreachable and is never dropped.

The dtor cannot save it: rhtab_mem_dtor() is only reached from
free_all() in memalloc.c, which runs immediately before free_one()
returns the object to the slab, never on the unit_alloc() reuse path.

Why htab is not affected: alloc_htab_elem() has no
check_and_init_map_value() call, so a recycled htab element genuinely
inherits the kptr, which is what a3a81d247651 relies on. Does rhtab
break that pairing?

__rhtab_map_lookup_and_delete_batch() amplifies it: the loop at line
3363 calls rhtab_delete_elem() up to attr->batch.count times, leaking
one reference per element.

> @@ -3024,10 +3033,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:
rhtab_map_update_existing() overwrites a live map value with
copy_map_value()/copy_map_value_locked() and, after this patch, no
longer calls bpf_obj_free_fields() on the old value.

BPF_MAP_TYPE_RHASH is in the map_check_btf() allowlists for
BPF_SPIN_LOCK/BPF_RES_SPIN_LOCK, BPF_TIMER/BPF_WORKQUEUE/BPF_TASK_WORK
and BPF_KPTR_UNREF/BPF_KPTR_REF/BPF_KPTR_PERCPU/BPF_REFCOUNT, making it
a field-capable map type. The subsystem guide notes that map operations
on field-capable types that copy values with copy_map_value() should
pair with check_and_init_map_value() (lookups) or bpf_obj_free_fields()
(updates).

Observable effect: after a successful update the element's kptr slot
still holds the previous value's kptr, which the new value inherits. A
program that writes a fresh value and then reads the kptr field sees a
pointer it never installed.

The removal appears deliberate and mirrors upstream a3a81d247651, which
made exactly this change for array_map_update_elem() and
pcpu_copy_value() so that kptr destructors are not run from NMI. That
commit's changelog explicitly accepts the relaxed semantics: "This
deliberately relaxes the eager-free semantics of map update/delete for
special fields." On that reading the field inheritance here would be
intended rather than a bug. Is that the correct interpretation?


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

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