Stop lockdep_assert_held*() from assuming lock ownership

Hi Peter and Marco,

Since commit 7c451541743c ("lockdep: Annotate lockdep assertions for context analysis"), lockdep_assert_held*() and lockdep_assert_in_rcu_*() not only verify at runtime that a lock is held but also instruct Clang context analysis to assume that the lock is held. This has the following disadvantages:
- These assertions silence context analysis warnings in functions that lack a __must_hold*() annotation. Hence, the compiler neither reports the missing function annotation nor checks at compile time whether callers hold the lock.
- lockdep_assert_held() makes context analysis assume exclusive lock ownership, even if a reader-writer lock is held in shared mode.
- lockdep_assert_held*() can only be used with lock types that are defined with context_lock_struct().

This patch series removes the context assumptions from the lockdep and RCU assertion macros and introduces separate macros for code that needs to inform context analysis that a lock is held:
- lockdep_assume_ctx_held() and lockdep_assume_shared_ctx_held().
- lockdep_assume_in_rcu_read_lock(), lockdep_assume_in_rcu_read_lock_bh(), lockdep_assume_in_rcu_read_lock_sched() and lockdep_assume_in_rcu_reader().
Unlike the __assume_*() macros, which only inform context analysis, the new lockdep_assume_*() macros also verify the assumption at runtime.

This patch series is organized as follows:
- Patches 1-4 fix issues in the existing context analysis support:
__bitlock() operator precedence, false positives for __free() helpers, enabling WARN_CONTEXT_ANALYSIS_ALL for allmodconfig and allyesconfig builds, and a too strict SRCU reader requirement in srcu_dereference_check().
- Patches 5-7 add __must_hold*() annotations to functions that currently rely on lockdep_assert_held() for context analysis.
- Patch 8 introduces lockdep_assume_ctx_held() and lockdep_assume_shared_ctx_held().
- Patches 9-11 convert code that accesses locks through pointer aliases or that uses __assume_ctx_lock() to lockdep_assume_ctx_held().
- Patch 12 removes the context assumptions from lockdep_assert_held*().
- Patches 13-14 do the same for the RCU assertions and introduce the lockdep_assume_in_rcu_*() macros.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Bart Van Assche (14):
bit_spinlock: Fix the __bitlock() definition
cleanup: Disable context analysis for __free() helpers
tracing: Disable branch profiling for COMPILE_TEST builds
srcu: Remove check from srcu_dereference_check()
futex: Annotate __attach_to_pi_owner() and fixup_pi_*owner() locking/rwsem: Annotate rwsem internal functions with __must_hold()
nvme-fabrics: Annotate nvmf_lookup_transport() with

    __must_hold_shared()

lockdep: Introduce lockdep_assume_ctx_held()
kfence: Use lockdep_assume_ctx_held() in kfence_report_error()
Bluetooth: Add lock context annotations for connection aliases
futex: Verify exit_mutex ownership in futex_exit_recursive()
lockdep: Stop lockdep_assert_held*() from assuming lock ownership
compiler-context-analysis: Split the RCU assertion tests
rcu: Introduce lockdep_assume_in_rcu_*()

 Documentation/dev-tools/context-analysis.rst | 20 +++--
 drivers/nvme/host/fabrics.c                  |  1 +
 include/linux/bit_spinlock.h                 |  2 +-
 include/linux/cleanup.h                      |  6 +-
 include/linux/lockdep.h                      | 77 ++++++++++++++++--
 include/linux/rcupdate.h                     | 82 +++++++++++++++++---
 include/linux/srcu.h                         | 20 +++--
 kernel/futex/core.c                          |  2 +-
 kernel/futex/futex.h                         |  3 +-
 kernel/futex/pi.c                            |  3 +
 kernel/locking/rtmutex.c                     |  7 +-
 kernel/locking/rwsem.c                       |  3 +
 kernel/trace/Kconfig                         |  3 +-
 lib/test_context-analysis.c                  | 38 +++++++--
 mm/kfence/report.c                           |  2 +-
 net/bluetooth/6lowpan.c                      |  2 +-
 net/bluetooth/iso.c                          |  8 +-
 net/bluetooth/l2cap_core.c                   | 48 ++++++------
 net/bluetooth/mgmt.c                         |  2 +-
 net/bluetooth/smp.c                          |  2 +-
 20 files changed, 256 insertions(+), 75 deletions(-)

Surround the bitnum argument and the entire __bitlock() macro expansion with parentheses because bitnum could be an expression that includes an operator with a lower precedence than the addition operator (e.g. the exclusive-or operator) and to prevent operator precedence issues in callers.

---
 include/linux/bit_spinlock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 7869a6e59b6a..c31d4f4d09ee 100644
@@ -17,7 +17,7 @@
  * static analysis can use.
  */
 context_lock_struct(__context_bitlock) { };
-#define __bitlock(bitnum, addr) (struct __context_bitlock *)(bitnum + (addr))
+#define __bitlock(bitnum, addr) ((struct __context_bitlock *)((bitnum) + (addr)))
 
 /*
  *  bit-based spin_lock()

The __free_##_name() helper functions defined by DEFINE_FREE() call the free expression that has been passed to DEFINE_FREE(). If that expression releases a lock, e.g. because it is a call to an unlock function, the Clang thread-safety analyzer complains because it cannot know that the variable that is passed to the helper function holds a lock. Hence, disable context analysis for the __free() helper functions.

---
 include/linux/cleanup.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 1fb8058b897d..f6afe80781a4 100644
@@ -207,8 +207,10 @@
  * Without the NULL test it turns into a mess and the compiler can't help us.
  */
 
-#define DEFINE_FREE(_name, _type, _free) \
-	static __always_inline void __free_##_name(void *p) { _type _T = *(_type *)p; _free; }
+#define DEFINE_FREE(_name, _type, _free)				\
+	static __always_inline void __free_##_name(void *p)		\
+		__context_unsafe(/* _free may release a lock */)		\
+	{ _type _T = *(_type *)p; _free; }
 
 #define __free(_name)	__cleanup(__free_##_name)
 

WARN_CONTEXT_ANALYSIS depends on !TRACE_BRANCH_PROFILING because branch profiling instruments every if-condition and obscures control flow from Clang's thread-safety analyzer. Make PROFILE_ANNOTATED_BRANCHES and PROFILE_ALL_BRANCHES depend on !COMPILE_TEST so that allmodconfig and allyesconfig builds do not enable TRACE_BRANCH_PROFILING and instead enable WARN_CONTEXT_ANALYSIS_ALL.

---
 kernel/trace/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 35d043b19332..afbf17099a68 100644
@@ -687,6 +687,7 @@ config BRANCH_PROFILE_NONE
 
 config PROFILE_ANNOTATED_BRANCHES
 	bool "Trace likely/unlikely profiler"
