Re: Thread Safety Analysis and the Linux kernel

On Wed, Mar 05, 2025 at 11:54:14PM +0000, Puchert, Aaron wrote:
> From: Marco Elver <[email protected]>
> > Hi Aaron ^ 2,
> > 
> > [+Cc a bunch of folks that were involved in discussions.]
> > 
> > After sending v2 of the -Wthread-safety / Capability Analysis patches
> > for the Linux kernel [1], a number of concrete improvements to
> > -Wthread-safety will be required sooner or later. It is unclear if
> > they are blockers to the Linux kernel's adoption because the feature
> > is currently designed as "opt-in per subsystem", but it's clear the
> > current state of things is not ideal.
> > 
> > [1] https://lore.kernel.org/all/[email protected]/
> 
> First of all, in addition to the documentation at https://clang.llvm.org/docs/ThreadSafetyAnalysis.html that you're already aware of, I can also recommend the paper by some of the original authors at https://static.googleusercontent.com/media/research.google.com/en/us/pubs/archive/42958.pdf.
> 
> > Peter Zijlstra's feedback:
> > https://lore.kernel.org/all/[email protected]/
> > -- much of which led to the below requests. Peter also managed to
> > crash Clang, but that's probably unrelated to -Wthread-safety
> > directly: https://github.com/llvm/llvm-project/issues/129873
> 
> Technically a parser issue and not in the analysis itself. But to my knowledge expressions in attributes were originally introduced for the analysis, so in some sense it is related.
> 
> > More straightforward requests:
> > 
> > 1. Re-entrant acquires: rcu_read_lock(), preempt_disable(), etc. are
> > all re-entrant locks. My proposal is to introduce an attribute that
> > can be added to "ACQUIRE(..)" annotated functions to indicate they are
> > re-entrant. Release-count needs to then match acquire-count to fully
> > release a capability.
> 
> We initially thought that this might be a problem for us as well, because we also use re-entrant locks. However, until now, it hasn't been an issue and I'm almost convinced that it never will be.
> 
> In essence, the analysis derives the set of statically known-to-be-held capabilities at every point in the program text. What does this mean? If you run the program, and stop it at any (time) point, you get the actual set of capabilities held at this point. However, a function might be called from different places, and depending on the caller, different capabilities might be held at the same point in the program text. The set that we compute does not and cannot contain all those capabilities, but only those that are held regardless of where execution comes from. So we generally don't know about capabilities held by callers, unless there is an annotation on the function, which is then checked at the call site.
> 
> Re-entrant locks are thus not necessarily an impediment: if one function acquires a lock and calls another function that also acquires it, the analysis won't complain. It analyzes functions in isolation. You'll only get a warning if you acquire a lock twice in one function without releasing it in between, or if the function is annotated as requiring the lock and acquires it again. However, in both cases the second acquisition can always be dropped. You can argue that this isn't a deadlock and hence not a bug, but the acquisition is unnecessary because the lock is already known to be held.
> 
> In theory I could construct cases where there are still problems: maybe we're locking twice because we're calling another function that unlocks once, but we still want the lock to be held afterwards. However, I've never seen this. In our code, re-entrant locks are only used because the caller might already hold the lock for some reason. The sophisticated patterns that I could think of never appeared. If this is different in the kernel, I'd be curious how it looks like.

We do have macros that acquire locks and enter RCU read-side critical
sections.  Also, depending on exactly where the analysis is done, static
inline functions might cause this to happen.

These can easily result in a function with nested calls to (for example)
rcu_read_lock().

							Thanx, Paul