| 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: <6c49739d-6f2a-4c49-a04b-9774f10a6925@suse.cz>
Date: Fri, 26 Sep 2025 16:16:56 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Vincent Mailhol <mailhol@...nel.org>,
Shakeel Butt <shakeel.butt@...ux.dev>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Alexei Starovoitov <ast@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>,
Boqun Feng <boqun.feng@...il.com>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce
risk of shadowing
+CC LOCKING PRIMITIVES maintainers. Looks like local_lock files were never
added to the section, should we?
On 9/24/25 20:03, Vincent Mailhol wrote:
> The Linux kernel coding style [1] advises to avoid common variable
> names in function-like macros to reduce the risk of collisions.
I think it would be better if the tools like sparse could recognize if the
shadowing happens inside a macro only and thus really unlikely to cause a
misuse due to confusion (code thinks it's manipulating an outer instance but
instead it's the inner one), because macros in their definition would never
intend to manipulate a possible outer instance, right? Or are there any
other problems due to shadowing besides this risk?
> Throughout local_lock_internal.h, several macros use the rather common
> variable names 'l' and 'tl'. This already resulted in an actual
> collision: the __local_lock_acquire() function like macro is currently
> shadowing the parameter 'l' of the:
>
> class_##_name##_t class_##_name##_constructor(_type *l)
>
> function factory from linux/cleanup.h.
>
> Rename the variable 'l' to '__l' and the variable 'tl' to '__tl'
> throughout the file to fix the current name collision and to prevent
> future ones.
>
> [1] https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
>
> Signed-off-by: Vincent Mailhol <mailhol@...nel.org>
That said I don't oppose the change, but not my call.
> ---
> Changes in v2:
>
> - __lock conflicted with an existing definition in lockdep.c. Use
> instead __l (and also, to keep things consistent, use __tl instead
> of tl for the trylock).
>
> - Apply the renaming to the entire file and not just to
> __local_lock_acquire().
>
> - Rewrite the patch description accordingly.
>
> Link to v1: https://lore.kernel.org/r/20250923-local_lock_internal_fix_shadow-v1-1-14e313c88a46@kernel.org
> ---
> include/linux/local_lock_internal.h | 56 ++++++++++++++++++-------------------
> 1 file changed, 28 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
> index d80b5306a2c0ccf95a3405b6b947b5f1f9a3bd38..a80b3fd7552376cd926aeaac8f53bcc44c8d2173 100644
> --- a/include/linux/local_lock_internal.h
> +++ b/include/linux/local_lock_internal.h
> @@ -96,18 +96,18 @@ do { \
>
> #define __local_lock_acquire(lock) \
> do { \
> - local_trylock_t *tl; \
> - local_lock_t *l; \
> + local_trylock_t *__tl; \
> + local_lock_t *__l; \
> \
> - l = (local_lock_t *)(lock); \
> - tl = (local_trylock_t *)l; \
> + __l = (local_lock_t *)(lock); \
> + __tl = (local_trylock_t *)__l; \
> _Generic((lock), \
> local_trylock_t *: ({ \
> - lockdep_assert(tl->acquired == 0); \
> - WRITE_ONCE(tl->acquired, 1); \
> + lockdep_assert(__tl->acquired == 0); \
> + WRITE_ONCE(__tl->acquired, 1); \
> }), \
> local_lock_t *: (void)0); \
> - local_lock_acquire(l); \
> + local_lock_acquire(__l); \
> } while (0)
>
> #define __local_lock(lock) \
> @@ -130,50 +130,50 @@ do { \
>
> #define __local_trylock(lock) \
> ({ \
> - local_trylock_t *tl; \
> + local_trylock_t *__tl; \
> \
> preempt_disable(); \
> - tl = (lock); \
> - if (READ_ONCE(tl->acquired)) { \
> + __tl = (lock); \
> + if (READ_ONCE(__tl->acquired)) { \
> preempt_enable(); \
> - tl = NULL; \
> + __tl = NULL; \
> } else { \
> - WRITE_ONCE(tl->acquired, 1); \
> + WRITE_ONCE(__tl->acquired, 1); \
> local_trylock_acquire( \
> - (local_lock_t *)tl); \
> + (local_lock_t *)__tl); \
> } \
> - !!tl; \
> + !!__tl; \
> })
>
> #define __local_trylock_irqsave(lock, flags) \
> ({ \
> - local_trylock_t *tl; \
> + local_trylock_t *__tl; \
> \
> local_irq_save(flags); \
> - tl = (lock); \
> - if (READ_ONCE(tl->acquired)) { \
> + __tl = (lock); \
> + if (READ_ONCE(__tl->acquired)) { \
> local_irq_restore(flags); \
> - tl = NULL; \
> + __tl = NULL; \
> } else { \
> - WRITE_ONCE(tl->acquired, 1); \
> + WRITE_ONCE(__tl->acquired, 1); \
> local_trylock_acquire( \
> - (local_lock_t *)tl); \
> + (local_lock_t *)__tl); \
> } \
> - !!tl; \
> + !!__tl; \
> })
>
> #define __local_lock_release(lock) \
> do { \
> - local_trylock_t *tl; \
> - local_lock_t *l; \
> + local_trylock_t *__tl; \
> + local_lock_t *__l; \
> \
> - l = (local_lock_t *)(lock); \
> - tl = (local_trylock_t *)l; \
> - local_lock_release(l); \
> + __l = (local_lock_t *)(lock); \
> + __tl = (local_trylock_t *)__l; \
> + local_lock_release(__l); \
> _Generic((lock), \
> local_trylock_t *: ({ \
> - lockdep_assert(tl->acquired == 1); \
> - WRITE_ONCE(tl->acquired, 0); \
> + lockdep_assert(__tl->acquired == 1); \
> + WRITE_ONCE(__tl->acquired, 0); \
> }), \
> local_lock_t *: (void)0); \
> } while (0)
>
> ---
> base-commit: cec1e6e5d1ab33403b809f79cd20d6aff124ccfe
> change-id: 20250923-local_lock_internal_fix_shadow-2e2a24e95e76
>
> Best regards,
Powered by blists - more mailing lists