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

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2025-11-05 at 15:30 +0000, Mykyta Yatsenko wrote:

[...]

> > > @@ -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?
>
> Yeah, we need to hold the reference to make sure even if cancel_and_free()
> go through, the underlying timer struct is not detached/freed, so we won't
> get into the situation when we first free, then schedule, with refcnt hold,
> we always first schedule and then free, this allows for cancellation run 
> when
> the last ref is put.

Sorry, I still don't get it.
In bpf_timer_start() you added `guard(rcu)()`.
In bpf_timer_cancel_and_free():

 - bpf_timer_cancel_and_free
   - bpf_async_put(cb: &t->cb, type: BPF_ASYNC_TYPE_TIMER)
     - bpf_timer_delete(t: (struct bpf_hrtimer *)cb);
       - bpf_timer_delete_work(work: &t->cb.delete_work);
       	 - call_rcu(head: &t->cb.rcu, func: bpf_async_cb_rcu_free)

So, it looks like `t->cb` is protected by RCU and can't go away
between `guard(rcu)()` and bpf_timer_start() exit.
What will go wrong if tryget is removed?

> > >   	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()?
>
> yes, this is probably a bit ugly, but I find it handy to have all the
> tricky code that mutates callback and prog inside the single function
> bpf_async_update_callback().

Probably subjective, but it makes things more confusing for me.