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

[bot+bpf-ci]

> 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