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]

Date:	Wed, 3 Aug 2016 14:51:23 -0700
From:	Bart Van Assche <bart.vanassche@...disk.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	Peter Zijlstra <peterz@...radead.org>,
	"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/03/2016 02:30 PM, Oleg Nesterov wrote:
> On 08/03, Bart Van Assche wrote:
>> try_to_wake_up() locks task_struct.pi_lock but abort_exclusive_wait() not.
>> My assumption is that the following sequence of events leads to the lockup
>> that I had mentioned in the description of my patch:
>> * try_to_wake_up() is called for the task that will execute
>>   abort_exclusive_wait().
>> * After try_to_wake_up() has checked task_struct.state and before
>>   autoremove_wake_function() has tried to remove the task from the wait
>>   queue, abort_exclusive_wait() is executed for the same task.
>
> But we do not care if we race with another try_to_wake_up(), or even with
> another exclusive wake_up_nr(wq)/whatever unless wq is the same.
>
> And if this wq is the same, then wake_up_nr() will do try_to_wake_up/autoremove
> either before or after abort_exclusive_wait(), wake_up_nr() takes the same
> wq->lock.
>
> And this means that abort_exclusive_wait() can't be called "After try_to_wake_up()"
> and "before autoremove_wake_function()".

Hello Oleg,

It is possible that my analysis is wrong. But what I see is that my 
patch makes the lockup disappear. However, what I had not expected is 
that I ran into the following (probably caused by the patch at the start 
of this thread):

WARNING: CPU: 1 PID: 26023 at lib/list_debug.c:33 __list_add+0x89/0xb0
list_add corruption. prev->next should be next (ffff88047ff4b0c8), but 
was ffff8803cbc037e0. (prev=ffff8803c863bd80).
Call Trace:
  [<ffffffff81320137>] dump_stack+0x68/0xa1
  [<ffffffff81061c46>] __warn+0xc6/0xe0
  [<ffffffff81061caa>] warn_slowpath_fmt+0x4a/0x50
  [<ffffffff8133d5d9>] __list_add+0x89/0xb0
  [<ffffffff810ab5c9>] prepare_to_wait_exclusive+0x79/0x80
  [<ffffffff8161fa2f>] __wait_on_bit_lock+0x2f/0xa0
  [<ffffffff8114fe89>] __lock_page+0xb9/0xc0
  [<ffffffff81165db0>] truncate_inode_pages_range+0x3e0/0x760
  [<ffffffff81166140>] truncate_inode_pages+0x10/0x20
  [ ... ]

So I started testing the patch below that should fix the same hang but 
without triggering any wait list corruption.

Bart.

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);
  }