| 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
| ||
|
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
Powered by blists - more mailing lists