On Sun Jul 26, 2026 at 5:51 PM CEST, Nuoqi Gui wrote:
> Commit 6905f8601298 ("bpf: Allow special fields in resizable hashtab") says
> that "kptr semantics under in-place updates are identical to array map."
> However, after copy_map_value() preserves special fields,
> rhtab_map_update_existing() calls bpf_obj_free_fields() and drops retained
> kptrs. BPF_EXIST can therefore unexpectedly clear a kptr.
>
> Use bpf_obj_cancel_fields() in the update and deferred deletion paths, as
> hash and array maps do. It cancels timer, workqueue, and task-work state
> while the allocator destructor releases kptrs at final reclamation.
>
> Fixes: 6905f8601298 ("bpf: Allow special fields in resizable hashtab")
> Signed-off-by: Nuoqi Gui <[email protected]>
> ---
I don't think this is enough. Upon reading the code, I am suspicious about the
check_and_init_map_value() in rhtab_map_update_elem() (post this change). It
will likely end up zeroing non-zero kptrs and leaking them, since recycled
elements will retain the value. Dtor path is fine but that only covers the case
where freed element is never reused and only freed on map destruction.
It might amount to simply remove that function call from the function. The other
user in delete path looks fine since it operates on output buffer.
Please make sure to also add tests for such behavior, having a map of at most
one element should allow for its reuse, you might have to reinvoke the program,
once doing update, then another doing delete, then another doing update to be
able to rellocate the same element. I am sure it can be figured out (with AI's
help), but please validate it using tests.
pw-bot: cr
> kernel/bpf/hashtab.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 9f394e1aa2e8..54ea111daa8b 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -2865,14 +2865,14 @@ 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_check_and_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));
> + bpf_obj_cancel_fields(&rhtab->map,
> + rhtab_elem_value(elem, rhtab->map.key_size));
> }
>
> static void rhtab_mem_dtor(void *obj, void *ctx)
> @@ -2964,8 +2964,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 reusable internal structs: bpf_timer, task_work, wq */
> + rhtab_check_and_cancel_fields(rhtab, elem);
> bpf_mem_cache_free_rcu(&rhtab->ma, elem);
> return 0;
> }
> @@ -3027,7 +3027,7 @@ static long rhtab_map_update_existing(struct bpf_map
> *map, struct rhtab_elem *el
> * kptrs/etc. still sit in the slot. Cancel them after the copy
> * to match arraymap's update semantics.
> */
> - rhtab_check_and_free_fields(rhtab, elem);
> + rhtab_check_and_cancel_fields(rhtab, elem);
> return 0;
> }
>
