Re: [PATCH RFC v1 5/5] bpf: remove lock from bpf_async_cb

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2025-10-31 at 21:58 +0000, Mykyta Yatsenko wrote:

[...]

> @@ -1344,12 +1348,17 @@ static int __bpf_async_init(struct bpf_async_kern *async, struct bpf_map *map, u
>  		/* maps with timers must be either held by user space
>  		 * or pinned in bpffs.
>  		 */
> -		WRITE_ONCE(async->cb, NULL);
> -		kfree_nolock(cb);
> -		ret = -EPERM;
> +		switch (type) {
> +		case BPF_ASYNC_TYPE_TIMER:
> +			bpf_timer_cancel_and_free(async);
> +			break;
> +		case BPF_ASYNC_TYPE_WQ:
> +			bpf_wq_cancel_and_free(async);
> +			break;
> +		}
> +		return -EPERM;

Just want to double-check my understanding, are below points correct?
- You need to call cancel_and_free() instead of kfree_nolock() because
  after cmpxchg() the map value is published and some other thread
  could have scheduled work/armed the timer.
- Previously this was not possible, because the other thread had to
  take the async->lock.

>  	}
> -out:
> -	__bpf_spin_unlock_irqrestore(&async->lock);
> +
>  	return ret;
>  }
>  
> @@ -1398,41 +1407,42 @@ static int bpf_async_swap_prog(struct bpf_async_cb *cb, struct bpf_prog *prog)
>  		if (IS_ERR(prog))
>  			return PTR_ERR(prog);
>  	}
> +	/* Make sure only one thread runs bpf_prog_put() */
> +	prev = xchg(&cb->prog, prog);
>  	if (prev)
>  		/* Drop prev prog refcnt when swapping with new prog */
>  		bpf_prog_put(prev);
>  
> -	cb->prog = prog;
>  	return 0;
>  }
>  
> -static int bpf_async_update_callback(struct bpf_async_kern *async, void *callback_fn,
> +static int bpf_async_update_callback(struct bpf_async_cb *cb, void *callback_fn,
>  				     struct bpf_prog *prog)
>  {
> -	struct bpf_async_cb *cb;
> +	enum bpf_async_state state;
>  	int err = 0;
>  
> -	__bpf_spin_lock_irqsave(&async->lock);
> -	cb = async->cb;
> -	if (!cb) {
> -		err = -EINVAL;
> -		goto out;
> -	}
> -	if (!atomic64_read(&cb->map->usercnt)) {
> -		/* maps with timers must be either held by user space
> -		 * or pinned in bpffs. Otherwise timer might still be
> -		 * running even when bpf prog is detached and user space
> -		 * is gone, since map_release_uref won't ever be called.
> -		 */
> -		err = -EPERM;
> -		goto out;
> -	}
> +	state = cmpxchg(&cb->state, BPF_ASYNC_READY, BPF_ASYNC_BUSY);
> +	if (state == BPF_ASYNC_BUSY)
> +		return -EBUSY;
> +	if (state == BPF_ASYNC_FREED)
> +		goto drop;

Why do you need to 'drop' at this point?
'cb' object had not been changed by current thread yet,
so it seems that something like:

  if (state == BPF_ASYNC_FREED)
    return -EPERM;

Should suffice.

>  
>  	err = bpf_async_swap_prog(cb, prog);
>  	if (!err)
>  		rcu_assign_pointer(cb->callback_fn, callback_fn);
> -out:
> -	__bpf_spin_unlock_irqrestore(&async->lock);
> +
> +	state = cmpxchg(&cb->state, BPF_ASYNC_BUSY, BPF_ASYNC_READY);
> +	if (state == BPF_ASYNC_FREED) {
> +		/*
> +		 * cb is freed concurrently, we may have overwritten prog and callback,
> +		 * make sure to drop them
> +		 */
> +drop:
> +		bpf_async_swap_prog(cb, NULL);
> +		rcu_assign_pointer(cb->callback_fn, NULL);
> +		return -EPERM;
> +	}
>  	return err;
>  }

[...]

> @@ -1472,12 +1489,19 @@ BPF_CALL_3(bpf_timer_start, struct bpf_async_kern *, timer, u64, nsecs, u64, fla
>  		return -EOPNOTSUPP;
>  	if (flags & ~(BPF_F_TIMER_ABS | BPF_F_TIMER_CPU_PIN))
>  		return -EINVAL;
> -	__bpf_spin_lock_irqsave(&timer->lock);
> -	t = timer->timer;
> -	if (!t || !t->cb.prog) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +
> +	guard(rcu)();
> +
> +	t = READ_ONCE(async->timer);
> +	if (!t)
> +		return -EINVAL;
> +
> +	/*
> +	 * Hold ref while scheduling timer, to make sure, we only cancel and free after
> +	 * hrtimer_start().
> +	 */
> +	if (!bpf_async_tryget(&t->cb))
> +		return -EINVAL;

Could you please explain in a bit more detail why tryget/put pair is
needed here?

>  	if (flags & BPF_F_TIMER_ABS)
>  		mode = HRTIMER_MODE_ABS_SOFT;

[...]

> @@ -1587,22 +1598,17 @@ static struct bpf_async_cb *__bpf_async_cancel_and_free(struct bpf_async_kern *a
>  {
>  	struct bpf_async_cb *cb;
>  
> -	/* Performance optimization: read async->cb without lock first. */
> -	if (!READ_ONCE(async->cb))
> -		return NULL;
> -
> -	__bpf_spin_lock_irqsave(&async->lock);
> -	/* re-read it under lock */
> -	cb = async->cb;
> -	if (!cb)
> -		goto out;
> -	drop_prog_refcnt(cb);
> -	/* The subsequent bpf_timer_start/cancel() helpers won't be able to use
> +	/*
> +	 * The subsequent bpf_timer_start/cancel() helpers won't be able to use
>  	 * this timer, since it won't be initialized.
>  	 */
> -	WRITE_ONCE(async->cb, NULL);
> -out:
> -	__bpf_spin_unlock_irqrestore(&async->lock);
> +	cb = xchg(&async->cb, NULL);
> +	if (!cb)
> +		return NULL;
> +
> +	/* cb is detached, set state to FREED, so that concurrent users drop it */
> +	xchg(&cb->state, BPF_ASYNC_FREED);
> +	bpf_async_update_callback(cb, NULL, NULL);

Calling bpf_async_update_callback() is a bit strange here.
That function protects 'cb' state by checking the 'cb->state',
but here that check is sidestepped.
Is this why you jump to drop for FREED state in bpf_async_update_callback()?

>  	return cb;
>  }

[...]