On Thu, Jun 3, 2021 at 9:17 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

quoted

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.

quoted

quoted

quoted

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 :)