Re: [PATCH 00/14] Stop lockdep_assert_held*() from assuming lock ownership

[Marco Elver <elver@google.com>]

[-- Attachment #1: Type: text/plain, Size: 4677 bytes --]

On Mon, Nov 02, 2026 at 01:34PM -0800, Bart Van Assche wrote:
> 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.

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

[-- Attachment #2: 0001-coccinelle-locks-Add-must_hold.cocci.patch --]
[-- Type: text/x-diff, Size: 4598 bytes --]

From 59f9f24bdc0e374a019a76e3bb8469c2cdc85646 Mon Sep 17 00:00:00 2001
From: Marco Elver <elver@google.com>
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.

Signed-off-by: Marco Elver <elver@google.com>
---
 scripts/coccinelle/locks/must_hold.cocci | 100 +++++++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 scripts/coccinelle/locks/must_hold.cocci

diff --git a/scripts/coccinelle/locks/must_hold.cocci b/scripts/coccinelle/locks/must_hold.cocci
new file mode 100644
index 000000000000..ba45544afc46
--- /dev/null
+++ b/scripts/coccinelle/locks/must_hold.cocci
@@ -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