On Wed, Mar 27, 2024 at 10:02 AM Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote: > > > goto out; > > > } > > > + spin_lock(&t->sleepable_lock); > > > drop_prog_refcnt(t); > > > + spin_unlock(&t->sleepable_lock); > > > > this also looks odd. > > I basically need to protect "t->prog = NULL;" from happening while > bpf_timer_work_cb is setting up the bpf program to be run. Ok. I think I understand the race you're trying to fix. The bpf_timer_cancel_and_free() is doing cancel_work() and proceeds with kfree_rcu(t, rcu); That's the only race and these extra locks don't help. The t->prog = NULL is nothing to worry about. The bpf_timer_work_cb() might still see callback_fn == NULL "when it's being setup" and it's ok. These locks don't help that. I suggest to drop sleepable_lock everywhere. READ_ONCE of callback_fn in bpf_timer_work_cb() is enough. Add rcu_read_lock_trace() before calling bpf prog. The race to fix is above 'cancel_work + kfree_rcu' since kfree_rcu might free 'struct bpf_hrtimer *t' while the work is pending and work_queue internal logic might UAF struct work_struct work. By the time it may luckily enter bpf_timer_work_cb() it's too late. The argument 'struct work_struct *work' might already be freed. To fix this problem, how about the following: don't call kfree_rcu and instead queue the work to free it. After cancel_work(&t->work); the work_struct can be reused. So set it up to call "freeing callback" and do schedule_work(&t->work); There is a big assumption here that new work won't be executed before cancelled work completes. Need to check with wq experts. Another approach is to do something smart with cancel_work() return code. If it returns true set a flag inside bpf_hrtimer and make bpf_timer_work_cb() free(t) after bpf prog finishes. > Also, side note: if anyone feels like it would go faster to fix those > races by themself instead of teaching me how to properly do it, this > is definitely fine from me :) Most of the time goes into analyzing and thinking :) Whoever codes it doesn't speed things much. Pls do another respin if you still have cycles to work on it.
