On Wed, Mar 27, 2024 at 10:02 AM Benjamin Tissoires benjamin.tissoires@redhat.com 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.