linux-kernel - Re: [PATCH -v3 07/10] mutex: Restructure wait loop

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>] [day] [month] [year] [list]

Message-ID: <20160914103348.GB5016@twins.programming.kicks-ass.net>
Date:   Wed, 14 Sep 2016 12:33:48 +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>
Subject: Re: [PATCH -v3 07/10] mutex: Restructure wait loop

On Fri, Sep 09, 2016 at 04:32:39PM -0400, Waiman Long wrote:

> >I think you need to remove the HANDOFF flag here in the error path too.
> >Maybe you should fix that in patch 6.
> >
> >It is also possible that __mutex_handoff() has been called at this point.
> >So this task may now be the owner of the lock. Maybe you should do a final
> >trylock here.
> 
> Alternatively, we can move the trylock in the main loop after the
> spin_lock_mutex(). In this case, we don't need an extra trylock in the error
> path.

I did something like so..

--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -629,12 +629,12 @@ __mutex_lock_common(struct mutex *lock,
 	if (__mutex_waiter_is_first(lock, &waiter))
 		__mutex_set_flag(lock, MUTEX_FLAG_WAITERS);
 
-	if (__mutex_trylock(lock, false))
-		goto remove_waiter;
-
 	lock_contended(&lock->dep_map, ip);
 
 	for (;;) {
+		if (__mutex_trylock(lock, first))
+			break;
+
 		/*
 		 * got a signal? (This code gets eliminated in the
 		 * TASK_UNINTERRUPTIBLE case.)
@@ -659,13 +659,9 @@ __mutex_lock_common(struct mutex *lock,
 			first = true;
 			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
 		}
-
-		if (__mutex_trylock(lock, first))
-			break;
 	}
 	__set_task_state(task, TASK_RUNNING);
 
-remove_waiter:
 	mutex_remove_waiter(lock, &waiter, task);
 	if (likely(list_empty(&lock->wait_list)))
 		__mutex_clear_flag(lock, MUTEX_FLAGS);