Re: [PATCH bpf-next 1/2] bpf: Cancel RHash special fields on value recycle

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 7/26/26 4:51 PM, 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 <gnq25@mails.tsinghua.edu.cn>
> ---

The change looks correct, corresponding htab fix was landed around
the same time as rhtab, so it was missed, and not replicated.

This patch correctly updates update/deletion callsites, which guarantees
that potentially NMI-unsafe kptr destructor is not called from NMI.
Final destructor rhtab_mem_dtor() left unchanged.

Acked-by: Mykyta Yatsenko <yatsenko@meta.com>

>  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;
>  }
>  
>