linux-kernel - Re: [PATCH] lib/klist: Remove mb() before wake_up

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: <CA+tQmHAJbqDenRE47OacSurF5HZ-XWHu6dRBf+A=UqbhiLomAA@mail.gmail.com>
Date:   Wed, 15 Jun 2022 11:30:51 +0800
From:   chi wu <wuchi.zero@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     alexios.zavras@...el.com, allison@...utok.net, armijn@...ldur.nl,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lib/klist: Remove mb() before wake_up_process

Greg KH <gregkh@...uxfoundation.org> 于2022年6月14日周二 22:58写道：
>
> On Tue, Jun 14, 2022 at 10:44:43PM +0800, wuchi wrote:
> > Function wake_up_process always executes a general memory barrier,
> > so remove the mb() before it.
>
> Really?  On all systems?  I do not see that, where does it happen?
>
As I understand it, it is on all systems.  Please help correct the
mistake, thanks.

1. Follow  Documentation/memory-barriers.txt  line 2128 ~ 2278,
especially line 2187 ~ 2202 snippet:
        A general memory barrier is executed by wake_up() if it wakes
something up.
        If it doesn't wake anything up then a memory barrier may or may not be
        executed; you must not rely on it. The barrier occurs before
the task state
        is accessed, in particular, it sits between the STORE to
indicate the event
        and the STORE to set TASK_RUNNING:
         CPU 1 (Sleeper)                                       CPU 2 (Waker)
        =============================== ===============================
        set_current_state();                                   STORE
event_indicated
            smp_store_mb();                                   wake_up();
                STORE current->state                           ...
                <general barrier>
<general barrier>
[*1-1*]
         LOAD event_indicated                                  if
((LOAD task->state) & TASK_NORMAL)

            STORE task->state
        where "task" is the thread being woken up and it equals CPU
1's "current".

2. Follow code wake_up_process in kernel/sched/core.c
    wake_up_process
        try_to_wake_up
            ....
            raw_spin_lock_irqsave                   [*2-1*]
            smp_mb__after_spinlock                [*2-2*]
            ....

[*2-1*] and [*2-2*] will match [*1-1*], though smp_mb__after_spinlock
does nothing on most architectures,
but the architectures implement ACQUIRE with an smp_mb() after the
LL/SC loop, Following include/linux/spinlock.h
line 172 ~ 178.

3. Following lib/klist.c
klist_release and klist_remove conform to model "SLEEP AND WAKE-UP
FUNCTIONS" in  Documentation/memory-barriers.txt,
so we do as the patch show.

> > Signed-off-by: wuchi <wuchi.zero@...il.com>
>
> We need a "real name" for commits.
>
> How did you test this patch?

Sorry, I didn't. Just found the mb before wake_up_process could be
remove when reading the code,
Maybe you can view this as a question I asked.

thanks for your time