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: <5d6641e3-3e89-465a-aaf5-558dc97a7581@kernel.org>
Date: Wed, 24 Sep 2025 19:26:10 +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 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?

> So the patch won't really fix the paranoid warning.
> I think it's better to fix sparse to silence this warn.

Well, it is not only about the tooling but also about the Linux kernel coding
style. I quote:

  5) namespace collisions when defining local variables in macros resembling
  functions:

	#define FOO(x)                          \
	({                                      \
	        typeof(x) ret;                  \
	        ret = calc_ret(x);              \
	        (ret);                          \
	})

  ret is a common name for a local variable - __foo_ret is less likely to
  collide with an existing variable.

I really do not want to open the debate on whether shadow warnings are useful or
not. The coding style says that we should care about this, let's just follow the
rule.

Your patch is causing noise in the files I am using. So I hope you can find a
solution for the annoyance your are causing me. I do not mind which solution it
is. I am proposing one, if you do not like it, I am also fine if you prefer to
go on a quest to rewrite the coding style and modify the tooling so that we do
not warn anymore about shadowing. But I think we can agree that it is not *my*
job to rewrite the kernel coding style and the tooling to *your* liking.

Yours sincerely,
Vincent Mailhol