linux-kernel - Re: get_online_cpus() from a preemptible() context (bug?)

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:   Mon, 06 Nov 2017 18:51:35 +0000
From:   James Morse <james.morse@....com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: get_online_cpus() from a  preemptible() context (bug?)

Hi Peter,

(combining your replies)

On 06/11/17 10:32, Peter Zijlstra wrote:
> On Fri, Nov 03, 2017 at 02:45:45PM +0000, James Morse wrote:
>> I'm trying to work out what stops a thread being pre-empted and migrated between
>> calling get_online_cpus() and put_online_cpus().

> Nothing; why would you think it would?

To stop the this_cpu_*() operations in down/up being applied on different CPUs,
affecting a different percpu:read_count.


> All those functions guarantee is
> that any CPU observed as being online says online (and its converse,
> that a CPU observed as being offline, says offline, although less people
> care about that one).


>> According to __percpu_down_read(), its the pre-empt count:
>>>  * Due to having preemption disabled the decrement happens on
>>>  * the same CPU as the increment, avoiding the
>>>  * increment-on-one-CPU-and-decrement-on-another problem.
>>
>>
>> So this:
>>> void cpus_read_lock(void)
>>> {
>>>        percpu_down_read(&cpu_hotplug_lock);
>>> +
>>> +       /* Can we migrated before we release this per-cpu lock? */
>>> +       WARN_ON(preemptible());
>>>  }
>>
>> should never fire?

> It should.. You're reading a comment on __percpu_down_read() and using
> percpu_down_read(), _not_ the same function ;-)

Yes, sorry, I thought you did a better job of describing the case I'm trying to
work-out.


> If you look at percpu_down_read(), you'll note it'll disable preemption
> before calling __percpu_down_read().

Yes, this is how __percpu_down_read() protects the combination of it's fast/slow
paths.

But next percpu_down_read() calls preempt_enable(), I can't see what stops us
migrating before percpu_up_read() preempt_disable()s to call __this_cpu_dec(),
which now affects a different variable.


> And yes, that whole percpu-rwsem code is fairly magical :-)

I think I'll file this under magical. That rcu_sync_is_idle() must know
something I don't!


Thanks!

James