Re: [PATCH] bpf: Cancel special fields on rhashtab value recycle instead of freeing

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Deferring kptr destruction to the BPF memory allocator's destructor causes permanent resource leaks because the allocator routinely recycles elements without invoking the destructor.
--

--- Patch [1]: [PATCH] bpf: Cancel special fields on rhashtab value recycle instead of freeing ---
commit 1ee647bc226bed9cb1804699aa9faf7fa9345ec2
Author: Muhammad Falak R Wani <[email protected]>

bpf: Cancel special fields on rhashtab value recycle instead of freeing

This commit alters the update and delete paths for resizable hashtables
(BPF_MAP_TYPE_RHASH) to avoid fully freeing special fields like referenced
kptrs. Instead, it uses bpf_obj_cancel_fields() so that these operations are
safe to execute in NMI context, deferring kptr destruction to the allocator's
destructor.

Fixes: 6905f8601298e ("bpf: Allow special fields in resizable hashtab")

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index d8db1cebc1936..5f521d02784f3 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c

[ ... ]

> @@ -2964,8 +2954,12 @@ 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 timer, workqueue, and task_work fields before deferring the
> +	 * element free. Referenced kptr destruction is not NMI-safe, so leave
> +	 * it for rhtab_mem_dtor() after the RCU grace periods.
> +	 */
> +	bpf_obj_cancel_fields(&rhtab->map, rhtab_elem_value(elem, rhtab->map.key_size));
>  	bpf_mem_cache_free_rcu(&rhtab->ma, elem);

[Severity: High]
Does this code leak the referenced kptr fields if the BPF memory allocator
recycles the element?

When rhtab_delete_elem() defers the element free to bpf_mem_cache_free_rcu()
without freeing the kptr, the allocator might recycle the element directly 
to satisfy a new allocation request without ever calling the registered
destructor.

For example, looking at alloc_bulk() in kernel/bpf/memalloc.c:

	for (i = 0; i < cnt; i++) {
		obj = llist_del_first(&c->free_by_rcu_ttrace);
		if (!obj)
			break;
		add_obj_to_free_list(c, obj);
	}

Because this path moves the object straight to the free list without invoking
c->dtor (which would ordinarily call rhtab_mem_dtor -> bpf_obj_free_fields),
the kptr remains intact. When this recycled element is subsequently
initialized for a new key, check_and_init_map_value() zeroes out the kptr
field, which permanently loses the reference without decrementing the
refcount.

Could this lead to permanent resource leaks of kptrs, or potentially cause a
new map element to unexpectedly inherit a valid kptr from a previously deleted
element?

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1