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

Message-ID: <20250801190203.GA939298@ax162>
Date: Fri, 1 Aug 2025 12:02:03 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: 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

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.

> - 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? 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.

Cheers,
Nathan

> -- 8< --
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index 4eb83dd71cfe..0dc7148d1b88 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -348,7 +348,8 @@ static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
>  		}                                                           \
>  		return _ptr;                                                \
>  	}                                                                   \
> -	static inline int class_##_name##_lock_err(class_##_name##_t *_T)   \
> +	static __maybe_unused inline int class_##_name##_lock_err(          \
> +		class_##_name##_t *_T)                                      \
>  	{                                                                   \
>  		long _rc = (__force unsigned long)*(_exp);                  \
>  		if (!_rc) {                                                 \