+	depends on !COMPILE_TEST
 	select TRACE_BRANCH_PROFILING
 	help
 	  This tracer profiles all likely and unlikely macros

@@ -698,7 +699,7 @@ config PROFILE_ANNOTATED_BRANCHES
 	  on if you need to profile the system's use of these macros.
 
 config PROFILE_ALL_BRANCHES
-	bool "Profile all if conditionals" if !FORTIFY_SOURCE
+	bool "Profile all if conditionals" if !FORTIFY_SOURCE && !COMPILE_TEST
 	select TRACE_BRANCH_PROFILING
 	help
 	  This tracer profiles all branch conditions. Every if ()

Users of srcu_dereference_check() must either hold an SRCU reader lock or satisfy the lockdep condition 'c', e.g. by holding the update-side lock. However, srcu_dereference_check() calls __srcu_read_lock_must_hold(), which tells the Clang context analyzer that the SRCU reader lock must be held. That requirement is too strict for update-side callers. Hence, remove the __srcu_read_lock_must_hold() call from srcu_dereference_check() while keeping the fake RCU acquire and release around __rcu_dereference_check(). Retain the runtime lockdep check in srcu_dereference_check().

srcu_dereference() and srcu_dereference_notrace() do not accept a lockdep condition and hence may only be used by SRCU readers. Call __srcu_read_lock_must_hold() from these macros such that Clang context analysis keeps verifying at compile time that their callers hold the SRCU reader lock.

---
 include/linux/srcu.h | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

index 1a8a465a5650..e72d2e1dfd82 100644
@@ -242,8 +242,11 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp)
  * No-op helper to denote that ssp must be held. Because SRCU-protected pointers
  * should still be marked with __rcu_guarded, and we do not want to mark them
  * with __guarded_by(ssp) as it would complicate annotations for writers, we
- * choose the following strategy: srcu_dereference_check() calls this helper
- * that checks that the passed ssp is held, and then fake-acquires 'RCU'.
+ * choose the following strategy: srcu_dereference() and
+ * srcu_dereference_notrace() call this helper that checks that the passed ssp
+ * is held, and srcu_dereference_check() fake-acquires 'RCU'.
+ * srcu_dereference_check() does not call this helper because its callers may
+ * satisfy the condition @c instead of holding ssp, e.g. on the update side.
  */
 static inline void __srcu_read_lock_must_hold(const struct srcu_struct *ssp) __must_hold_shared(ssp) { }
 

