linux-kernel - Re: [PATCH] locking/local_lock: fix shadowing in __local_lock

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: <CAADnVQ+xGQGdR85R6EnX7Rrv+RqkrTfLjfdZOYJ4bPb9+Govyg@mail.gmail.com>
Date: Wed, 24 Sep 2025 18:07:23 +0100
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Vincent Mailhol <mailhol@...nel.org>
Cc: Vlastimil Babka <vbabka@...e.cz>, Shakeel Butt <shakeel.butt@...ux.dev>, 
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>, Alexei Starovoitov <ast@...nel.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] locking/local_lock: fix shadowing in __local_lock_acquire()

On Wed, Sep 24, 2025 at 4:07 PM Vincent Mailhol <mailhol@...nel.org> wrote:
>
> On 24/09/2025 at 19:26, Vincent Mailhol wrote:
> > On 24/09/2025 at 04:38, Alexei Starovoitov wrote:
> >> On Tue, Sep 23, 2025 at 7:02 AM Vincent Mailhol <mailhol@...nel.org> wrote:
> >>>
> >>> The __local_lock_acquire() macro uses a local variable named 'l'. This
> >>> being a common name, there is a risk of shadowing other variables.
> >>>
> >>> For example, it is currently shadowing the parameter 'l' of the:
> >>>
> >>>   class_##_name##_t class_##_name##_constructor(_type *l)
> >>>
> >>> function factory from linux/cleanup.h.
> >>>
> >>> Both sparse (with default options) and GCC (with W=2 option) warn
> >>> about this shadowing.
> >>>
> >>> This is a bening warning, but because the issue appears in a header,
> >>> it is spamming whoever is using it. So better to fix to remove some
> >>> noise.
> >>>
> >>> Rename the variable from 'l' to '__lock' (with two underscore prefixes
> >>> as suggested in the Linux kernel coding style [1]) in order to prevent
> >>> the name collision.
> >>
> >> lockdep has __lock as a local variable.
> >
> > OK. I didn't saw this one.
> >
> > But there is a major difference between a shadowing in lockdep.c versus a
> > shadowing in an header: the shadowing warning is local to lockdep.c and does not
> > pollute the other users.
> >
> > My worry is only about the spam created by warnings in headers.
> >
> > Regardless, would renaming to __locked or __l instead of __lock help to address
> > your concern?
>
> __locked was a bad suggestion. It could be named __local_lock (there is already
> a __local_lock() function like macro, but function like macro does not conflict
> with variable names).
>
> But now, my current preference goes to __ll (and also, to keep things
> consistent, __tl for the trylock).

I think s/l/__l/ and s/tl/__tl/ is fine,
but do it for all macros in that file,
since renaming one is fishy. It doesn't fix what
you're claiming to fix, hence the pushback.

Better yet, learn to ignore overly verbose tools.
sparse/checkpatch/gcc are not always correct.