linux-kernel - Re: [PATCH -v4 6/8] locking/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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 17 Oct 2016 12:44:49 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Will Deacon <will.deacon@....com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Waiman Long <waiman.long@....com>,
        Jason Low <jason.low2@....com>,
        Ding Tianhong <dingtianhong@...wei.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        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: [PATCH -v4 6/8] locking/mutex: Restructure wait loop

On Thu, Oct 13, 2016 at 04:17:21PM +0100, Will Deacon wrote:
> Hi Peter,
> 
> I'm struggling to get my head around the handoff code after this change...
> 
> On Fri, Oct 07, 2016 at 04:52:49PM +0200, Peter Zijlstra wrote:
> > --- a/kernel/locking/mutex.c
> > +++ b/kernel/locking/mutex.c
> > @@ -631,13 +631,21 @@ __mutex_lock_common(struct mutex *lock,
> >  
> >  	lock_contended(&lock->dep_map, ip);
> >  
> > +	set_task_state(task, state);
> >  	for (;;) {
> > +		/*
> > +		 * Once we hold wait_lock, we're serialized against
> > +		 * mutex_unlock() handing the lock off to us, do a trylock
> > +		 * before testing the error conditions to make sure we pick up
> > +		 * the handoff.
> > +		 */
> >  		if (__mutex_trylock(lock, first))
> > -			break;
> > +			goto acquired;
> >  
> >  		/*
> > -		 * got a signal? (This code gets eliminated in the
> > -		 * TASK_UNINTERRUPTIBLE case.)
> > +		 * Check for signals and wound conditions while holding
> > +		 * wait_lock. This ensures the lock cancellation is ordered
> > +		 * against mutex_unlock() and wake-ups do not go missing.
> >  		 */
> >  		if (unlikely(signal_pending_state(state, task))) {
> >  			ret = -EINTR;
> > @@ -650,16 +658,27 @@ __mutex_lock_common(struct mutex *lock,
> >  				goto err;
> >  		}
> >  
> > -		__set_task_state(task, state);
> >  		spin_unlock_mutex(&lock->wait_lock, flags);
> >  		schedule_preempt_disabled();
> > -		spin_lock_mutex(&lock->wait_lock, flags);
> >  
> >  		if (!first && __mutex_waiter_is_first(lock, &waiter)) {
> >  			first = true;
> >  			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
> >  		}
> > +
> > +		set_task_state(task, state);
> 
> With this change, we no longer hold the lock wit_hen we set the task
> state, and it's ordered strictly *after* setting the HANDOFF flag.
> Doesn't that mean that the unlock code can see the HANDOFF flag, issue
> the wakeup, but then we come in and overwrite the task state?
> 
> I'm struggling to work out whether that's an issue, but it certainly
> feels odd and is a change from the previous behaviour.

Right, so I think the code is fine, since in that case the
__mutex_trylock() must see the handoff and we'll break the loop and
(re)set the state to RUNNING.

But you're right in that its slightly odd. I'll reorder them and put the
set_task_state() above the !first thing.