Re: [PATCH] bpf: defer freeing htab internal structs to workqueue to fix sleep-in-atomic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] Incorrect `container_of` usage in `htab_map_free_internal_structs_deferred` computes an invalid map pointer, leading to severe memory corruption and kernel panic.
- [High] Unbalanced `bpf_map_inc()` when `schedule_work()` returns false causes a permanent map reference leak.
- [High] Deferring the clearance of hash map internal structs to a workqueue creates a data race where active timers/kptrs can be wrongly destroyed if the map is concurrently resurrected.

Pre-existing issues:
- [High] Unsafe non-RCU list traversal under RCU read lock allows concurrent modifications to cause data races and torn pointers.
--

commit 8d67520d6573d4b74187481274921ee2cb635076
Author: Luo Gengkun <luogengkun2@huawei.com>

bpf: defer freeing htab internal structs to workqueue to fix sleep-in-atomic

This commit reverts commit 4f375ade6aa9 to restore VFS RCU protection, and
defers the freeing of htab internal structs to a workqueue. This avoids a
sleep-in-atomic bug that occurs when calling cond_resched() inside an RCU
grace period.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 3dd9b4924ae4f..6d6f1faeec67a 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -102,6 +102,7 @@ struct bpf_htab {
>  	u32 n_buckets;	/* number of hash buckets */
>  	u32 elem_size;	/* size of each element in bytes */
>  	u32 hashrnd;
> +	struct work_struct work;
>  };

[ ... ]

> @@ -1606,18 +1610,27 @@ static void htab_free_malloced_internal_structs(struct bpf_htab *htab)
>  	rcu_read_unlock();
>  }

[Severity: High]
This is a pre-existing issue, but looking at the function
htab_free_malloced_internal_structs() ending here:

Is it safe to use hlist_nulls_for_each_entry() inside the RCU read lock?

While the map's usercnt is zero, attached BPF programs can still execute in
parallel and modify the map. Since hlist_nulls_for_each_entry() does not use
rcu_dereference(), couldn't concurrent modifications cause torn pointers
and lead to data races or infinite loops?

Should this use hlist_nulls_for_each_entry_rcu() instead?

> -static void htab_map_free_internal_structs(struct bpf_map *map)
> +static void htab_map_free_internal_structs_deferred(struct work_struct *work)
>  {
> +	struct bpf_map *map = container_of(work, struct bpf_map, work);
>  	struct bpf_htab *htab = container_of(map, struct bpf_htab, map);

[Severity: Critical]
Is the container_of macro calculating the correct map pointer here?

The work item was initialized using &htab->work, but here it is cast via
container_of(work, struct bpf_map, work). Since 'work' is a member of
struct bpf_htab and not struct bpf_map, doesn't this shift the map pointer
into the middle of the struct instead of its start?

Could this lead to memory corruption when bpf_map_put(map) is subsequently
called with the invalid pointer?

Should this be calculated as:
	htab = container_of(work, struct bpf_htab, work);
	map = &htab->map;

> -	/* We only free internal structs on uref dropping to zero */
> -	if (!bpf_map_has_internal_structs(map))
> -		return;
> -
>  	if (htab_is_prealloc(htab))
>  		htab_free_prealloced_internal_structs(htab);
>  	else
>  		htab_free_malloced_internal_structs(htab);
> +	bpf_map_put(map);
> +}
> +
> +static void htab_map_free_internal_structs(struct bpf_map *map)
> +{
> +	struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
> +	/* We only free internal structs on uref dropping to zero */
> +	if (!bpf_map_has_internal_structs(map))
> +		return;
> +
> +	bpf_map_inc(map);
> +	schedule_work(&htab->work);

[Severity: High]
What happens if schedule_work() returns false?

A userspace program could repeatedly fetch the map via BPF_MAP_GET_FD_BY_ID
and close the file descriptor. Each closure drops usercnt to 0, triggering
this function.

Since bpf_map_inc() is called unconditionally, but schedule_work() returns
false if the work is already pending, won't this result in unbalanced
reference counts and permanently leak the BPF map?

[Severity: High]
Does deferring this operation to a workqueue introduce a race condition?

When usercnt reaches zero, we queue the internal structs (like timers and
kptrs) for deletion. Because map->refcnt can still be positive, can't
userspace immediately resurrect the map via BPF_MAP_GET_FD_BY_ID, bumping
usercnt back above zero?

If new timers or kptrs are then inserted, won't the deferred work eventually
run and indiscriminately destroy the internal structs of a now-active map?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260606022827.3553277-1-luogengkun2@huawei.com?part=1