linux-kernel - Re: [PATCH 2/4] locking/rwsem: Drop superfluous waiter refcount

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: <20160509185118.GC29630@linux-uzut.site>
Date:	Mon, 9 May 2016 11:51:18 -0700
From:	Davidlohr Bueso <dave@...olabs.net>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	mingo@...nel.org, tglx@...utronix.de, Waiman.Long@....com,
	jason.low2@...com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] locking/rwsem: Drop superfluous waiter refcount

On Mon, 09 May 2016, Peter Zijlstra wrote:

>> >So I think you're wrong here; imagine this:
>> >
>> >
>> >	rwsem_down_read_failed()			rwsem_wake()
>> >	  get_task_struct();
>> >	  raw_spin_lock_irq(&wait_lock);
>> >	  list_add_tail(&waiter.list, &wait_list);
>> >	  raw_spin_unlock_irq(&wait_lock);
>> >							  raw_spin_lock_irqsave(&wait_lock)
>> >							  __rwsem_do_wake()
>> >	  while (true) {
>> >	    set_task_state(tsk, TASK_UNINTERRUPTIBLE);
>> >							    waiter->task = NULL
>> >	    if (!waiter.task) // true
>> >	      break;
>> >
>> >	  __set_task_state(tsk, TASK_RUNNING);
>> >
>> >	do_exit();
>> >							    wake_up_process(tsk); /* BOOM */
>>
>> I may be missing something, but rwsem_down_read_failed() will not return until
>> after the wakeup is done by the rwsem_wake() thread.
>
>The above never gets to schedule(), and even if it did, a spurious
>wakeup could've happened, no?

Ah indeed, you are most certainly correct. For some reason  I was always
considering schedule() in the picture. Hmm I'll have to think about this
some more, but given the small chance of a waiter actually seeing the nil
task at the first iteration I'm wondering if we could just invert the code
and call schedule() before the task check. Saving the refcounts will serve
_all_ reader waiters otoh, but this would obviously need numbers...

Thanks,
Davidlohr