linux-kernel - Re: [PATCH 03/11] locking, rwsem: introduce basis for down_write

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, 11 May 2016 10:35:12 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Michal Hocko <mhocko@...nel.org>
Cc:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
	LKML <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	"David S. Miller" <davem@...emloft.net>,
	Tony Luck <tony.luck@...el.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Chris Zankel <chris@...kel.net>,
	Max Filippov <jcmvbkbc@...il.com>,
	Davidlohr Bueso <dave@...olabs.net>,
	Waiman Long <Waiman.Long@....com>
Subject: Re: [PATCH 03/11] locking, rwsem: introduce basis for
 down_write_killable

On Wed, May 11, 2016 at 09:23:57AM +0200, Michal Hocko wrote:
> On Tue 10-05-16 14:38:06, Peter Zijlstra wrote:

> > Also, looking at it again; I think we're forgetting to re-adjust the
> > BIAS for the cancelled writer.
> 
> Hmm, __rwsem_down_write_failed_common does
> 
> 	/* undo write bias from down_write operation, stop active locking */
> 	count = rwsem_atomic_update(-RWSEM_ACTIVE_WRITE_BIAS, sem);
> 
> which should remove the bias AFAIU.

Right; at this point we're neutral wrt bias.

> Later we do
> 
> 	if (waiting) {
> 		count = READ_ONCE(sem->count);
> 
> 		/*
> 		 * If there were already threads queued before us and there are
> 		 * no active writers, the lock must be read owned; so we try to
> 		 * wake any read locks that were queued ahead of us.
> 		 */
> 		if (count > RWSEM_WAITING_BIAS)
> 			sem = __rwsem_do_wake(sem, RWSEM_WAKE_READERS);
> 
> 	} else
> 		count = rwsem_atomic_update(RWSEM_WAITING_BIAS, sem);
> 
> and that might set RWSEM_WAITING_BIAS but the current holder of the lock
> should handle that correctly and wake the waiting tasks IIUC. I will go
> and check the code closer. It is quite easy to get this subtle code
> wrong..

Subtle; yes.

So if you look at rwsem_try_write_lock() -- traditionally the only way
to exit this wait loop, you see it does:

	if (count == RWSEM_WAITING_BIAS &&
	    cmpxchg_acquire(&sem->count, RWSEM_WAITING_BIAS,
		    RWSEM_ACTIVE_WRITE_BIAS) == RWSEM_WAITING_BIAS) {
		if (!list_is_singular(&sem->wait_list))
			rwsem_atomic_update(RWSEM_WAITING_BIAS, sem);
		rwsem_set_owner(sem);
		return true;
	}

Which ends up clearing RWSEM_WAITING_BIAS is we were the only waiter --
or rather, it always clear WAITING, but then tests the list and re-sets
it if there's more than one waiters on.

Now, the signal break doesn't clear WAITING if we were the only waiter
on the list; which means any further down_read() will block (I didn't
look at what a subsequent down_write() would do).

So I think we needs something like this, to clear WAITING if we leave
the list empty.

Does that make sense?

diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index df4dcb883b50..7011dd1c286c 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -489,6 +489,8 @@ __rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
 		do {
 			if (signal_pending_state(state, current)) {
 				raw_spin_lock_irq(&sem->wait_lock);
+				if (list_singular(&sem->wait_list))
+					rwsem_atomic_update(-RWSEM_WAITING_BIAS, sem);
 				ret = ERR_PTR(-EINTR);
 				goto out;
 			}