Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1756684AbeAHVJ6 (ORCPT <rfc822;ralf@linux-mips.org> + 1 other);
        Mon, 8 Jan 2018 16:09:58 -0500
Received: from mx0a-00010702.pphosted.com ([148.163.156.75]:49822 "EHLO
        mx0b-00010702.pphosted.com" rhost-flags-OK-OK-OK-FAIL)
        by vger.kernel.org with ESMTP id S1751289AbeAHVJy (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 8 Jan 2018 16:09:54 -0500
Date: Mon, 8 Jan 2018 15:09:21 -0600
From: Julia Cartwright <julia@ni.com>
To: Peter Zijlstra <peterz@infradead.org>
CC: Gratian Crisan <gratian.crisan@ni.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        <linux-kernel@vger.kernel.org>, Darren Hart <dvhart@infradead.org>,
        Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH] futex: Avoid violating the 10th rule of futex
Message-ID: <20180108210921.GA1153@jcartwri.amer.corp.natinst.com>
References: <20171129175605.GA863@jcartwri.amer.corp.natinst.com>
 <20171206234622.GZ3326@worktop> <87y3mf8f1j.fsf@ni.com>
 <20171207104516.ljmivyqx7yrthflu@hirez.programming.kicks-ass.net>
 <20171207142648.n4h3vzyajw2zlxv2@hirez.programming.kicks-ass.net>
 <87mv2ur3ew.fsf@kerf.amer.corp.natinst.com>
 <20171207195052.GG18445@jcartwri.amer.corp.natinst.com>
 <87lgieqgz3.fsf@kerf.amer.corp.natinst.com>
 <20171208124939.7livp7no2ov65rrc@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20171208124939.7livp7no2ov65rrc@hirez.programming.kicks-ass.net>
User-Agent: Mutt/1.9.2 (2017-12-15)
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-01-08_12:,,
 signatures=0
X-Proofpoint-Spam-Details: rule=inbound_policy_notspam policy=inbound_policy score=30
 priorityscore=1501 malwarescore=0 suspectscore=2 phishscore=0 bulkscore=0
 spamscore=0 clxscore=1011 lowpriorityscore=0 mlxscore=0 impostorscore=0
 mlxlogscore=999 adultscore=0 classifier=spam adjust=30 reason=mlx
 scancount=1 engine=8.0.1-1711220000 definitions=main-1801080300
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Return-Path: <linux-kernel-owner@vger.kernel.org>

On Fri, Dec 08, 2017 at 01:49:39PM +0100, Peter Zijlstra wrote:
> On Thu, Dec 07, 2017 at 05:02:40PM -0600, Gratian Crisan wrote:
[..]
> Assuming nothing bad happens; find below the patch with a Changelog
> attached.
> 
> ---
> Subject: futex: Avoid violating the 10th rule of futex
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Thu Dec  7 16:54:23 CET 2017
> 
> Julia reported futex state corruption in the following scenario:
> 
>    waiter                                  waker                                            stealer (prio > waiter)
> 
>    futex(WAIT_REQUEUE_PI, uaddr, uaddr2,
>          timeout=[N ms])
>       futex_wait_requeue_pi()
>          futex_wait_queue_me()
>             freezable_schedule()
>             <scheduled out>
>                                            futex(LOCK_PI, uaddr2)
>                                            futex(CMP_REQUEUE_PI, uaddr,
>                                                  uaddr2, 1, 0)
>                                               /* requeues waiter to uaddr2 */
>                                            futex(UNLOCK_PI, uaddr2)
>                                                  wake_futex_pi()
>                                                     cmp_futex_value_locked(uaddr2, waiter)
>                                                     wake_up_q()
>            <woken by waker>
>            <hrtimer_wakeup() fires,
>             clears sleeper->task>
>                                                                                            futex(LOCK_PI, uaddr2)
>                                                                                               __rt_mutex_start_proxy_lock()
>                                                                                                  try_to_take_rt_mutex() /* steals lock */
>                                                                                                     rt_mutex_set_owner(lock, stealer)
>                                                                                               <preempted>
>          <scheduled in>
>          rt_mutex_wait_proxy_lock()
>             __rt_mutex_slowlock()
>                try_to_take_rt_mutex() /* fails, lock held by stealer */
>                if (timeout && !timeout->task)
>                   return -ETIMEDOUT;
>             fixup_owner()
>                /* lock wasn't acquired, so,
>                   fixup_pi_state_owner skipped */
> 
>    return -ETIMEDOUT;
> 
>    /* At this point, we've returned -ETIMEDOUT to userspace, but the
>     * futex word shows waiter to be the owner, and the pi_mutex has
>     * stealer as the owner */
> 
>    futex_lock(LOCK_PI, uaddr2)
>      -> bails with EDEADLK, futex word says we're owner.
> 
> And suggested that what commit:
> 
>   73d786bd043e ("futex: Rework inconsistent rt_mutex/futex_q state")
> 
> removes from fixup_owner() looks to be just what is needed. And indeed
> it is -- I completely missed that requeue_pi could also result in this
> case. So we need to restore that, except that subsequent patches, like
> commit:
> 
>   16ffa12d7425 ("futex: Pull rt_mutex_futex_unlock() out from under hb->lock")
> 
> changed all the locking rules. Even without that, the sequence:
> 
> -               if (rt_mutex_futex_trylock(&q->pi_state->pi_mutex)) {
> -                       locked = 1;
> -                       goto out;
> -               }
> 
> -               raw_spin_lock_irq(&q->pi_state->pi_mutex.wait_lock);
> -               owner = rt_mutex_owner(&q->pi_state->pi_mutex);
> -               if (!owner)
> -                       owner = rt_mutex_next_owner(&q->pi_state->pi_mutex);
> -               raw_spin_unlock_irq(&q->pi_state->pi_mutex.wait_lock);
> -               ret = fixup_pi_state_owner(uaddr, q, owner);
> 
> already suggests there were races; otherwise we'd never have to look
> at next_owner.
> 
> So instead of doing 3 consecutive wait_lock sections with who knows
> what races, we do it all in a single section. Additionally, the usage
> of pi_state->owner in fixup_owner() was only safe because only the
> rt_mutex owner would modify it, which this additional case wrecks.
> 
> Luckily the values can only change away and not to the value we're
> testing, this means we can do a speculative test and double check once
> we have the wait_lock.
> 
> Fixes: 73d786bd043e ("futex: Rework inconsistent rt_mutex/futex_q state")
> Reported-and-Tested-by: Julia Cartwright <julia@ni.com>
> Reported-and-Tested-by: Gratian Crisan <gratian.crisan@ni.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Hey Peter-

We've been running w/ this patch now without further regression.  I was
expecting to see this hit 4.15-rc* eventually, but haven't seen it land
anywhere.  Is this in the queue for 4.16, then?

Thanks!
   Julia