On Thu, Jun 3, 2021 at 9:17 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > So your idea is to cmpxchg() to NULL while bpf_timer_start() or > > > bpf_timer_cancel() works with the timer? Wouldn't that cause > > > bpf_timer_init() believe that that timer is not yet initialized and > > > not return -EBUSY. Granted that's a corner-case race, but still. > > > > Not following. > > bpf prog should do bpf_timer_init only once. > > bpf_timer_init after bpf_timer_cancel is a wrong usage. > > hrtimer api doesn't have any protection for such use. > > while bpf_timer_init returns EBUSY. > > 2nd bpf_timer_init is just a misuse of bpf_timer api. > > Yes, clearly a bad use of API, but it's not prevented by verifier. not prevented only because it's hard to do in the verifier. > > > > Currently thinking to do cmpxchg in bpf_timer_start() and > > > > bpf_timer_cancel*() similar to bpf_timer_init() to address it. > > because that seemed like you were going to exchange (temporarily) a > pointer to NULL while doing bpf_timer_start() or bpf_timer_cancel(), > and then setting NULL -> valid ptr back again (this sequence would > open up a window when bpf_timer_init() can be used twice on the same > element). But again, with spinlock embedded doesn't matter anymore. Right, except bpf_timer_start and bpf_timer_cancel would xchg with -1 or similar and bpf_timer_init won't get confused. If two bpf_timer_start()s race on the same timer one would receive -EMISUSEOFAPI right away. Whereas with spin_lock inside bpf_timer both will be serialized and both will succeed. One can argue that bpf_timer_start and bpf_timer_cancel on different cpus is a realistic scenario. So xchg approach would need two special pointers -1 and -2 to distinguish start/start bad race vs start/cancel good race. And everything gets too clever. spin_lock is "obviously correct", though it doesn't have an advantage of informing users of api misuse. I coded it up and it's surviving the tests so far :)
