> The code that deadlocked was entirely written by Superluminal who grabbed a shared lock from a interrupt handler
We don't "grab a shared lock". We call a kernel-provided eBPF helper function `bpf_ringbuf_reserve`, which, we now know, internally grabs a lock. The spinlock usage is entirely internal to the eBPF ringbuffer implementation and is not exposed to or controlled by the eBPF program at all.
The whole design behind eBPF is that it is a very controlled and constrained environment, backed by a verifier to ensure safety within the kernel context. It has a specific, limited kernel API in the form of eBPF helper functions and data structures that are guaranteed to succeed in that environment. If it compiles, passes the verifier, and loads, it should work. It is not feasible to know as a developer which of the many eBPF helpers[1] are and aren't safe to call in which contexts.
If `bpf_ringbuf_reserve` is unsafe to use from an interrupt context, then that would be one thing, but if so, it should be rejected by the verifier. There are other eBPF helper functions that only work within specific eBPF program types and are rejected outside of those contexts, so the verifier already knows how to make this distinction.
> The only way this could be considered a issue is that it appears that the Linux kernel added the rqspinlock which is supposed to automatically detect incorrect code at runtime and kind of “un-incorrect” it. That piece of code did not correctly detect callers who were blindly using it incorrectly in ways that the writers probably expected to detect.
Yeeeeah....that is, in fact, what the kernel did, and what the entire article is about. It's not about "incorrect code" though. Our use of `bpf_ringbuf_reserve` is, again, perfectly valid. It is more about giving the internal kernel helpers a way to deal with unexpected locking situations other than deadlocking.
> I do not see how you can effectively program in the rqspinlock environment. Any lock acquire can now fail with a timeout because some poorly written eBPF program decided that deadlocks were a enjoyable activity. Every single code path that acquires more than one lock must be able to guarantee global consistency before every lock acquire.
It is not "any lock", it is "any usage of rqspinlock within eBPF". This is intentional and already accounted for throughout eBPF. In this particular case, `bpf_ringbuf_reserve` is specified to return NULL on failure, and the verifier already forces you to deal with that in your eBPF program. The lock failing to acquire is one of the reasons why it returns NULL, but as the consumer of the API, you don't (or shouldn't) have to care about that. That's the explicit design contract.
> Furthermore, even if you do that all it does is turn deadlocks due to incorrect code into incredibly long multi-millisecond denials of service due to incorrect code
It doesn't turn them into "incredibly long multi-millisecond denials of service"...as long as the bugs are fixed. That is, again, what the entire article is about; with the fixes, it now recovers instantly in this scenario.
You should read the article. I hear it's good.