Re: [PATCH bpf-next v2 31/43] bpf: Keep values of a dead owner for sleepable programs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: special BTF fields leaked in bpf_selem_unlink_nofail() race
--

commit 441f9cc60cc3e26f38a89830b7d3f94a34c7d383
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Keep values of a dead owner for sleepable programs

This commit defers freeing BPF local storage elements and their special BTF
fields until after an RCU Tasks Trace grace period when an owner dies and
the map may be accessed by sleepable BPF programs, including maps that have
been used as inner maps.

> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> index 49ea00c993909..699430f971787 100644
> --- a/kernel/bpf/bpf_local_storage.c
> +++ b/kernel/bpf/bpf_local_storage.c
> @@ -352,7 +352,8 @@ int bpf_selem_unlink(struct bpf_local_storage_elem *selem)
>  static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
>  				    struct bpf_local_storage_map_bucket *b)
>  {
> -	bool in_map_free = !!b, free_storage = false;
> +	bool in_map_free = !!b, free_storage = false, sleepable_users = false;
> +	bool keep_fields = false;
>  	struct bpf_local_storage *local_storage;
>  	struct bpf_local_storage_map *smap;
>  	unsigned long flags;
> @@ -362,17 +363,26 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
>  	smap = rcu_dereference_check(SDATA(selem)->smap, bpf_rcu_lock_held());
>  
>  	if (smap) {
> +		/* A map that is freed, or NULL here, has no program left. */
> +		sleepable_users = !in_map_free &&
> +				  (atomic64_read(&smap->map.sleepable_refcnt) ||
> +				   READ_ONCE(smap->map.was_inner_map));
>  		b = b ? : select_bucket(smap, local_storage);
>  		err = raw_res_spin_lock_irqsave(&b->lock, flags);
>  		if (!err) {
>  			/*
>  			 * Call bpf_obj_free_fields() under b->lock to make sure it is done
>  			 * exactly once for an selem. Safe to free special fields immediately
> -			 * as no BPF program should be referencing the selem.
> +			 * as no BPF program should be referencing the selem, unless it
> +			 * is sleepable: see below.
>  			 */
>  			if (likely(selem_linked_to_map(selem))) {
>  				hlist_del_init_rcu(&selem->map_node);
> -				bpf_obj_free_fields(smap->map.record, SDATA(selem)->data);
> +				if (sleepable_users)
> +					keep_fields = true;
> +				else
> +					bpf_obj_free_fields(smap->map.record,
> +							    SDATA(selem)->data);
>  				unlink++;
>  			}
>  			raw_res_spin_unlock_irqrestore(&b->lock, flags);
> @@ -387,7 +397,14 @@ static void bpf_selem_unlink_nofail(struct bpf_local_storage_elem *selem,
>  		 * with selem1 as raw_res_spin_lock_irqsave() returns -ETIMEDOUT.
>  		 */
>  		WARN_ON_ONCE(err && in_map_free);
> -		if (!err || in_map_free)
> +		/*
> +		 * A sleepable program can still use, and fill, the fields of a value
> +		 * that it got before it let go of the owner. As for bpf_selem_unlink(),
> +		 * bpf_selem_free_trace_rcu() frees them, and takes smap for that:
> +		 * map_free() waits for this RCU read side critical section and then
> +		 * for the callback.
> +		 */
> +		if ((!err && !keep_fields) || in_map_free)
>  			RCU_INIT_POINTER(SDATA(selem)->smap, NULL);
>  	}

[Severity: High]
Can this race between bpf_local_storage_destroy() and
bpf_local_storage_map_free() leak the special BTF fields in
SDATA(selem)->data (such as referenced kptrs or pinned uptr pages) when
smap->map.was_inner_map is true?

Once a local storage map has been inserted into an outer map via
bpf_map_fd_get_ptr(), smap->map.was_inner_map remains permanently true even
when bpf_local_storage_map_free() later tears down the map.

If bpf_local_storage_destroy() and bpf_local_storage_map_free() concurrently
call bpf_selem_unlink_nofail() on the same selem and both read a non-NULL
smap before either clears SDATA(selem)->smap:

1. In bpf_local_storage_destroy() (in_map_free is false), sleepable_users
   evaluates to true because smap->map.was_inner_map is true. If it acquires
   b->lock first, it sees selem_linked_to_map(selem), unlinks selem->map_node,
   and sets its local keep_fields = true instead of calling
   bpf_obj_free_fields(). It then leaves SDATA(selem)->smap non-NULL so
   bpf_selem_free_trace_rcu() can free the fields later.

2. In the racing bpf_local_storage_map_free() (in_map_free is true), after it
   acquires b->lock, selem_linked_to_map(selem) is now false, so it also skips
   bpf_obj_free_fields(). However, because in_map_free is true (and its local
   keep_fields is false), it then executes:

	if ((!err && !keep_fields) || in_map_free)
		RCU_INIT_POINTER(SDATA(selem)->smap, NULL);

3. When bpf_selem_free_trace_rcu() later runs after the RCU Tasks Trace grace
   period:

	smap = rcu_dereference_check(SDATA(selem)->smap, 1);

	if (smap)
		bpf_obj_free_fields(smap->map.record, SDATA(selem)->data);

   smap is now NULL, so bpf_obj_free_fields() is skipped there as well and
   never runs for selem.

[ ... ]

>
> -- 
> 2.51.2
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017163745.3341787-1-alexei.starovoitov@gmail.com?part=31