Message-ID: <CANpmjNOsf1RnYJuZ7pmWVYNqVYMYe+eTL0vhDKU5sLihR9fN-g@mail.gmail.com>
Date: Thu, 6 Feb 2025 19:20:33 +0100
From: Marco Elver <elver@...gle.com>
To: Bart Van Assche <bvanassche@....org>
Cc: Peter Zijlstra <peterz@...radead.org>, Will Deacon <will@...nel.org>, 
	Christoph Hellwig <hch@....de>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Nick Desaulniers <ndesaulniers@...gle.com>, Nathan Chancellor <nathan@...nel.org>, Kees Cook <kees@...nel.org>, 
	Jann Horn <jannh@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 00/33] Compile-time thread-safety checking

On Thu, 6 Feb 2025 at 18:52, Bart Van Assche <bvanassche@....org> wrote:
>
> Hi Peter,
>
> As you may know the Clang compiler can perform certain thread-safety checks at
> compile time. This includes checking whether locking calls in a function match
> the function attributes and also whether annotated structure members are
> protected by the synchronization objects declared with GUARDED_BY(). As one can
> see this patch series includes a significant number of fixes for locking bugs.
> All these locking bugs have been discovered by the Clang thread-safety checker.
> I think this shows that it is useful to enable compile-time thread-safety
> checks.
>
> The Clang thread-safety infrastructure is not new. Some user space code in the
> kernel tree already uses it. See also the header file tools/perf/util/mutex.h.
> There are other projects that started using this infrastructure a while ago,
> e.g. Qemu. The header file include/qemu/clang-tsa.h was added more than two
> years ago to the Qemu project.
>
> This patch series has been posted as an RFC because at this stage my goal is to
> gather feedback about the following:
>  - Whether or not to enable compile-time thread-safety checking in the Linux
>    kernel.
>  - The names of the macros in header file include/linux/thread_safety.h. The
>    proposed names come from the Clang documentation (ACQUIRE(), RELEASE(),
>    REQUIRES(), ...). In the Qemu header file for thread-safety support these
>    macros have a prefix (TSA_ = Thread-Safety Analysis).
>  - The changes made in header file include/linux/cleanup.h that support
>    annotating constructor functions with function attributes.
>  - Whether or not to split the two treewide patches in this series into one
>    patch per kernel driver.
>  - How to merge this patch series. The second last patch in this series modifies
>    888 source files. If it is split into one patch per driver, this patch series
>    will become too large to be posted as a single patch series.
>
> Annotating struct mutex is a first step. Once struct mutex is annotated, driver
> contributors and/or maintainers can annotate structure members with GUARDED_BY()
> where this is considered useful.
>
> There are more synchronization objects that can be annotated with thread-safety
> attributes, e.g. spinlocks. This patch series only annotates struct mutex
> because annotating other synchronization objects too would have made this patch
> series too large.

Thanks Bart, for posting your approach.
I will also discuss my approach, as it will give everyone a clearer
picture of the design space here.

Bart and I have concurrently been working on this feature, and only in
recent days learned we both have been working on bringing
-Wthread-safety (aka. Clang's ThreadSafety Analysis [1], aka.
Capability analysis) to the Linux kernel. I have heard there have been
prior attempts, but only recently did Clang make -Wthread-safety
properly work with C code [2].

[1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
[2] https://github.com/llvm/llvm-project/issues/20777

However, we also realized there are 2 ways to bring this feature to the kernel:

1. Tree-wide all-or-nothing approach (this RFC). Adding support for
more "capabilities" - initially primarily synchronization primitives -
becomes incredibly hard after the first capability (currently "mutex"
only). This approach comes with all the known caveats of adding a new
tree-wide static analysis to the kernel.

In fact, what it really is, is yet another dialect of C with a
"capability system", with its own constraints. I dislike Clang's
naming and the notion this is about "thread safety only" - it's not.
It's a simple capability system, that can do much more.

In that vein, because we can't change the programming language (even
if from one C dialect to another) of the kernel overnight, a different
approach might cause less friction.

2. A selective, incremental, and much less intrusive approach.
Maintainers of subsystems opt in their modules or directories into
"capability analysis" (via Makefile directive
"CAPABILITY_ANALYSIS_foo.o := y" or "CAPABILITY_ANALYSIS := y"). Most
(eventually all) synchronization primitives and more capabilities
(including ones that could track "irq disabled", "preemption"
disabled, etc.) could be supported.

A prototype of approach #2 can be found here:
https://git.kernel.org/pub/scm/linux/kernel/git/melver/linux.git/log/?h=cap-analysis
Here's the RFC:
https://lore.kernel.org/all/20250206181711.1902989-1-elver@google.com/T/#u

Combining approach #1 and #2 may somehow be possible, but it is
currently eluding me. Clearly, based on the bugs that Bart found, some
way to do tree-wide analysis is very useful!
One idea was to have "capability-selective tree-wide analysis" (such
as mutex only) be controllable via a Kconfig option - the
implementation of that (without excessive ifdefs sprinkled
everywhere), however, most likely requires compiler support.

Depending on the feedback that results from these RFCs, I think we
will be able to plan better which direction things should go.

Thanks,
-- Marco

Powered by blists - more mailing lists