linux-kernel - Re: re. Spurious wakeup on a newly created kthread

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: <YrmcsnHLjadryMSx@dhcp22.suse.cz>
Date:   Mon, 27 Jun 2022 14:04:02 +0200
From:   Michal Hocko <mhocko@...e.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Tejun Heo <tj@...nel.org>, Petr Mladek <pmladek@...e.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>,
        "Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: re. Spurious wakeup on a newly created kthread

On Sat 25-06-22 19:53:34, Linus Torvalds wrote:
> On Sat, Jun 25, 2022 at 6:58 PM Tejun Heo <tj@...nel.org> wrote:
[...]
> > * If there are no true spurious wakeups, where did the racing wakeup
> >   come from? The task just got created w/ TASK_NEW and woken up once
> >   with wake_up_new_task(). It hasn't been on any wait queue or
> >   advertised itself to anything.
> 
> I don't think it was ever a spurious wakeup at all.
> 
> The create_worker() code does:
> 
>         worker->task = kthread_create_on_node(..
>         ..
>         worker_attach_to_pool(worker, pool);
>         ..
>         wake_up_process(worker->task);
> 
> and thinks that the wake_up_process() happens after the worker_attach_to_pool().
> 
> But I don't see that at all.
> 
> The reality seems to be that the wake_up_process() is a complete
> no-op, because the task was already woken up by
> kthread_create_on_node().

Just for the record.
the newly created thread is not running our thread function at this
stage. It is rather subtle and took me some time to decypher but
kthread_create_on_node will create and wake up kernel thread running
kthread() function:
[...]
        /*
         * Thread is going to call schedule(), do not preempt it,
         * or the creator may spend more time in wait_task_inactive().
         */
        preempt_disable();
        complete(done);
        schedule_preempt_disabled();
        preempt_enable();

        ret = -EINTR;
        if (!test_bit(KTHREAD_SHOULD_STOP, &self->flags)) {
                cgroup_kthread_ready();
                __kthread_parkme(self);
                ret = threadfn(data);
        }

so the newly created thread will go into sleep before calling the
threadfn (worker_thread here). Somebody must have woken it up other than
create_worker. I couldn't have found out who that was (see my other
email with some notes from the crash dump).

I do agree that a simple schedule without checking for a condition is
dubious, fragile and wrong. If anything wait_for_completion would be less
confusing and targetted waiting.

Petr has added that completion into worker_thread to address this
specific case and a better fix would be to address all callers because
who knows how many of those are similarly broken.

I also do agree that this whole scheme is rather convoluted and having
an init() callback to be executed before threadfn would be much more
easier to follow.
-- 
Michal Hocko
SUSE Labs