linux-kernel - Re: [PATCH] stop_machine: Disable preemption after queueing stopper threads

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: <109d0e70606ccd34861a80525d6d11aa@codeaurora.org>
Date:   Mon, 30 Jul 2018 10:12:43 -0700
From:   Sodagudi Prasad <psodagud@...eaurora.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        isaacm@...eaurora.org, matt@...eblueprint.co.uk, mingo@...nel.org,
        linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        pkondeti@...eaurora.org, stable@...r.kernel.org
Subject: Re: [PATCH] stop_machine: Disable preemption after queueing stopper
 threads

On 2018-07-30 05:41, Thomas Gleixner wrote:
> On Mon, 30 Jul 2018, Peter Zijlstra wrote:
> 
>> On Mon, Jul 30, 2018 at 12:20:57PM +0200, Thomas Gleixner wrote:
>> > On Tue, 24 Jul 2018, Sebastian Andrzej Siewior wrote:
>> > > On 2018-07-23 18:13:48 [-0700], isaacm@...eaurora.org wrote:
>> > > > Hi all,
>> > > Hi,
>> > >
>> > > > Are there any comments about this patch?
>> > >
>> > > I haven't look in detail at this but your new preempt_disable() makes
>> > > things unbalanced for the err != 0 case.
>> >
>> > It doesn't but that code is really an unreadable pile of ...
>> 
>> ---
>> Subject: stop_machine: Reflow cpu_stop_queue_two_works()
>> 
>> The code flow in cpu_stop_queue_two_works() is a little arcane; fix
>> this by lifting the preempt_disable() to the top to create more 
>> natural
>> nesting wrt the spinlocks and make the wake_up_q() and 
>> preempt_enable()
>> unconditional at the end.
>> 
>> Furthermore, enable preemption in the -EDEADLK case, such that we
>> spin-wait with preemption enabled.
>> 
>> Suggested-by: Thomas Gleixner <tglx@...utronix.de>
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> 
Hi Peter/Thomas,

How about including below change as well?  Currently, there is no way to 
identify thread migrations completed or not.  When we observe this 
issue, the symptom was work queue lock up. It is better to have some 
timeout here and induce the bug_on.

There is no way to identify the migration threads stuck or not.

--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -290,6 +290,7 @@ int stop_two_cpus(unsigned int cpu1, unsigned int 
cpu2, cpu_stop_fn_t fn, void *
         struct cpu_stop_done done;
         struct cpu_stop_work work1, work2;
         struct multi_stop_data msdata;
+       int ret;

         msdata = (struct multi_stop_data){
                 .fn = fn,
@@ -312,7 +313,10 @@ int stop_two_cpus(unsigned int cpu1, unsigned int 
cpu2, cpu_stop_fn_t fn, void *
         if (cpu_stop_queue_two_works(cpu1, &work1, cpu2, &work2))
                 return -ENOENT;

-       wait_for_completion(&done.completion);
+       ret = wait_for_completion_timeout(&done.completion,  
msecs_to_jiffies(1000));
+       if (!ret)
+               BUG_ON(1);
+


> Thanks for cleaning that up!
> 
> Reviewed-by: Thomas Gleixner <tglx@...utronix.de>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project