linux-kernel - Re: [PATCH] sched: Avoid that __wait_on_bit

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: <16207b90-2e6c-fe23-1b4b-3763e5cf0384@sandisk.com>
Date:	Fri, 5 Aug 2016 10:41:33 -0700
From:	Bart Van Assche <bart.vanassche@...disk.com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	Oleg Nesterov <oleg@...hat.com>,
	"mingo@...nel.org" <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Johannes Weiner <hannes@...xchg.org>,
	Neil Brown <neilb@...e.de>,
	Michael Shaver <jmshaver@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sched: Avoid that __wait_on_bit_lock() hangs

On 08/04/2016 07:09 AM, Peter Zijlstra wrote:
> On Wed, Aug 03, 2016 at 02:51:23PM -0700, Bart Van Assche wrote:
>> So I started testing the patch below that should fix the same hang but
>> without triggering any wait list corruption.
>>
>> diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
>> index f15d6b6..4e3f651 100644
>> --- a/kernel/sched/wait.c
>> +++ b/kernel/sched/wait.c
>> @@ -282,7 +282,7 @@ void abort_exclusive_wait(wait_queue_head_t *q,
>> wait_queue_t *wait,
>>  	spin_lock_irqsave(&q->lock, flags);
>>  	if (!list_empty(&wait->task_list))
>>  		list_del_init(&wait->task_list);
>> -	else if (waitqueue_active(q))
>> +	if (waitqueue_active(q))
>>  		__wake_up_locked_key(q, mode, key);
>>  	spin_unlock_irqrestore(&q->lock, flags);
>>  }
>
> So the problem with this patch is that it will violate the nr_exclusive
> semantics in that it can result in too many wakeups -- which is a much
> less severe (typically harmless) issue.
>
> We now always wake up the next waiter, even if there wasn't an actual
> wakeup we raced against. And if we then also get a wakeup, we can end up
> with 2 woken tasks (instead of the nr_exclusive=1).
>
> Now, since wait loops must all deal with spurious wakeups, this ends up
> as harmless overhead.

How about adding a fifth argument to abort_exclusive_wait() that 
indicates whether or not the "if (waitqueue_active(q)) 
__wake_up_locked_key(q, mode, key)" code should be executed? 
__wait_event() could pass "condition" as fifth argument when calling 
abort_exclusive_wait().

> But I'd still like to understand where we loose the wakeup.

My assumption is that __wake_up_common() and signal delivery happen 
concurrently, that __wake_up_common() wakes up bit_wait_io() and that 
signal delivery happens after bit_wait_io() has been woken up but before 
it tests the signal pending state.

Bart.