linux-kernel - Re: [PATCH] kthread: Atomically set completion and perform dequeue in __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]

Date:   Wed, 5 Jul 2017 10:21:13 -0700
From:   Vikram Mulukutla <markivx@...eaurora.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     rusty@...tcorp.com.au, tj@...nel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH] kthread: Atomically set completion and perform dequeue in
 __kthread_parkme

On 7/4/2017 9:07 AM, Peter Zijlstra wrote:
> On Mon, Jun 26, 2017 at 03:18:03PM -0700, Vikram Mulukutla wrote:
>>   kernel/kthread.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/kthread.c b/kernel/kthread.c
>> index 26db528..7ad3354 100644
>> --- a/kernel/kthread.c
>> +++ b/kernel/kthread.c
>> @@ -171,9 +171,20 @@ static void __kthread_parkme(struct kthread *self)
>>   {
>>   	__set_current_state(TASK_PARKED);
>>   	while (test_bit(KTHREAD_SHOULD_PARK, &self->flags)) {
>> +		/*
>> +		 * Why the preempt_disable?
>> +		 * Hotplug needs to ensure that 'self' is off of the runqueue
>> +		 * as well, before scheduling the stopper thread that will
>> +		 * migrate tasks off of the runqeue that 'self' was running on.
>> +		 * This avoids unnecessary migration work and also ensures that
>> +		 * kthread_unpark in the cpu_up path doesn't race with
>> +		 * __kthread_parkme.
>> +		 */
>> +		preempt_disable();
>>   		if (!test_and_set_bit(KTHREAD_IS_PARKED, &self->flags))
>>   			complete(&self->parked);
>> +		schedule_preempt_disabled();
> 
> This is broken. schedule_preempt_disable() doesn't guarantee no
> preemptions, just makes it less likely.

Right, the API just informs the scheduler that the calling thread
wishes to have preemption disabled when the API returns. I thought
it was going to guarantee no preemption until the thread is actually
off of the runqueue, but I see the window where an interrupt might
preempt. Doh.

Separate from this hotplug problem, would it be entirely moronic to have
the API disable and enable local interrupts across that short window? I
suppose there's no one that needs this sort of thing so.. no?

> 
>> +		preempt_enable();
>>   		__set_current_state(TASK_PARKED);
>>   	}
>>   	clear_bit(KTHREAD_IS_PARKED, &self->flags);

Thanks,
Vikram