linux-kernel - Re: [PATCH v3 0/8] cleanup: Introduce ACQUIRE(), a guard() for conditional locks

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] [day] [month] [year] [list]

Message-ID: <688d5314c0e82_cff991007c@dwillia2-xfh.jf.intel.com.notmuch>
Date: Fri, 1 Aug 2025 16:51:48 -0700
From: <dan.j.williams@...el.com>
To: Nathan Chancellor <nathan@...nel.org>, <dan.j.williams@...el.com>
CC: Andy Shevchenko <andriy.shevchenko@...el.com>,
	<linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Alison Schofield
	<alison.schofield@...el.com>, Dave Jiang <dave.jiang@...el.com>, "David
 Lechner" <dlechner@...libre.com>, Davidlohr Bueso <dave@...olabs.net>, "Fabio
 M. De Francesco" <fabio.m.de.francesco@...ux.intel.com>, Ingo Molnar
	<mingo@...nel.org>, Ira Weiny <ira.weiny@...el.com>, Jonathan Cameron
	<jonathan.cameron@...wei.com>, Linus Torvalds
	<torvalds@...ux-foundation.org>, "Peter Zijlstra (Intel)"
	<peterz@...radead.org>, Shiju Jose <shiju.jose@...wei.com>, Vishal Verma
	<vishal.l.verma@...el.com>
Subject: Re: [PATCH v3 0/8] cleanup: Introduce ACQUIRE(), a guard() for
 conditional locks

Nathan Chancellor wrote:
> On Fri, Aug 01, 2025 at 11:49:22AM -0700, dan.j.williams@...el.com wrote:
> > Andy Shevchenko wrote:
> > > This series broke `make W=1` build vor clang. +Cc Nathan.
> > > 
> > > Par exemple:
> > > 
> > > /kernel/time/posix-timers.c:89:1: error: unused function 'class_lock_timer_lock_err' [-Werror,-Wunused-function]
> > >    89 | DEFINE_CLASS_IS_COND_GUARD(lock_timer);
> > >       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > /include/linux/cleanup.h:376:2: note: expanded from macro 'DEFINE_CLASS_IS_COND_GUARD'
> > >   376 |         __DEFINE_GUARD_LOCK_PTR(_name, _T)
> > >       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > /include/linux/cleanup.h:358:20: note: expanded from macro '__DEFINE_GUARD_LOCK_PTR'
> > >   358 |         static inline int class_##_name##_lock_err(class_##_name##_t *_T)   \
> > >       |                           ^~~~~~~~~~~~~~~~~~~~~~~~
> > > <scratch space>:24:1: note: expanded from here
> > >    24 | class_lock_timer_lock_err
> > >       | ^~~~~~~~~~~~~~~~~~~~~~~~~
> > > 1 error generated.
> > 
> > A few observations:
> > 
> > - This is odd, the inline should have been compiled away if not used.
> > - __always_inline does not help
> > - Seems to go away with __maybe_unused, but that seems more like a
> >   compiler band-aid than a fix
> 
> See commit 6863f5643dd7 ("kbuild: allow Clang to find unused static
> inline functions for W=1 build") for more information on the difference
> between GCC and Clang when it comes to how 'static inline' functions
> behave with -Wunused-function, namely that Clang will warn for functions
> defined in .c files (but not .h files), whereas GCC will not warn for
> either.

Ah, thanks!

> > - This locking pattern is not immediately amenable to the ACQUIRE_ERR()
> >   approach because the unlock object is the return code from the
> >   constructor.
> > 
> > Given all that, and that an ACQUIRE_ERR() would end up being messier
> > than the scoped_timer_get_or_fail() approach, I think the best fix is to
> > quiet the warning, but maybe Peter and Nathan have other ideas?
> 
> Yes, this is what I would recommend, as we never care if this function
> is unused, right?

Hmm, it would be nice to care. For example, if someone did something
like use ACQUIRE() without a corresponding ACQUIRE_ERR(). It would be
nice to catch that, but that warning would only fire for clang builds
where the lock guard is defined in a C file.

> You could probably outright substitute '__maybe_unused' for 'inline'
> in this case, since the compiler is already free to ignore it and the
> attribute takes care of any potential unused warnings, which I think
> 'inline' is primarily used for nowadays.

Oh, true, good point.

Another option is make scoped_cond_guard() use the lock error helper.
The compiler should always elide that __guard_err() call, and in the
meantime maybe catch a true positive unused symbol for
class_##_name##_lock_err.

-- 8< --
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 4eb83dd71cfe..534723b14d92 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -421,13 +421,14 @@ _label:									\
 #define scoped_guard(_name, args...)	\
 	__scoped_guard(_name, __UNIQUE_ID(label), args)
 
-#define __scoped_cond_guard(_name, _fail, _label, args...)		\
-	for (CLASS(_name, scope)(args); true; ({ goto _label; }))	\
-		if (!__guard_ptr(_name)(&scope)) {			\
-			BUILD_BUG_ON(!__is_cond_ptr(_name));		\
-			_fail;						\
-_label:									\
-			break;						\
+#define __scoped_cond_guard(_name, _fail, _label, args...)        \
+	for (CLASS(_name, scope)(args); true; ({ goto _label; })) \
+		if (!__guard_ptr(_name)(&scope) ||                \
+		    __guard_err(_name)(&scope)) {                 \
+			BUILD_BUG_ON(!__is_cond_ptr(_name));      \
+			_fail;                                    \
+_label:                                                           \
+			break;                                    \
 		} else
 
 #define scoped_cond_guard(_name, _fail, args...)	\