linux-kernel - Re: [RFC][PATCH -v2 4/4] locking/mutex: Add lock handoff to avoid starvation

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: <20160830115342.GH10168@twins.programming.kicks-ass.net>
Date:   Tue, 30 Aug 2016 13:53:42 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Waiman Long <waiman.long@....com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Jason Low <jason.low2@....com>,
        Ding Tianhong <dingtianhong@...wei.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Will Deacon <Will.Deacon@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Imre Deak <imre.deak@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Davidlohr Bueso <dave@...olabs.net>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Terry Rudd <terry.rudd@....com>,
        "Paul E. McKenney" <paulmck@...ibm.com>,
        Jason Low <jason.low2@...com>,
        Chris Wilson <chris@...is-wilson.co.uk>,
        Daniel Vetter <daniel.vetter@...ll.ch>
Subject: Re: [RFC][PATCH -v2 4/4] locking/mutex: Add lock handoff to avoid
 starvation

On Mon, Aug 29, 2016 at 05:41:09PM +0200, Peter Zijlstra wrote:
> On Fri, Aug 26, 2016 at 07:40:34PM -0400, Waiman Long wrote:
> > On 08/26/2016 11:18 AM, Peter Zijlstra wrote:
> 
> > >Still need to look at adding spinning to the handoff case.
> > >Also need to look at writing (much) better changelogs, they stink.
> > >
> > 
> > I have looked at the handoff code and I didn't see any problem.
> 
> So I found (or rather the buildbot did) a problem with it.
> 
> locking-selftest has testcases like:
> 
> 
> 	lock(&A);
> 	if (trylock(&A))
> 		/* fail */
> 
>   and
> 
> 	ww_lock(&A)
> 	if (ww_lock(&A) != -EDEADLK)
> 		/* fail */
> 
> But with the 'trylock' accepting the lock if owner==current, in order to
> accept the hand-off, this breaks in interesting ways.
> 
> Now, ARCH_MIN_TASKALIGN is at least 8 (mips, s390, parisc) which would
> give us one more FLAG bit to play with.
> 
> 
> The below seems to make things happy again..

Much simpler solution... only accept handoffs when we're stuck in the
wait loop (which precludes doing recursive locking, since that would've
failed much earlier).

Now, let me look at that spinner patch you sent.

---
--- kernel/locking/mutex.c.mod	2016-08-30 11:08:15.410551744 +0200
+++ kernel/locking/mutex.c	2016-08-30 13:38:30.185550669 +0200
@@ -69,7 +69,7 @@
 /*
  * Actual trylock that will work on any unlocked state.
  */
-static inline bool __mutex_trylock(struct mutex *lock)
+static inline bool __mutex_trylock(struct mutex *lock, const bool handoff)
 {
 	unsigned long owner, curr = (unsigned long)current;
 
@@ -78,8 +78,10 @@
 		unsigned long old;
 
 		if (__owner_task(owner)) {
-			if ((unsigned long)__owner_task(owner) == curr)
+			if (handoff && unlikely(__owner_task(owner) == current)) {
+				smp_mb(); /* ACQUIRE */
 				return true;
+			}
 
 			return false;
 		}
@@ -134,6 +136,10 @@
 	return list_first_entry(&lock->wait_list, struct mutex_waiter, list) == waiter;
 }
 
+/*
+ * Give up ownership to a specific task, when @task = NULL, this is equivalent
+ * to a regular unlock.
+ */
 static void __mutex_handoff(struct mutex *lock, struct task_struct *task)
 {
 	unsigned long owner = atomic_long_read(&lock->owner);
@@ -148,7 +154,7 @@
 		new = (owner & MUTEX_FLAG_WAITERS);
 		new |= (unsigned long)task;
 
-		old = atomic_long_cmpxchg(&lock->owner, owner, new);
+		old = atomic_long_cmpxchg_release(&lock->owner, owner, new);
 		if (old == owner)
 			break;
 
@@ -425,7 +431,7 @@
 			break;
 
 		/* Try to acquire the mutex if it is unlocked. */
-		if (__mutex_trylock(lock)) {
+		if (__mutex_trylock(lock, false)) {
 			osq_unlock(&lock->osq);
 			return true;
 		}
@@ -570,7 +576,12 @@
 	preempt_disable();
 	mutex_acquire_nest(&lock->dep_map, subclass, 0, nest_lock, ip);
 
-	if (__mutex_trylock(lock) || mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx)) {
+	/*
+	 * The first __mutex_trylock() must not accept handoffs, otherwise its
+	 * possible to allow recursive lock attempts by accident.
+	 */
+	if (__mutex_trylock(lock, false) ||
+	    mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx)) {
 		/* got the lock, yay! */
 		lock_acquired(&lock->dep_map, ip);
 		if (use_ww_ctx) {
@@ -588,7 +599,7 @@
 	/*
 	 * Once more, try to acquire the lock.
 	 */
-	if (__mutex_trylock(lock))
+	if (__mutex_trylock(lock, false))
 		goto skip_wait;
 
 	debug_mutex_lock_common(lock, &waiter);
@@ -601,7 +612,7 @@
 	if (__mutex_waiter_is_first(lock, &waiter))
 		__mutex_set_flag(lock, MUTEX_FLAG_WAITERS);
 
-	if (__mutex_trylock(lock))
+	if (__mutex_trylock(lock, false))
 		goto remove_waiter;
 
 	lock_contended(&lock->dep_map, ip);
@@ -629,7 +640,7 @@
 		schedule_preempt_disabled();
 		spin_lock_mutex(&lock->wait_lock, flags);
 
-		if (__mutex_trylock(lock))
+		if (__mutex_trylock(lock, true))
 			break;
 
 		if (__mutex_waiter_is_first(lock, &waiter))
@@ -923,7 +934,7 @@
  */
 int __sched mutex_trylock(struct mutex *lock)
 {
-	bool locked = __mutex_trylock(lock);
+	bool locked = __mutex_trylock(lock, false);
 
 	if (locked)
 		mutex_acquire(&lock->dep_map, 0, 1, _RET_IP_);