linux-kernel - Re: Warning from swake_up_all in 4.14.15-rt13 non-RT

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]

Date:   Fri, 9 Mar 2018 10:03:22 -0600
From:   Corey Minyard <cminyard@...sta.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Steven Rostedt <rostedt@...dmis.org>,
        linux-rt-users <linux-rt-users@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Tejun Heo <tj@...nel.org>
Subject: Re: Warning from swake_up_all in 4.14.15-rt13 non-RT

On 03/09/2018 08:58 AM, Sebastian Andrzej Siewior wrote:
> On 2018-03-09 07:29:31 [-0600], Corey Minyard wrote:
>>  From what I can tell, wake_up_q() is unbounded, and you have undone what
>> the previous code had tried to accomplish.  In the scenario I'm talking
>> about,
>> interrupts are still disabled here.  That's why I was asking about where to
>> put
>> wake_up_q(), I knew you could put it here, but it didn't seem to me to help
>> at all.
> So you are worried about unbound latencies on !RT. Okay. So for !RT this
> does not help but it is not worse then before (before the RT patch was
> applied and changed things).
> In fact it is better now (with RT patch and this one) because before
> that patch you would not only open interrupts between the wake up but you
> would leave the function with interrupts open which is wrong. Any
> interrupt (or a context switch due to need-resched() that would invoke
> percpu_ref_put() would freeze the CPU/system.
> Also, every user that invoked swake_up_all() with enabled interrupts
> will still perform the wake up with enabled interrupts. So nothing
> changes here.

Ah, ok, that makes sense.  Sorry, I was mixing things up.  Yes. on RT 
this should
fix the unbounded time issue, and it should also solve the interrupts 
disabled
issue on !RT.

I'll try this out.

-corey

>>>> I had another idea.  This is only occurring if RT is not enabled, because
>>>> with
>>>> RT all the irq disable things go away and you are generally running in task
>>>> context.  So why not have a different version of swake_up_all() for non-RT
>>>> that does work from irqs-off context?
>>> With the patch above I have puzzle part which would allow to use swait
>>> based completions upstream. That ifdef would probably not help.
>> I agree that having a bounded time way to wake up a bunch of threads while
>> interrupts are disabled would solve a bunch of issues.  I just don't see how
>> it
>> can be done without pushing it off to a softirq or workqueue.
> true but this is a different story. We started with a WARN_ON() which
> triggered correctly and the problem it pointed to looks solved to me.
>
> This "unbounded runtime during the wake up of many tasks with interrupts
> disabled via percpu_ref_kill() -> blk_queue_usage_counter_release()"
> thing exists already in the vanilla kernel and does not exist
> with the RT patch applied and RT enabled. If you are affected by this
> and you don't like it - fine. Using a workqueue is one way of getting
> around it (the softirq is not preemptible in !RT so it wouldn't change
> much). However, I see no benefit in carrying such a patch because as I
> said only !RT is affected by this.
>
>> -corey
> Sebastian