linux-kernel - Re: [PATCH] locking/local_lock, mm: Replace localtry_ helpers with local_trylock

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: <5feaf4c7-4970-4d9b-84a2-fcba2cbe0bc4@suse.cz>
Date: Thu, 3 Apr 2025 11:09:57 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 Linus Torvalds <torvalds@...ux-foundation.org>, bpf <bpf@...r.kernel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 Martin KaFai Lau <martin.lau@...nel.org>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Peter Zijlstra <peterz@...radead.org>, Steven Rostedt <rostedt@...dmis.org>,
 Shakeel Butt <shakeel.butt@...ux.dev>, Michal Hocko <mhocko@...e.com>,
 linux-mm <linux-mm@...ck.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] locking/local_lock, mm: Replace localtry_ helpers with
 local_trylock_t type

On 4/2/25 23:35, Alexei Starovoitov wrote:
> On Wed, Apr 2, 2025 at 2:02 AM Vlastimil Babka <vbabka@...e.cz> wrote:
> 
> This is because the macro specifies the type:
> DEFINE_GUARD(local_lock, local_lock_t __percpu*,
> 
> and that type is used to define two static inline functions
> with that type,
> so by the time our __local_lock_acquire() macro is used
> it sees 'local_lock_t *' and not the actual type of memcg.stock_lock.

Hm but I didn't even try to instantiate any guard. In fact the compilation
didn't even error on compiling my slub.o but earlier in compiling
arch/x86/kernel/asm-offsets.c

I think the problem is rather that the guard creates static inline functions
and _Generic() only works via macros as you pointed out in the reply to Andrew?

I guess it's solvable if we care in the future, but it means more code
duplication - move the _Generic() dispatch outside the whole implementation
to choose between two variants, have guards use use the specific variant
directly without _Generic()?

Or maybe there's a simpler way I'm just not familiar with both the guards
and _Generic() enough.

> Your macro can be hacked with addition of:
> local_lock_t *l = NULL;
> ...
> l = (void *)this_cpu_ptr(lock);
> ...
> tl = (void *)this_cpu_ptr(lock);
> ...
> DEFINE_GUARD(local_lock, void __percpu*,
> 
> then
> guard(local_lock)(&memcg_stock.stock_lock);
> 
> will compile without warnings with both
> typeof(stock_lock) = local_lock_t and local_trylock_t,
> 
> but the generated code will take default:(void)0) path
> and will pass NULL into local_lock_acquire(NULL);
> 
> In other words guard(local_lock) can only support one
> specific type. It cannot be made polymorphic with _Generic() trick.
> This is an unfortunate tradeoff with this approach.
> Thankfully there are no users of it in the tree:
> git grep 'guard(local'|wc -l
> 0
> 
> so I think it's ok that guard(local_lock) can only be used with local_lock_t.