Re: [PATCH bpf-next v3 4/6] bpf: Invalidate async callbacks before detaching them

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] bpf: stale async->cb pointer when losing xchg() race in
  bpf_async_cancel_and_free()
--

commit 686504449c958e635d8414edba153676c8d63adc
Author: Yun Lu <luyun@kylinos.cn>

bpf: Invalidate async callbacks before detaching them

This commit changes bpf_async_cancel_and_free() to clear cb->map with xchg()
under RCU before clearing async->cb, so that a callback object is invalidated
before it is detached from the map value slot.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index a08b2af56ee64..716be7a852b58 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1747,15 +1747,18 @@ static void bpf_async_cancel_and_free(struct bpf_async_kern *async)
>  {
>  	struct bpf_async_cb *cb;
>  
> -	if (!READ_ONCE(async->cb))
> +	/* Another canceller can release the map-owned cb reference. */
> +	guard(rcu)();
> +	/* Order unlink before reading async->cb, including an empty slot. */
> +	smp_mb();
> +	/* Acquire any invalidation published before the slot was cleared. */
> +	cb = smp_load_acquire(&async->cb);
> +	/* Only the first invalidator detaches cb and drops the map's reference. */
> +	if (!cb || !xchg(&cb->map, NULL))
>  		return;
>  
> -	cb = xchg(&async->cb, NULL);
> -	if (!cb)
> -		return;
> +	WRITE_ONCE(async->cb, NULL);

[Severity: Medium]
Can a losing canceller return from bpf_async_cancel_and_free() while
async->cb is still non-NULL?

Before this patch, xchg(&async->cb, NULL) guaranteed that async->cb was
NULL whenever bpf_async_cancel_and_free() returned.  With the split between
xchg(&cb->map, NULL) and WRITE_ONCE(async->cb, NULL), a second canceller
can observe the invalidated cb and return early before the slot is cleared:

CPU0                              CPU1
array_map_update_elem()           array_map_update_elem()
  bpf_obj_cancel_fields()           copy_map_value()
    bpf_async_cancel_and_free()     bpf_obj_cancel_fields()
      cb = smp_load_acquire(...)      bpf_async_cancel_and_free()
      xchg(&cb->map, NULL)              cb = smp_load_acquire(...)
                                        !xchg(&cb->map, NULL)
                                        return;
                                  bpf_timer_init()
                                    __bpf_async_init()
                                      READ_ONCE(async->cb) != NULL
                                      return -EBUSY;  <-- spurious failure
      WRITE_ONCE(async->cb, NULL)

The same race occurs on a single CPU if an interrupt or NMI BPF program
updates the element and calls bpf_timer_init() or bpf_wq_init() while the
interrupted task is between xchg(&cb->map, NULL) and
WRITE_ONCE(async->cb, NULL), or when check_and_cancel_fields() races with
map_release_uref() before recycling a hash map element.

Having the losing canceller also clear the slot (for example via
cmpxchg(&async->cb, cb, NULL)) before returning would preserve the
guarantee that async->cb is NULL on return without overwriting a newly
published callback.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111024333.228633-1-luyun_611@163.com?part=4