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: <CAADnVQKSL9Vp5ZjQ6k1ZH9Gs6ytj_PEbcDMu+Pa7AxXngF79Jg@mail.gmail.com>
Date: Wed, 2 Apr 2025 17:22:51 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Vlastimil Babka <vbabka@...e.cz>
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 Wed, Apr 2, 2025 at 2:35 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Wed, Apr 2, 2025 at 2:02 AM Vlastimil Babka <vbabka@...e.cz> wrote:
> >
> > On 4/2/25 09:30, Sebastian Andrzej Siewior wrote:
> > > On 2025-03-31 17:51:34 [-0700], Alexei Starovoitov wrote:
> > >> From: Alexei Starovoitov <ast@...nel.org>
> > >>
> > >> Partially revert commit 0aaddfb06882 ("locking/local_lock: Introduce localtry_lock_t").
> > >> Remove localtry_*() helpers, since localtry_lock() name might
> > >> be misinterpreted as "try lock".
> > >
> > > So we back to what you suggested initially. I was more a fan of
> > > explicitly naming things but if this is misleading so be it. So
> > >
> > > Acked-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> > >
> > > While at it, could you look at the hunk below and check if it worth it?
> > > The struct duplication and hoping that the first part remains the same,
> > > is hoping. This still relies that the first part remains the same but…
> >
> > I've updated your fixups to v2
> > https://lore.kernel.org/all/20250401205245.70838-1-alexei.starovoitov@gmail.com/
>
> Sebastian, Vlastimil,
> Thanks for the fixups. Folded.

I was about to send v3, but decided to play with
my stress test a bit more and it caught this race:

[   18.114617] DEBUG_LOCKS_WARN_ON(l->owner)
[   18.114717] WARNING: CPU: 0 PID: 2159 at
include/linux/local_lock_internal.h:46 try_charge_memcg+0x73b/0x7b0
[   18.114733] RIP: 0010:try_charge_memcg+0x73b/0x7b0
[   18.114769]  <NMI>
[   18.114774]  __memcg_kmem_charge_page+0xbe/0x390
[   18.114778]  try_alloc_pages_noprof+0x11e/0x280
[   18.114782]  nmi_callback+0x55/0xc0 [bpf_testmod]
...
[   18.114820]  end_repeat_nmi+0xf/0x18
[   18.114822] RIP: 0010:refill_stock+0xef/0x1e0
[   18.114832]  ? refill_stock+0xef/0x1e0
[   18.114834]  </NMI>
[   18.114834]  <TASK>
[   18.114835]  obj_cgroup_uncharge_pages+0x44/0x170
[   18.114837]  __memcg_kmem_uncharge_page+0x52/0x180
[   18.114839]  __free_frozen_pages+0xce/0x6c0

The same issue is present in the __localtry_unlock*()...
these two should be done in the opposite order:
    WRITE_ONCE(lt->acquired, 0);
    local_lock_release(&lt->llock);

Though IRQs are still disabled at this point
the local_trylock() from NMI will succeed and
local_lock_acquire(l); will warn.

The following fix will be included in v3:

diff --git a/include/linux/local_lock_internal.h
b/include/linux/local_lock_internal.h
index e41ca62fadea..bf2bf40d7b18 100644
--- a/include/linux/local_lock_internal.h
+++ b/include/linux/local_lock_internal.h
@@ -169,13 +169,13 @@ do {
                 \
                                                                        \
                l = (local_lock_t *)this_cpu_ptr(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);            \
                        }),                                             \
                        default:(void)0);                               \
-               local_lock_release(l);                                  \
        } while (0)

After that the stress tester runs cleanly.