| 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: <d83311bc-96b9-4d8e-8a81-4737e303902a@redhat.com>
Date: Wed, 26 Nov 2025 14:19:35 -0500
From: Waiman Long <llong@...hat.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
linux-kernel@...r.kernel.org
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Will Deacon <will@...nel.org>, Vincent Mailhol <mailhol@...nel.org>,
Vlastimil Babka <vbabka@...e.cz>
Subject: Re: [PATCH v3] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce
risk of shadowing
On 10/9/25 6:39 AM, Sebastian Andrzej Siewior wrote:
> From: Vincent Mailhol <mailhol@...nel.org>
>
> The Linux kernel coding style advises to avoid common variable
> names in function-like macros to reduce the risk of collisions.
>
> 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.
>
> [ bigeasy: Rebase, update all l and tl instances in macros ]
>
> Signed-off-by: Vincent Mailhol <mailhol@...nel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> ---
>
> Looks harmless enough to make everyone happy.
>
> v2…v3:
> - Rebase
>
> v1…v2: https://lore.kernel.org/all/20250925-local_lock_internal_fix_shadow-v2-1-d3b85ee775a4@kernel.org/
>
> - __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.
>
> v1: https://lore.kernel.org/r/20250923-local_lock_internal_fix_shadow-v1-1-14e313c88a46@kernel.org
>
> include/linux/local_lock_internal.h | 62 ++++++++++++++---------------
> 1 file changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
> index a4dc479157b5c..8f82b4eb542f2 100644
> --- a/include/linux/local_lock_internal.h
> +++ b/include/linux/local_lock_internal.h
> @@ -99,18 +99,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) \
> @@ -133,36 +133,36 @@ 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; \
> })
>
> /* preemption or migration must be disabled before calling __local_lock_is_locked */
> @@ -170,16 +170,16 @@ do { \
>
> #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)
> @@ -223,12 +223,12 @@ typedef spinlock_t local_trylock_t;
> #define INIT_LOCAL_LOCK(lockname) __LOCAL_SPIN_LOCK_UNLOCKED((lockname))
> #define INIT_LOCAL_TRYLOCK(lockname) __LOCAL_SPIN_LOCK_UNLOCKED((lockname))
>
> -#define __local_lock_init(l) \
> +#define __local_lock_init(__l) \
> do { \
> - local_spin_lock_init((l)); \
> + local_spin_lock_init((__l)); \
> } while (0)
>
> -#define __local_trylock_init(l) __local_lock_init(l)
> +#define __local_trylock_init(__l) __local_lock_init(__l)
>
> #define __local_lock(__lock) \
> do { \
Make sense.
Acked-by: Waiman Long <longman@...hat.com>
Powered by blists - more mailing lists