linux-kernel - Re: [PATCH -v6 04/13] futex,rt_mutex: Provide futex specific rt

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: <20170406121728.h3lj2fg4bbst4663@hirez.programming.kicks-ass.net>
Date:   Thu, 6 Apr 2017 14:17:28 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Darren Hart <dvhart@...radead.org>
Cc:     tglx@...utronix.de, mingo@...nel.org, juri.lelli@....com,
        rostedt@...dmis.org, xlpang@...hat.com, bigeasy@...utronix.de,
        linux-kernel@...r.kernel.org, mathieu.desnoyers@...icios.com,
        jdesfossez@...icios.com, bristot@...hat.com
Subject: Re: [PATCH -v6 04/13] futex,rt_mutex: Provide futex specific
 rt_mutex API

On Wed, Apr 05, 2017 at 08:02:17AM -0700, Darren Hart wrote:
> > @@ -1364,20 +1364,18 @@ static int wake_futex_pi(u32 __user *uad
> >  	pi_state->owner = new_owner;
> >  	raw_spin_unlock(&new_owner->pi_lock);
> >  
> >  	/*
> > +	 * We've updated the uservalue, this unlock cannot fail.
> 
> It isn't clear to me what I should understand from this new comment. How does
> the value of the uval affect whether or not the pi_state->pi_mutex can be
> unlocked or not? Or are you noting that we've set FUTEX_WAITIERS so any valid
> userspace operations will be forced intot he kernel and can't race with us since
> we hold the hb->lock? With futexes, I think it's important that we be very
> explicit in our comment blocks.

The critical point is that once you've modified uval we must not fail;
there is no way to undo things thereafter.

> >  	 */
> > +	deboost = __rt_mutex_futex_unlock(&pi_state->pi_mutex, &wake_q);
> > +
> > +	raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock);
> >  	spin_unlock(&hb->lock);
> > +
> > +	if (deboost) {
> > +		wake_up_q(&wake_q);
> 
> Is moving wake_up_q under deboost related to this change or is it just an
> optimization since there is no need to wake unless we are deboosting ourselves -
> which was true before as well?
> 
> If this is due to the rt_mutex_futex* API, I haven't made the connection.

It's how rt_mutex does wakeups, note that later patches clean this up.