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

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 11/5/25 22:44, Eduard Zingerman wrote:
> 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?
bpf_timer_delete() also calls hrtimer_cancel(). If bpf_timer_start()
does not hold refcnt, we may run into the situation when hrtimer_cancel()
runs before hrtimer_start(). The timer is going to be deleted after the
grace period but it is not cancelled, and the timer callback may read 
after free.
Holding refcnt makes sure hrtimer_cancel() will be called after 
hrtimer_start()
(or way before it, and we error out).
>>>>    	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.
Yes, I think I see how it may not be super obvious from reader's 
perspective,
I'll change this, thanks!