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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160509155607.GB29630@linux-uzut.site>
Date:	Mon, 9 May 2016 08:56:07 -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:

>On Sun, May 08, 2016 at 09:56:08PM -0700, Davidlohr Bueso wrote:
>> Read waiters are currently reference counted from the time it enters
>> the slowpath until the lock is released and the waiter is awoken. This
>> is fragile and superfluous considering everything occurs within down_read()
>> without returning to the caller, and the very nature of the primitive does
>> not suggest that the task can disappear from underneath us. In addition,
>> spurious wakeups can make the whole refcount useless as get_task_struct()
>> is only called when setting up the waiter.
>
>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. So racing with do_exit() isn't
going to occur because the task is still blocked at that point. This is even more
so with delaying the wakeup. Similarly, we don't do this for writers either, which
could also suffer from similar scenarios.

Thanks,
Davidlohr