@@ -261,7 +264,6 @@ static inline void __srcu_read_lock_must_hold(const struct srcu_struct *ssp) __m
  */
 #define srcu_dereference_check(p, ssp, c)					\
 ({										\
-	__srcu_read_lock_must_hold(ssp);					\
 	__acquire_shared_ctx_lock(RCU);					\
 	__auto_type __v = __rcu_dereference_check((p), __UNIQUE_ID(rcu),	\
 				(c) || srcu_read_lock_held(ssp), __rcu);	\

@@ -279,7 +281,11 @@ static inline void __srcu_read_lock_must_hold(const struct srcu_struct *ssp) __m
  * is enabled, invoking this outside of an RCU read-side critical
  * section will result in an RCU-lockdep splat.
  */
-#define srcu_dereference(p, ssp) srcu_dereference_check((p), (ssp), 0)
+#define srcu_dereference(p, ssp)					\
+({									\
+	__srcu_read_lock_must_hold(ssp);				\
+	srcu_dereference_check((p), (ssp), 0);				\
+})
 
 /**
  * srcu_dereference_notrace - no tracing and no lockdep calls from here

@@ -287,7 +293,11 @@ static inline void __srcu_read_lock_must_hold(const struct srcu_struct *ssp) __m
  * @ssp: pointer to the srcu_struct, which is used to check that we
  *	really are in an SRCU read-side critical section.
  */
-#define srcu_dereference_notrace(p, ssp) srcu_dereference_check((p), (ssp), 1)
+#define srcu_dereference_notrace(p, ssp)				\
+({									\
+	__srcu_read_lock_must_hold(ssp);				\
+	srcu_dereference_check((p), (ssp), 1);				\
+})
 
 /**
  * srcu_read_lock - register a new reader for an SRCU-protected structure.

Annotate __attach_to_pi_owner() with __must_hold(&p->pi_lock) and annotate fixup_pi_state_owner() and fixup_pi_owner() with __must_hold(q->lock_ptr).

__attach_to_pi_owner() modifies p->futex.pi_state_list and attaches a new pi_state to owner task p, which requires holding p->pi_lock. Enforce this requirement at compile time with __must_hold(&p->pi_lock).

__fixup_pi_state_owner() requires holding q->lock_ptr. Its caller fixup_pi_state_owner() and outer caller fixup_pi_owner() must both be called with the hash bucket lock held. Currently, lockdep_assert_held(q->lock_ptr) in fixup_pi_state_owner() implicitly assumes lock ownership for Clang context analysis. Prepare for removing the context assumption from lockdep_assert_held() by annotating both fixup_pi_state_owner() and fixup_pi_owner() with __must_hold(q->lock_ptr).

---
 kernel/futex/futex.h | 3 ++-
 kernel/futex/pi.c    | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

index f00f0863ed44..3e162837e9d2 100644
@@ -394,7 +394,8 @@ extern int futex_lock_pi_atomic(u32 __user *uaddr, struct futex_hash_bucket *hb,
 extern int refill_pi_state_cache(void);
 extern void get_pi_state(struct futex_pi_state *pi_state);
 extern void put_pi_state(struct futex_pi_state *pi_state);
-extern int fixup_pi_owner(u32 __user *uaddr, struct futex_q *q, int locked);
+extern int fixup_pi_owner(u32 __user *uaddr, struct futex_q *q, int locked)
+	__must_hold(q->lock_ptr);
 
 /*
  * Express the locking dependencies for lockdep:

index 98f1b962e59a..e0310776b484 100644
@@ -420,6 +420,7 @@ static int handle_exit_race(u32 __user *uaddr, u32 uval)
 
 static void __attach_to_pi_owner(struct task_struct *p, union futex_key *key,
 				 struct futex_pi_state **ps)
+	__must_hold(&p->pi_lock)
 {
 	/*
 	 * No existing pi state. First waiter. [2]

@@ -912,6 +913,7 @@ static int __fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 
 static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
 				struct task_struct *argowner)
+	__must_hold(q->lock_ptr)
 {
 	struct futex_pi_state *pi_state = q->pi_state;
 	int ret;

@@ -940,6 +942,7 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q,
  *  - <0 - on error (-EFAULT)
  */
 int fixup_pi_owner(u32 __user *uaddr, struct futex_q *q, int locked)
+	__must_hold(q->lock_ptr)
 {
 	if (locked) {
 		/*

Annotate rwsem_del_waiter(), rwsem_mark_wake(), and rwsem_cond_wake_waiter() with __must_hold(&sem->wait_lock).

rwsem_del_waiter(), rwsem_mark_wake(), and rwsem_cond_wake_waiter() access members guarded by sem->wait_lock and call functions requiring sem->wait_lock. Currently, lockdep_assert_held(&sem->wait_lock) implicitly assumes lock ownership for Clang context analysis. Prepare for removing the context assumption from lockdep_assert_held() by explicitly annotating these functions with __must_hold(&sem->wait_lock).

---
 kernel/locking/rwsem.c | 3 +++
 1 file changed, 3 insertions(+)

index b9c180ac1eee..c76a3a712c31 100644
@@ -392,6 +392,7 @@ bool __rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
  */
 static inline bool
 rwsem_del_waiter(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
+	__must_hold(&sem->wait_lock)
 {
 	lockdep_assert_held(&sem->wait_lock);
 	if (__rwsem_del_waiter(sem, waiter))

@@ -429,6 +430,7 @@ struct rwsem_waiter *next_waiter(const struct rw_semaphore *sem,
 static void rwsem_mark_wake(struct rw_semaphore *sem,
 			    enum rwsem_wake_type wake_type,
 			    struct wake_q_head *wake_q)
+	__must_hold(&sem->wait_lock)
 {
 	struct rwsem_waiter *waiter, *next;
 	long oldcount, woken = 0, adjustment = 0;

@@ -995,6 +997,7 @@ rwsem_spin_on_owner(struct rw_semaphore *sem)
  */
 static inline void rwsem_cond_wake_waiter(struct rw_semaphore *sem, long count,
 					  struct wake_q_head *wake_q)
+	__must_hold(&sem->wait_lock)
 {
 	enum rwsem_wake_type wake_type;
 

Annotate nvmf_lookup_transport() with
__must_hold_shared(&nvmf_transports_rwsem).

nvmf_lookup_transport() reads nvmf_transports while holding nvmf_transports_rwsem for reading. Currently, lockdep_assert_held(&nvmf_transports_rwsem) implicitly assumes lock ownership for Clang context analysis. Prepare for removing the context assumption from lockdep_assert_held() by explicitly annotating nvmf_lookup_transport() with __must_hold_shared() to declare that callers must hold nvmf_transports_rwsem in shared mode.

---
 drivers/nvme/host/fabrics.c | 1 +
 1 file changed, 1 insertion(+)

index 3d7ad86c1955..d3d00adedd14 100644
@@ -644,6 +644,7 @@ EXPORT_SYMBOL_GPL(nvmf_unregister_transport);
 
 static struct nvmf_transport_ops *nvmf_lookup_transport(
 		struct nvmf_ctrl_options *opts)
+	__must_hold_shared(&nvmf_transports_rwsem)
 {
 	struct nvmf_transport_ops *ops;
 

I'm only seeing this one of 14 patches, so I have no idea what to do with it.

Hi Christoph,

If I have to repost this patch series I will Cc you on the entire series. The cover letter and the rest of this patch series are available here:
https://lore.kernel.org/lkml/[email protected]/

As explained in the cover letter, the purpose of this patch series is to prevent that lockdep_assert_held() suppresses the need to annotate the functions in which it occurs with __must_hold().

Thanks,

Bart.

So what is the point of the series? Do you want individual patches in the maintainer trees? Do they even work standalone? Do you want
ACKs to merge them together? That's the reason why you should never
ever selectively CC anyone - persons or lists. It just does not work.

Introduce lockdep_assume_ctx_held() and lockdep_assume_shared_ctx_held() for code that needs to inform Clang's context analysis that a lock is held in code paths where lock ownership cannot be proven at compile time, e.g. because the lock is accessed through a pointer alias that the analysis cannot resolve. These macros call respectively __assume_ctx_lock() and __assume_shared_ctx_lock() and also verify at runtime with lockdep that the lock is held. Store the macro argument in a local variable with a unique name such that it is evaluated only once and such that nested use of these macros does not cause shadowing. Rely on Clang's context analysis resolving that local variable to the expression it has been initialized with. Document both macros with kernel-doc comments, including that __assume_ctx_lock() and __assume_shared_ctx_lock() must be used for context locks without lockdep support, e.g. bit spinlocks and the RCU context tokens.

Convert the lockdep_assert_held() tests in lib/test_context-analysis.c into lockdep_assume_ctx_held() tests and add tests for lockdep_assume_shared_ctx_held().

This patch prepares for removing the context assumptions from lockdep_assert_held*().

---
 include/linux/lockdep.h     | 59 +++++++++++++++++++++++++++++++++++++
 lib/test_context-analysis.c | 16 ++++++++--
 2 files changed, 73 insertions(+), 2 deletions(-)

index a6451ecbbe9a..f774b22a35c5 100644
@@ -412,6 +412,65 @@ extern int lockdep_is_held(const void *);
 
 #endif /* !LOCKDEP */
 
+#define _lockdep_assume_ctx_held(var, uniq)				\
+	do {								\
+		__auto_type uniq = (var);				\
+									\
+		__assume_ctx_lock(uniq);				\
+		lockdep_assert_held(uniq);				\
+	} while (0)
+
+/**
+ * lockdep_assume_ctx_held() - assume and verify that a context lock is held
+ * @var: context lock instance pointer
+ *
+ * Instruct the compiler's context analysis to assume that @var is held
+ * exclusively from this point on and verify at runtime with
+ * lockdep_assert_held() that @var is held. Use this macro in code paths
+ * where lock ownership cannot be proven at compile time, e.g. because the
+ * lock is accessed through an alias the analysis cannot resolve or because
+ * a function does not always require the lock, making __must_hold()
+ * inappropriate. @var is evaluated only once.
+ *
+ * @var must point at a lock object that is tracked by lockdep, i.e. that
+ * has a dep_map member. For context locks that are not tracked by lockdep,
+ * e.g. bit spinlocks and the RCU context tokens, use
+ * __assume_ctx_lock() instead.
+ */
+#define lockdep_assume_ctx_held(var)					\
+	_lockdep_assume_ctx_held(var, __UNIQUE_ID(ctx_lock))
+
+#define _lockdep_assume_shared_ctx_held(var, uniq)			\
+	do {								\
+		__auto_type uniq = (var);				\
+									\
+		__assume_shared_ctx_lock(uniq);				\
+		lockdep_assert_held(uniq);				\
+	} while (0)
+
+/**
+ * lockdep_assume_shared_ctx_held() - assume and verify shared lock ownership
+ * @var: context lock instance pointer
+ *
+ * Instruct the compiler's context analysis to assume that @var is held with
+ * shared access from this point on and verify at runtime with
+ * lockdep_assert_held() that @var is held. Use this macro for reader-writer
+ * locks, e.g. rwlock_t and struct rw_semaphore, in code paths where shared
+ * lock ownership cannot be proven at compile time. @var is evaluated only
+ * once.
+ *
+ * Since lockdep_assert_held() is used, no lockdep warning is reported if
+ * @var is held for writing instead of for reading. Holding a lock
+ * exclusively implies that shared access is allowed. Use
+ * lockdep_assume_ctx_held() if exclusive access is required.
+ *
+ * @var must point at a lock object that is tracked by lockdep, i.e. that
+ * has a dep_map member. For context locks that are not tracked by lockdep,
+ * e.g. the RCU context tokens, use __assume_shared_ctx_lock() instead.
+ */
+#define lockdep_assume_shared_ctx_held(var)				\
+	_lockdep_assume_shared_ctx_held(var, __UNIQUE_ID(ctx_lock))
+
 #ifdef CONFIG_PROVE_LOCKING
 void lockdep_set_lock_cmp_fn(struct lockdep_map *, lock_cmp_fn, lock_print_fn);
 

index 316f4dfcda65..022891999c9c 100644
@@ -68,7 +68,7 @@ static void __used test_common_helpers(void)
 	}											\
 	static void __used test_##class##_assert(struct test_##class##_data *d)			\
 	{											\
-		lockdep_assert_held(&d->lock);							\
+		lockdep_assume_ctx_held(&d->lock);						\
 		op(d->counter);									\
 	}											\
 	static void __used test_##class##_guard(struct test_##class##_data *d)			\

@@ -156,6 +156,12 @@ TEST_SPINLOCK_COMMON(read_lock,
 		     read_trylock,
 		     TEST_OP_RO);
 
+static void __used test_read_lock_assert_shared(struct test_read_lock_data *d)
+{
+	lockdep_assume_shared_ctx_held(&d->lock);
+	TEST_OP_RO(d->counter);
+}
+
 struct test_mutex_data {
 	struct mutex mtx;
 	int counter __guarded_by(&mtx);

@@ -203,7 +209,7 @@ static void __used test_mutex_trylock(struct test_mutex_data *d, atomic_t *a)
 
 static void __used test_mutex_assert(struct test_mutex_data *d)
 {
-	lockdep_assert_held(&d->mtx);
+	lockdep_assume_ctx_held(&d->mtx);
 	d->counter++;
 }
 

@@ -339,6 +345,12 @@ static void __used test_rwsem_assert(struct test_rwsem_data *d)
 	d->counter++;
 }
 
+static void __used test_rwsem_assert_shared(struct test_rwsem_data *d)
+{
+	lockdep_assume_shared_ctx_held(&d->sem);
+	(void)d->counter;
+}
+
 static void __used test_rwsem_guard(struct test_rwsem_data *d)
 {
 	{ guard(rwsem_read)(&d->sem); (void)d->counter; }

So the only convincing rationale from 0/n for doing this was that the current annotation can hide the need, and thus cause a lacking of __must_hold() on the function.

I'm not sure I understand how this alternative form of the same avoids that same problem, and the above doesn't seem to mention the whole problem -- either that, or I just can't read this morning.

There's no problem if you're dealing with new code. But we have a lot of lockdep_assert_held*() sprinkled all over existing code, which when compiling the kernel with WARN_CONTEXT_ANALYSIS_ALL will not warn where we would be better off with a __must_hold(). I believe Bart had been working on enabling this more broadly and doesn't want lockdep_assert_held*() hide warnings.

This latest iteration is a compromise for that problem, rather than the previous proposals of removing __assume_ctx_lock() from lockdep primitives entirely. The programming model is unchanged: we delegate to dynamic checks where the static analysis fails, and that's the main purpose of __assume_ctx_lock(). The only difference is that we'd have to be more explicit about it, just because of legacy use of lockdep_assert_held*().

I'm somewhat on the fence still, although would be willing to accept this compromise if it does help catch more bugs, and is neutral to only mildly more mentally taxing in writing new code.

Thanks,
-- Marco

Aaah, so the 'solution' is to rename lockdep_assert_held(), such that the pre-existing annotation doesn't pre-pollute things? But if you start by converting existing lockdep_assert_held() to the new lockdep_assume_*_ctx_held() you'll run into the exact same problems?

That is a very minor and very subtle 'fix'.

Yes.

The number of lockdep_assert_held() users that has to be modified is very small. There are only three cases in the entire kernel tree: futex, kfence and Bluetooth. There three changes have been included in this patch series. I have verified this by adding lock context annotations in the entire kernel tree and by enabling lock context analysis for the entire kernel tree. I'm sending the patches that add lock context annotations to kernel maintainers as time permits. An example is available here: "[PATCH v4 00/54] scsi: Enable context analysis for most drivers"
(https://lore.kernel.org/linux-scsi/[email protected]/).

Thanks,

Bart.

Yes.

You are right that the patch description is not clear enough. I will fix this when I post a second version of this patch series.

Whether or not to use lockdep_assert_held_read() in the definition of _lockdep_assume_shared_ctx_held() is the aspect in this patch series I was not sure of. If nobody objects I will switch to lockdep_assert_held_read() in this macro.

Thanks,

Bart.

Replace lockdep_assert_held(&meta->lock) with lockdep_assume_ctx_held(&meta->lock) in kfence_report_error().

kfence_print_object() requires holding &meta->lock. In kfence_report_error(), meta is optional (nullable), so the function cannot be annotated with __must_hold(&meta->lock). Currently, lockdep_assert_held(&meta->lock) implicitly assumes lock ownership for Clang context analysis. Prepare for removing the context assumption from lockdep_assert_held() by using lockdep_assume_ctx_held(&meta->lock) inside the "if (meta)" block. This informs Clang context analysis that meta->lock is held and preserves the runtime lockdep check.

---
 mm/kfence/report.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d548536864b1..5b8588d35b77 100644
@@ -286,7 +286,7 @@ kfence_report_error(unsigned long address, bool is_write, struct pt_regs *regs,
 	stack_trace_print(stack_entries + skipnr, num_stack_entries - skipnr, 0);
 
 	if (meta) {
-		lockdep_assert_held(&meta->lock);
+		lockdep_assume_ctx_held(&meta->lock);
 		pr_err("\n");
 		kfence_print_object(NULL, meta);
 	}

Replace lockdep_assert_held() with lockdep_assume_ctx_held() where hdev->lock, an L2CAP connection lock or an L2CAP channel lock is accessed through a pointer alias.

Currently, lockdep_assert_held() implicitly assumes lock ownership for Clang context analysis. Prepare for removing the context assumption from lockdep_assert_held(). In Bluetooth code, functions access members guarded by hdev->lock (such as l2cap_data) or call functions requiring hdev->lock through connection pointer aliases (hcon->hdev->lock, conn->hdev->lock, link->conn->hdev->lock). Similarly, L2CAP functions call functions that require chan->conn->lock while holding conn->lock, and l2cap_ecred_rsp_defer() relies on rsp->locked_chan->lock, rsp->locked_chan->conn->lock, chan->lock and chan->conn->lock being held.

Replace lockdep_assert_held() with lockdep_assume_ctx_held() at these alias access points. This informs Clang context analysis that the lock is held and preserves the runtime lockdep checks.

---
 net/bluetooth/6lowpan.c    |  2 +-
 net/bluetooth/iso.c        |  8 +++----
 net/bluetooth/l2cap_core.c | 48 +++++++++++++++++++-------------------
 net/bluetooth/mgmt.c       |  2 +-
 net/bluetooth/smp.c        |  2 +-
 5 files changed, 31 insertions(+), 31 deletions(-)

index 8c51c20f6c4b..0a1a067d4870 100644
@@ -1019,7 +1019,7 @@ static int get_l2cap_conn(char *buf, bdaddr_t *addr, u8 *addr_type,
 		return -ENOENT;
 	}
 
-	lockdep_assert_held(&hcon->hdev->lock);
+	lockdep_assume_ctx_held(&hcon->hdev->lock);
 
 	*conn = l2cap_conn_hold_unless_zero(hcon->l2cap_data);
 

index c1fdd9846eeb..bedac9b0682e 100644
@@ -466,7 +466,7 @@ static int iso_connect_bis(struct sock *sk)
 			iso_pi(sk)->bc_sid = hcon->sid;
 	}
 
-	lockdep_assert_held(&hcon->hdev->lock);
+	lockdep_assume_ctx_held(&hcon->hdev->lock);
 
 	conn = iso_conn_add(hcon);
 	if (!conn) {

@@ -572,7 +572,7 @@ static int iso_connect_cis(struct sock *sk)
 		}
 	}
 
-	lockdep_assert_held(&hcon->hdev->lock);
+	lockdep_assume_ctx_held(&hcon->hdev->lock);
 
 	/* The socket lock keeps the current attachment and its hcon stable. */
 	if (iso_pi(sk)->conn && iso_pi(sk)->conn->hcon != hcon) {

@@ -1390,7 +1390,7 @@ static int iso_listen_bis(struct sock *sk)
 		goto unlock;
 	}
 
-	lockdep_assert_held(&hcon->hdev->lock);
+	lockdep_assume_ctx_held(&hcon->hdev->lock);
 
 	conn = iso_conn_add(hcon);
 	if (!conn) {

@@ -2657,7 +2657,7 @@ static void iso_connect_cfm(struct hci_conn *hcon, __u8 status)
 
 			list_for_each_entry_safe(link, t, &hcon->link_list,
 						 list) {
-				lockdep_assert_held(&link->conn->hdev->lock);
+				lockdep_assume_ctx_held(&link->conn->hdev->lock);
 				iso_conn_del(link->conn, bt_to_errno(status));
 			}
 

index 3b9c82bd2f02..022e7f41a91a 100644
@@ -3902,7 +3902,7 @@ static int user_pairing_resp(struct sock *sk, struct hci_dev *hdev,
 	}
 
 	if (addr->type == BDADDR_LE_PUBLIC || addr->type == BDADDR_LE_RANDOM) {
-		lockdep_assert_held(&conn->hdev->lock);
+		lockdep_assume_ctx_held(&conn->hdev->lock);
 
 		err = smp_user_confirm_reply(conn, mgmt_op, passkey);
 		if (!err)

index 5dac8b2eae42..21640632368b 100644
@@ -2446,7 +2446,7 @@ int smp_cancel_and_remove_pairing(struct hci_dev *hdev, bdaddr_t *bdaddr,
 	if (!hcon)
 		goto done;
 
-	lockdep_assert_held(&hcon->hdev->lock);
+	lockdep_assume_ctx_held(&hcon->hdev->lock);
 
 	conn = hcon->l2cap_data;
 	if (!conn)

futex_exit_recursive() unlocks tsk->futex.exit_mutex if the futex state is FUTEX_STATE_EXITING. It uses __assume_ctx_lock() to inform Clang's context analysis that this mutex is held. However, __assume_ctx_lock() does not verify that assumption. Convert the __assume_ctx_lock() call into a lockdep_assume_ctx_held() call such that the lock ownership assumption is also verified at runtime by lockdep.

---
 kernel/futex/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a240638f69e5..4e5aa0160dd2 100644
@@ -1503,7 +1503,7 @@ void futex_exit_recursive(struct task_struct *tsk)
 {
 	/* If the state is FUTEX_STATE_EXITING then futex_exit_mutex is held */
 	if (tsk->futex.state == FUTEX_STATE_EXITING) {
-		__assume_ctx_lock(&tsk->futex.exit_mutex);
+		lockdep_assume_ctx_held(&tsk->futex.exit_mutex);
 		mutex_unlock(&tsk->futex.exit_mutex);
 	}
 	tsk->futex.state = FUTEX_STATE_DEAD;

Since commit 7c451541743c ("lockdep: Annotate lockdep assertions for context analysis"), lockdep_assert_held(), lockdep_assert_held_write() and lockdep_assert_held_read() instruct Clang's context analysis to assume that a lock is held. This has the following disadvantages:
- lockdep_assert_held*() silences context analysis warnings in functions that lack a __must_hold() or __must_hold_shared() annotation. Hence, the compiler neither reports the missing function annotation nor checks at compile time whether callers hold the lock.
- lockdep_assert_held() makes context analysis assume exclusive lock ownership, even if a reader-writer lock is held in shared mode.
- lockdep_assert_held*() can only be used with lock types that are defined with context_lock_struct().

Hence, remove the context assumptions from lockdep_assert_held*() for both CONFIG_LOCKDEP and !CONFIG_LOCKDEP builds. Code that needs to inform context analysis that a lock is held should use lockdep_assume_ctx_held() or lockdep_assume_shared_ctx_held() instead. Mention this in the kernel-doc comments of these macros.

Convert the __assume_ctx_lock() calls in kernel/locking/rtmutex.c into lockdep_assume_ctx_held() calls such that the lock ownership assumptions are verified at runtime. Remove the lockdep_assert_held(&lock->wait_lock) call from __rt_mutex_slowlock() because lockdep_assume_ctx_held() now performs the same runtime check.

Update Documentation/dev-tools/context-analysis.rst accordingly.

---
 Documentation/dev-tools/context-analysis.rst | 18 ++++++++++++------
 include/linux/lockdep.h                      | 18 ++++++++++++------
 kernel/locking/rtmutex.c                     |  7 +++----
 3 files changed, 27 insertions(+), 16 deletions(-)

index 8e71e1e75b5b..88b4ed6a182f 100644
@@ -111,12 +111,18 @@ checking): either with a ``context_unsafe(var = init)`` expression, or by
 marking small initialization functions with the ``__context_unsafe(init)``
 attribute.
 
-Lockdep assertions, such as `lockdep_assert_held()`, inform the compiler's
-context analysis that the associated synchronization primitive is held after
-the assertion. This avoids false positives in complex control-flow scenarios
-and encourages the use of Lockdep where static analysis is limited. For
-example, this is useful when a function doesn't *always* require a lock, making
-`__must_hold()` inappropriate.
+`lockdep_assume_ctx_held()` and `lockdep_assume_shared_ctx_held()` inform the
+compiler's context analysis that the associated synchronization primitive is
+held and verify this at runtime via Lockdep. This avoids false positives in
+complex control-flow scenarios while still checking lock ownership with Lockdep
+where static analysis is limited. For example, this is useful when a function
+doesn't *always* require a lock, making `__must_hold()` inappropriate.
+
+Lockdep assertions, such as `lockdep_assert_held()`, only perform a runtime
+check and do not inform the compiler's context analysis that a lock is held.
+For context locks that are not tracked by Lockdep, e.g. bit spinlocks, use
+`__assume_ctx_lock()` or `__assume_shared_ctx_lock()` instead. These only
+inform the compiler's context analysis and do not perform a runtime check.
 
 Keywords
 ~~~~~~~~

index f774b22a35c5..ac2f1d2997a1 100644
@@ -285,16 +285,16 @@ extern u32 lock_sequence(struct lockdep_map *lock);
 	do { WARN_ON_ONCE(debug_locks && !(cond)); } while (0)
 
 #define lockdep_assert_held(l)		\
-	do { lockdep_assert(lockdep_is_held(l) != LOCK_STATE_NOT_HELD); __assume_ctx_lock(l); } while (0)
+	lockdep_assert(lockdep_is_held(l) != LOCK_STATE_NOT_HELD)
 
 #define lockdep_assert_not_held(l)	\
 	lockdep_assert(lockdep_is_held(l) != LOCK_STATE_HELD)
 
 #define lockdep_assert_held_write(l)	\
-	do { lockdep_assert(lockdep_is_held_type(l, 0)); __assume_ctx_lock(l); } while (0)
+	lockdep_assert(lockdep_is_held_type(l, 0))
 
 #define lockdep_assert_held_read(l)	\
-	do { lockdep_assert(lockdep_is_held_type(l, 1)); __assume_shared_ctx_lock(l); } while (0)
+	lockdep_assert(lockdep_is_held_type(l, 1))
 
 #define lockdep_assert_held_once(l)		\
 	lockdep_assert_once(lockdep_is_held(l) != LOCK_STATE_NOT_HELD)

@@ -392,10 +392,10 @@ extern int lockdep_is_held(const void *);
 #define lockdep_assert(c)			do { } while (0)
 #define lockdep_assert_once(c)			do { } while (0)
 
-#define lockdep_assert_held(l)			__assume_ctx_lock(l)
+#define lockdep_assert_held(l)			do { (void)(l); } while (0)
 #define lockdep_assert_not_held(l)		do { (void)(l); } while (0)
-#define lockdep_assert_held_write(l)		__assume_ctx_lock(l)
-#define lockdep_assert_held_read(l)		__assume_shared_ctx_lock(l)
+#define lockdep_assert_held_write(l)		do { (void)(l); } while (0)
+#define lockdep_assert_held_read(l)		do { (void)(l); } while (0)
 #define lockdep_assert_held_once(l)		do { (void)(l); } while (0)
 #define lockdep_assert_none_held_once()	do { } while (0)
 

@@ -436,6 +436,9 @@ extern int lockdep_is_held(const void *);
  * has a dep_map member. For context locks that are not tracked by lockdep,
  * e.g. bit spinlocks and the RCU context tokens, use
  * __assume_ctx_lock() instead.
+ *
+ * Note: lockdep_assert_held() does not inform the compiler's context
+ * analysis that a lock is held. Use this macro if that is needed.
  */
 #define lockdep_assume_ctx_held(var)					\
 	_lockdep_assume_ctx_held(var, __UNIQUE_ID(ctx_lock))

@@ -467,6 +470,9 @@ extern int lockdep_is_held(const void *);
  * @var must point at a lock object that is tracked by lockdep, i.e. that
  * has a dep_map member. For context locks that are not tracked by lockdep,
  * e.g. the RCU context tokens, use __assume_shared_ctx_lock() instead.
+ *
+ * Note: lockdep_assert_held_read() does not inform the compiler's context
+ * analysis that a lock is held. Use this macro if that is needed.
  */
 #define lockdep_assume_shared_ctx_held(var)				\
 	_lockdep_assume_shared_ctx_held(var, __UNIQUE_ID(ctx_lock))

index 5a9534c715b8..424cd38a72d2 100644
@@ -1259,7 +1259,7 @@ static int __sched task_blocks_on_rt_mutex(struct rt_mutex_base *lock,
 
 		/* Check whether the waiter should back out immediately */
 		rtm = container_of(lock, struct rt_mutex, rtmutex);
-		__assume_ctx_lock(&rtm->rtmutex.wait_lock);
+		lockdep_assume_ctx_held(&rtm->rtmutex.wait_lock);
 		res = __ww_mutex_add_waiter(waiter, rtm, ww_ctx, wake_q);
 		if (res) {
 			raw_spin_lock(&task->pi_lock);

@@ -1634,7 +1634,7 @@ static int __sched rt_mutex_slowlock_block(struct rt_mutex_base *lock,
 	struct task_struct *owner;
 	int ret = 0;
 
-	__assume_ctx_lock(&rtm->rtmutex.wait_lock);
+	lockdep_assume_ctx_held(&rtm->rtmutex.wait_lock);
 
 	lockevent_inc(rtmutex_slow_block);
 	for (;;) {

@@ -1724,8 +1724,7 @@ static int __sched __rt_mutex_slowlock(struct rt_mutex_base *lock,
 	struct ww_mutex *ww = ww_container_of(rtm);
 	int ret;
 
-	__assume_ctx_lock(&rtm->rtmutex.wait_lock);
-	lockdep_assert_held(&lock->wait_lock);
+	lockdep_assume_ctx_held(&rtm->rtmutex.wait_lock);
 	lockevent_inc(rtmutex_slowlock);
 
 	/* Try to acquire the lock again: */

Split test_rcu_assert_variants() in lib/test_context-analysis.c into test_rcu_assert(), test_rcu_assert_bh() and test_rcu_assert_sched() such that each RCU assertion variant is tested independently. Add test_rcu_assert_reader() to test lockdep_assert_in_rcu_reader().

---
 lib/test_context-analysis.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

index 022891999c9c..048b839bae92 100644
@@ -464,18 +464,30 @@ static void __used test_rcu_lock_reentrant(void)
 	rcu_read_unlock();
 }
 
-static void __used test_rcu_assert_variants(void)
+static void __used test_rcu_assert(void)
 {
 	lockdep_assert_in_rcu_read_lock();
 	wants_rcu_held();
+}
 
+static void __used test_rcu_assert_bh(void)
+{
 	lockdep_assert_in_rcu_read_lock_bh();
 	wants_rcu_held_bh();
+}
 
+static void __used test_rcu_assert_sched(void)
+{
 	lockdep_assert_in_rcu_read_lock_sched();
 	wants_rcu_held_sched();
 }
 
+static void __used test_rcu_assert_reader(void)
+{
+	lockdep_assert_in_rcu_reader();
+	wants_rcu_held();
+}
+
 struct test_srcu_data {
 	struct srcu_struct srcu;
 	long __rcu_guarded *data;

Remove the context assumptions from lockdep_assert_rcu_helper() and from the !CONFIG_PROVE_RCU definitions of lockdep_assert_in_rcu_*() in include/linux/rcupdate.h. Introduce the following macros and document them with kernel-doc comments:
- lockdep_assume_in_rcu_read_lock(): assume RCU and call lockdep_assert_in_rcu_read_lock().
- lockdep_assume_in_rcu_read_lock_bh(): assume RCU and RCU_BH and call lockdep_assert_in_rcu_read_lock_bh().
- lockdep_assume_in_rcu_read_lock_sched(): assume RCU and RCU_SCHED and call lockdep_assert_in_rcu_read_lock_sched().
- lockdep_assume_in_rcu_reader(): assume RCU and call lockdep_assert_in_rcu_reader().
Assume RCU in the _bh() and _sched() variants because rcu_read_lock_bh() and rcu_read_lock_sched() acquire RCU in addition to RCU_BH and RCU_SCHED, and because __rcu_guarded pointers are guarded by RCU. Use the lockdep_assume_ prefix for consistency with lockdep_assume_ctx_held() and lockdep_assume_shared_ctx_held(): __assume_*() macros only inform context analysis while lockdep_assume_*() macros also verify the assumption at runtime.

Mention lockdep_assume_in_rcu_read_lock() in Documentation/dev-tools/context-analysis.rst. Convert the RCU assertion tests in lib/test_context-analysis.c to the lockdep_assume_in_rcu_*() macros and verify in the _bh() and _sched() tests that RCU is assumed too.

Prevent lockdep_assert_in_rcu_*() from silencing Clang context analysis warnings in functions that lack a __must_hold_shared(RCU) annotation, such that the compiler reports missing function annotations and checks callers for RCU read lock ownership at compile time. This makes the RCU assertions consistent with lockdep_assert_held*().

Verify at runtime with lockdep that the RCU read lock is held whenever one of the lockdep_assume_in_rcu_*() macros instructs Clang context analysis to assume RCU read lock ownership without compile-time proof.

Change lockdep_assert_rcu_helper() from __always_inline back into inline. Commit 451c3a035609 ("rcu: Mark lockdep_assert_rcu_helper() __always_inline") made this function __always_inline because if it was not inlined, its callers passed the address of the RCU, RCU_BH or RCU_SCHED context token to it. Since these tokens are not defined anywhere, that resulted in link failures. Now that the ctx argument has been removed from lockdep_assert_rcu_helper(), the context tokens are no longer referenced and hence __always_inline is no longer necessary.

---
 Documentation/dev-tools/context-analysis.rst | 26 ++++---
 include/linux/rcupdate.h                     | 82 +++++++++++++++++---
 lib/test_context-analysis.c                  | 10 ++-
 3 files changed, 92 insertions(+), 26 deletions(-)

index 88b4ed6a182f..bc94e5bb56a5 100644
@@ -111,18 +111,20 @@ checking): either with a ``context_unsafe(var = init)`` expression, or by
 marking small initialization functions with the ``__context_unsafe(init)``
 attribute.
 
-`lockdep_assume_ctx_held()` and `lockdep_assume_shared_ctx_held()` inform the
-compiler's context analysis that the associated synchronization primitive is
-held and verify this at runtime via Lockdep. This avoids false positives in
-complex control-flow scenarios while still checking lock ownership with Lockdep
-where static analysis is limited. For example, this is useful when a function
-doesn't *always* require a lock, making `__must_hold()` inappropriate.
-
-Lockdep assertions, such as `lockdep_assert_held()`, only perform a runtime
-check and do not inform the compiler's context analysis that a lock is held.
-For context locks that are not tracked by Lockdep, e.g. bit spinlocks, use
-`__assume_ctx_lock()` or `__assume_shared_ctx_lock()` instead. These only
-inform the compiler's context analysis and do not perform a runtime check.
+`lockdep_assume_ctx_held()`, `lockdep_assume_shared_ctx_held()` and
+`lockdep_assume_in_rcu_read_lock()` inform the compiler's context analysis that
+the associated synchronization primitive is held and verify this at runtime via
+Lockdep. This avoids false positives in complex control-flow scenarios while
+still checking lock ownership with Lockdep where static analysis is limited.
+For example, this is useful when a function doesn't *always* require a lock,
+making `__must_hold()` inappropriate.
+
+Lockdep assertions, such as `lockdep_assert_held()` and
+`lockdep_assert_in_rcu_read_lock()`, only perform a runtime check and do not
+inform the compiler's context analysis that a lock is held. For context locks
+that are not tracked by Lockdep, e.g. bit spinlocks, use `__assume_ctx_lock()`
+or `__assume_shared_ctx_lock()` instead. These only inform the compiler's
+context analysis and do not perform a runtime check.
 
 Keywords
 ~~~~~~~~

index 048b839bae92..2b5e8612c14e 100644
@@ -466,25 +466,27 @@ static void __used test_rcu_lock_reentrant(void)
 
 static void __used test_rcu_assert(void)
 {
-	lockdep_assert_in_rcu_read_lock();
+	lockdep_assume_in_rcu_read_lock();
 	wants_rcu_held();
 }
 
 static void __used test_rcu_assert_bh(void)
 {
-	lockdep_assert_in_rcu_read_lock_bh();
+	lockdep_assume_in_rcu_read_lock_bh();
+	wants_rcu_held();
 	wants_rcu_held_bh();
 }
 
 static void __used test_rcu_assert_sched(void)
 {
-	lockdep_assert_in_rcu_read_lock_sched();
+	lockdep_assume_in_rcu_read_lock_sched();
+	wants_rcu_held();
 	wants_rcu_held_sched();
 }
 
 static void __used test_rcu_assert_reader(void)
 {
-	lockdep_assert_in_rcu_reader();
+	lockdep_assume_in_rcu_reader();
 	wants_rcu_held();
 }
 

Patches 1-7 are fixes that stand on their own; split them out so they can go in separately.

I'm still 50/50 on this; while it looks better than removing completely, I'm still not convinced by 8-14 esp. in light of my below Coccinelle experiment.

The end state is still inconsistent: rwsem_assert_held*() and lockdep_assert_rq_held() still carry __assumes_ctx_lock(). Either "assert does not assume" is the rule or it is not.

More fundamentally: assert-implies-assume is what Clang's assert_capability was designed for (I mentioned this before). What this series had to do to flip that is 6 missing __must_hold() (patches 5-7), and in exchange ~35 Bluetooth alias workarounds are renamed 1:1. It's the same but under a new name, which results in churn and confusion.

I understand that the goal is finding functions with lockdep_assert_held() that lack __must_hold(), which is strictly speaking a legacy-code problem.

But I believe we can have our cake and eat it too, without changing what the existing API means. Prototype (AI generated) Coccinelle script attached.

On v7.4-rc2:

rwsem.c:396: rwsem_del_waiter() asserts &sem->wait_lock is held but lacks __must_hold(&sem->wait_lock) rwsem.c:437: rwsem_mark_wake() asserts &sem->wait_lock is held but lacks __must_hold(&sem->wait_lock) pi.c:919: fixup_pi_state_owner() asserts q->lock_ptr is held but lacks __must_hold(q->lock_ptr) fabrics.c:650: nvmf_lookup_transport() asserts &nvmf_transports_rwsem is held but lacks __must_hold(&nvmf_transports_rwsem)
[...]

i.e. the functions with an assertion that patches 5-7 annotate. It also reports more, e.g.:

pi.c:51: pi_state_update_owner() asserts &pi_state->pi_mutex.wait_lock is held but lacks __must_hold(&pi_state->pi_mutex.wait_lock)
rtmutex.c:518: rt_mutex_enqueue_pi() asserts &task->pi_lock is held but lacks __must_hold(&task->pi_lock) rtmutex.c:526: rt_mutex_dequeue_pi() asserts &task->pi_lock is held but lacks __must_hold(&task->pi_lock) [...]

It is a prototype (only run on a handful of files, misses annotations on prototypes in headers), but it shows missing annotations can be found without making the APIs worse. Coccinelle was designed for this kind of thing I believe.

If other maintainers prefer the explicit API I won't stand in the way, though.

Thanks,
-- Marco

From 59f9f24bdc0e374a019a76e3bb8469c2cdc85646 Mon Sep 17 00:00:00 2001
From: Marco Elver <[email protected]>
Date: Wed, 11 Nov 2026 10:50:19 +0000
Subject: [PATCH] coccinelle: locks: Add must_hold.cocci

Clang's context analysis treats lockdep_assert_held*() as an assumption that the lock is held. A function that asserts instead of being annotated with __must_hold() therefore passes the analysis, but its callers are never checked.

Add a script that reports functions which assert on all paths that a lock is held, where the lock can be named in a function attribute (it is reachable from a parameter or is a global lock), and which lack a __must_hold*() or __releases*() annotation.

For example, on v7.4-rc2:

kernel/locking/rwsem.c:396:1-20: rwsem_del_waiter() asserts &sem->wait_lock is held but lacks __must_hold(&sem->wait_lock) kernel/locking/rwsem.c:437:1-20: rwsem_mark_wake() asserts &sem->wait_lock is held but lacks __must_hold(&sem->wait_lock) kernel/futex/pi.c:919:1-20: fixup_pi_state_owner() asserts q->lock_ptr is held but lacks __must_hold(q->lock_ptr) drivers/nvme/host/fabrics.c:650:1-20: nvmf_lookup_transport() asserts &nvmf_transports_rwsem is held but lacks __must_hold(&nvmf_transports_rwsem)

Limitations:

- Annotations on a prototype in another file are not seen; non-static functions are reported with "check prototype".
- Annotations are detected textually and are not matched against the asserted lock.
- Functions that require a lock without asserting it are not found.

---
 scripts/coccinelle/locks/must_hold.cocci | 100 +++++++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 scripts/coccinelle/locks/must_hold.cocci

new file mode 100644
index 000000000000..ba45544afc46
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/// Find functions that unconditionally assert with lockdep that a lock is
+/// held, but lack the corresponding __must_hold() annotation. For such
+/// functions the compiler's context analysis trusts the assertion instead of
+/// checking the callers.
+///
+/// Only locks that can be named in a function attribute are reported, i.e.
+/// those reachable from a parameter or a global lock (&lock). Assertions on
+/// locks reached through local aliases are skipped.
+//
+// Confidence: Moderate
+// Comments: Annotations on a prototype in another file are not seen; such
+//	     functions are reported with "check prototype".
+// Options: --no-includes --include-headers
+
+virtual report
+virtual org
+
+@initialize:python@
+@@
+
+import re
+
+def squash(s):
+	return re.sub(r'\s+', '', s)
+
+def missing_must_hold(f, ps, E, pf, p):
+	e = squash(E)
+	m = re.match(r'[&(]*(\w+)', e)
+	if not m:
+		return None
+	params = re.findall(r'(\w+)\s*(?:\[[^\]]*\]\s*)*(?:,|$)', str(ps))
+	if m.group(1) not in params and not re.fullmatch(r'&\w+', e):
+		return None	# Local alias: cannot be named in an attribute.
+
+	with open(pf[0].file, errors='replace') as fp:
+		lines = fp.readlines()
+	first, last = int(pf[0].line), int(p[0].line)
+
+	# Attributes between the declarator and the assertion.
+	hdr = ''.join(lines[first - 1:last - 1])
+	if re.search(r'__(must_hold|must_hold_shared|releases|releases_shared|'
+		     r'context_unsafe|no_context_analysis)\b', hdr):
+		return None
+
+	# Attributes on a prototype in the same file.
+	for d in re.finditer(r'\b%s\s*\([^;{}]*\)([^;{}]*);' % re.escape(f),
+			     ''.join(lines)):
+		if '__must_hold' in d.group(1):
+			return None
+
+	static = 'static' in ''.join(lines[max(first - 3, 0):first])
+	shared = 'lockdep_assert_held_read' in lines[last - 1]
+	return "%s() asserts %s is held but lacks __must_hold%s(%s)%s" % (
+		f, e, "_shared" if shared else "", e,
+		"" if static else "; check prototype")
+
+@r@
+identifier f;
+parameter list ps;
+expression E;
+position pf, p;
+@@
+
+f@pf(ps)
+{
+	...
+(
+	lockdep_assert_held@p(E);
+|
+	lockdep_assert_held_write@p(E);
+|
+	lockdep_assert_held_read@p(E);
+)
+	...
+}
+
+@script:python depends on report@
+f << r.f;
+ps << r.ps;
+E << r.E;
+pf << r.pf;
+p << r.p;
+@@
+
+msg = missing_must_hold(f, ps, E, pf, p)
+if msg:
+	coccilib.report.print_report(p[0], msg)
+
+@script:python depends on org@
+f << r.f;
+ps << r.ps;
+E << r.E;
+pf << r.pf;
+p << r.p;
+@@
+
+msg = missing_must_hold(f, ps, E, pf, p)
+if msg:
+	coccilib.org.print_todo(p[0], msg)

2.56.0.542.g3ef89b7020-goog