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] [day] [month] [year] [list]

Message-ID: <90cecd21-3ad2-4c21-a0d3-4b30a79d4eac@kernel.org>
Date: Thu, 25 Sep 2025 03:04:21 +0900
From: Vincent Mailhol <mailhol@...nel.org>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
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 25/09/2025 at 02:07, Alexei Starovoitov wrote:
> 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.

OK. This was a fair pushback :)

I took your suggestion and renamed all instances in the file. I just sent the v2.

Thanks!


Yours sincerely,
Vincent Mailhol