linux-kernel - Re: [RFC PATCH v2] Simply enable one to write code like:

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZvwTZxN1F6X6Wd2i@google.com>
Date: Tue, 1 Oct 2024 08:21:11 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: linux-kernel@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
	amadeuszx.slawinski@...ux.intel.com,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	nex.sw.ncis.osdt.itp.upstreaming@...el.com, netdev@...r.kernel.org,
	Markus Elfring <Markus.Elfring@....de>,
	Dan Carpenter <dan.carpenter@...aro.org>,
	Andy Shevchenko <andriy.shevchenko@...el.com>
Subject: Re: [RFC PATCH v2] Simply enable one to write code like:

Hi Przemek,

On Tue, Oct 01, 2024 at 04:57:18PM +0200, Przemek Kitszel wrote:
> int foo(struct my_drv *adapter)
> {
> 	scoped_guard(spinlock, &adapter->some_spinlock)
> 		return adapter->spinlock_protected_var;
> }

Could you change the subject to say something like:

"Adjust cond_guard() implementation to avoid potential warnings"

And then give detailed explanation in the body?

> 
> Current scoped_guard() implementation does not support that,
> due to compiler complaining:
> error: control reaches end of non-void function [-Werror=return-type]
> 
> One could argue that for such use case it would be better to use
> guard(spinlock)(&adapter->some_spinlock), I disagree. I could also say
> that coding with my proposed locking style is also very pleasant, as I'm
> doing so for a few weeks already.

I'd drop this paragraph from the patch description (and moved past "---"
if you prefer to keep it for additional context.

> 
> Technical stuff about the change:
> scoped_guard() macro uses common idiom of using "for" statement to declare
> a scoped variable. Unfortunately, current logic is too hard for compiler
> diagnostics to be sure that there is exactly one loop step; fix that.
> 
> To make any loop so trivial that there is no above warning, it must not
> depend on any variable to tell if there are more steps. There is no
> obvious solution for that in C, but one could use the compound statement
> expression with "goto" jumping past the "loop", effectively leaving only
> the subscope part of the loop semantics.
> 
> More impl details:
> one more level of macro indirection is now needed to avoid duplicating
> label names;
> I didn't spot any other place that is using
> 	if (0) past_the_loop:; else for (...; 1; ({goto past_the_loop}))
> idiom, so it's not packed for reuse what makes actual macros code cleaner.
> 
> There was also a need to introduce const 0/1 variable per lock class, it
> is used to aid compiler diagnostics reasoning about "exactly 1 step" loops
> (note that converting that to function would undo the whole benefit).
> 
> NAKed-by: Andy Shevchenko <andriy.shevchenko@...el.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> ---
> Andy believes that this change is completely wrong C, the reasons
> (that I disagree with of course, are in v1, below the commit message).
> 
> v2:
>  remove ", 1" condition, as scoped_guard() could be used also for
>  conditional locks (try-lock, irq-lock, etc) - this was pointed out by
>  Dmitry Torokhov and Dan Carpenter;
>  reorder macros to have them defined prior to use - Markus Elfring.
> 
> v1:
> https://lore.kernel.org/netdev/20240926134347.19371-1-przemyslaw.kitszel@intel.com
> ---
>  include/linux/cleanup.h | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index a3d3e888cf1f..72dcfeb3ec13 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -151,12 +151,18 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>   *
>   */
>  
> +
> +#define DEFINE_CLASS_IS_CONDITIONAL(_name, _is_cond)	\

This is not supposed to be used outside of cleanup.h so probably
__DEFINE_CLASS_IS_CONDITIONAL()?
> +static __maybe_unused const bool class_##_name##_is_conditional = _is_cond
> +
>  #define DEFINE_GUARD(_name, _type, _lock, _unlock) \
> +	DEFINE_CLASS_IS_CONDITIONAL(_name, 0); \
>  	DEFINE_CLASS(_name, _type, if (_T) { _unlock; }, ({ _lock; _T; }), _type _T); \
>  	static inline void * class_##_name##_lock_ptr(class_##_name##_t *_T) \
>  	{ return *_T; }
>  
>  #define DEFINE_GUARD_COND(_name, _ext, _condlock) \
> +	DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, 1); \
>  	EXTEND_CLASS(_name, _ext, \
>  		     ({ void *_t = _T; if (_T && !(_condlock)) _t = NULL; _t; }), \
>  		     class_##_name##_t _T) \
> @@ -167,10 +173,18 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>  	CLASS(_name, __UNIQUE_ID(guard))
>  
>  #define __guard_ptr(_name) class_##_name##_lock_ptr
> +#define __is_cond_ptr(_name) class_##_name##_is_conditional
> +
> +#define scoped_guard(_name, args...)	\
> +	__scoped_guard_labeled(__UNIQUE_ID(label), _name, args)
>  
> -#define scoped_guard(_name, args...)					\
> -	for (CLASS(_name, scope)(args),					\
> -	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
> +#define __scoped_guard_labeled(_label, _name, args...)	\
> +	if (0)						\
> +		_label: ;				\
> +	else						\
> +		for (CLASS(_name, scope)(args);		\
> +		     __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name); \
> +		     ({goto _label;}))

The "jump back" throws me a little, do you think if can be rewritten as:

	if (true)
		for (...)
	else
		_label: /* dummy */ ;

>  
>  #define scoped_cond_guard(_name, _fail, args...) \
>  	for (CLASS(_name, scope)(args), \

With your __is_cond_ptr() can this be made to warn or error if
scoped_cond_guard() is used with a non-conditional lock/class? As that
would make no sense.

> @@ -233,14 +247,17 @@ static inline class_##_name##_t class_##_name##_constructor(void)	\
>  }
>  
>  #define DEFINE_LOCK_GUARD_1(_name, _type, _lock, _unlock, ...)		\
> +DEFINE_CLASS_IS_CONDITIONAL(_name, 0);					\
>  __DEFINE_UNLOCK_GUARD(_name, _type, _unlock, __VA_ARGS__)		\
>  __DEFINE_LOCK_GUARD_1(_name, _type, _lock)
>  
>  #define DEFINE_LOCK_GUARD_0(_name, _lock, _unlock, ...)			\
> +DEFINE_CLASS_IS_CONDITIONAL(_name, 0);					\
>  __DEFINE_UNLOCK_GUARD(_name, void, _unlock, __VA_ARGS__)		\
>  __DEFINE_LOCK_GUARD_0(_name, _lock)
>  
>  #define DEFINE_LOCK_GUARD_1_COND(_name, _ext, _condlock)		\
> +	DEFINE_CLASS_IS_CONDITIONAL(_name##_ext, 1);			\
>  	EXTEND_CLASS(_name, _ext,					\
>  		     ({ class_##_name##_t _t = { .lock = l }, *_T = &_t;\
>  		        if (_T->lock && !(_condlock)) _T->lock = NULL;	\
> 
> base-commit: c824deb1a89755f70156b5cdaf569fca80698719
> -- 
> 2.39.3
> 

Thanks.

-- 
Dmitry