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

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2025-11-05 at 23:39 +0000, Mykyta Yatsenko wrote:
> 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).

Ok, so the following path is possible:
- bpf_timer_cancel_and_free
  - bpf_async_put
    - bpf_timer_delete (if refcount_dec_and_test(r: &cb->refcnt) returns true)
      - queue_work
      	- bpf_timer_delete_work
	  - hrtimer_cancel
	  - call_rcu(&t->cb.rcu, bpf_async_cb_rcu_free)

And thus, the following sequence of events would be possible w/o the
tryget:

  Thread A                      Thread B
  --------------------------    ------------------------------
  enter bpf_timer_start
  enter RCU protected region
                                bpf_timer_cancel_and_free call
				hrtimer_cancel()
				call_rcu(&t->cb.rcu, bpf_async_cb_rcu_free)
  hrtimer_start()
  exit RCU protected region

                     ... some thread ...
		     bpf_async_cb_rcu_free()
		     timer is popped from the queue, use after free
 
Makes sense, thank you for explaining.

[...]