linux-kernel - Re: [PATCH RFC 03/33] locking: Introduce <linux/thread

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <567ceff3-bbd9-4724-a80d-3db46ee40c98@acm.org>
Date: Fri, 7 Feb 2025 14:34:25 -0800
From: Bart Van Assche <bvanassche@....org>
To: Christoph Hellwig <hch@....de>
Cc: Peter Zijlstra <peterz@...radead.org>, Will Deacon <will@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Marco Elver <elver@...gle.com>, 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,
 Ingo Molnar <mingo@...hat.com>, Boqun Feng <boqun.feng@...il.com>,
 Waiman Long <longman@...hat.com>
Subject: Re: [PATCH RFC 03/33] locking: Introduce <linux/thread_safety.h>

On 2/6/25 7:53 PM, Christoph Hellwig wrote:
>> - a struct that represents a synchronization object is annotated with the
>>    CAPABILITY() attribute,
>> - the operations on that synchronization object are annotated with the
>>    ACQUIRE() and RELEASE() attributes,
>> - if variables or members that should be guarded by a synchronization
>>    object are annotated with GUARDED_BY(),
> 
> Those are all nasty shouting names, without and good prefixing.
> 
> But more importantly ACQUIRE() and RELEASE() seems to duplicate the
> existing __acquires/__releases annotations from sparse.  We really need
> to find away to unify them instead of duplicating the annotations.

I think that we are better off to drop support for the sparse locking
annotations. Linus added the macro __cond_acquires() two years ago in
the Linux kernel but support for that macro has never been implemented
in sparse. In the description of commit 4a557a5d1a61 ("sparse: introduce
conditional lock acquire function attribute") I found the following URL:
https://lore.kernel.org/all/CAHk-=wjZfO9hGqJ2_hGQG3U_XzSh9_XaXze=HgPdvJbgrvASfA@mail.gmail.com/

That URL points to an e-mail from Linus Torvalds with a patch for sparse
that implements support for __cond_acquires(). It seems to me that the
sparse patch has never been applied to the sparse code base (the git URL
for sparse is available at https://sparse.docs.kernel.org/en/latest/).

So while __cond_acquires() can be translated into a Clang annotation,
__cond_acquires() is not supported by sparse. There are other
challenges:
* The argument of __acquire() and __release() can be a "capability" or
   the address of a synchronization object. A few examples where the
   argument represents a capability:

   __acquire(RCU);
   __acquire(RCU_BH);
   __acquire(RCU_SCHED);
   __acquire(bitlock);

   An example where the argument represents the address of a
   synchronization object:

   static inline void do_raw_spin_lock(raw_spinlock_t *lock)
         __acquires(lock)
   {
	__acquire(lock);
	arch_spin_lock(&lock->raw_lock);
	mmiowb_spin_lock();
   }

   If __acquire() and __release() would have to be supported for both
   sparse and Clang, that would probably involve modifying all instances
   of these two macros across the entire kernel tree and making it
   explicit whether the argument is a capability or the address of a
   synchronization object.

* Clang verifies the thread-safety attribute arguments at compile time
   while sparse does not. An example from the scheduler:

extern
struct rq *__task_rq_lock(struct task_struct *p, struct rq_flags *rf)
	__acquires(rq->lock);

   An example from the workqueue implementation:

static void maybe_create_worker(struct worker_pool *pool)
__releases(&pool->lock)
__acquires(&pool->lock)
{
[ ... ]
}

   If __acquires() and __releases() are converted into Clang function
   attributes then compilation fails because Clang verifies the
   expressions passed to function attributes before the function body is
   compiled.

The team that is maintaining sparse is small (a single person?) and the
rate of change of sparse is low (the latest commit was published more
than a year ago). If we support both sparse locking annotations and the
Clang thread-safety annotations then we will end up fixing bugs in both
compilers. I propose to focus the maintenance effort on the project with
the highest velocity (Clang) and to drop support for the sparse locking
annotations.

Thanks,

Bart.