From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751455AbeAVKj4 (ORCPT ); Mon, 22 Jan 2018 05:39:56 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:37279 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750828AbeAVKjy (ORCPT ); Mon, 22 Jan 2018 05:39:54 -0500 Date: Mon, 22 Jan 2018 11:39:47 +0100 From: Peter Zijlstra To: Geert Uytterhoeven Cc: Ingo Molnar , Linus Torvalds , Linux Kernel Mailing List , Thomas Gleixner , "Paul E. McKenney" , Andrew Morton Subject: Re: [GIT PULL] locking fixes Message-ID: <20180122103947.GD2228@hirez.programming.kicks-ass.net> References: <20180117152416.eazu647rhjvy4rw6@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 22, 2018 at 10:43:36AM +0100, Geert Uytterhoeven wrote: > > static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, > > + struct task_struct *argowner) > > { > > struct futex_pi_state *pi_state = q->pi_state; > > u32 uval, uninitialized_var(curval), newval; > > + struct task_struct *oldowner, *newowner; > > + u32 newtid; > > new tid is no longer initialized... > > > int ret; > > > > + lockdep_assert_held(q->lock_ptr); > > + > > raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock); > > > > oldowner = pi_state->owner; > > @@ -2317,11 +2316,17 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, > > newtid |= FUTEX_OWNER_DIED; > > ... leading to a compiler warning with gcc 4.1.2: > > warning: ‘newtid’ is used uninitialized in this function > > I guess newer compilers don't give the warning, as the result of the > assignment above is not used at all, and thus may be optimized away... > > > > > /* > > + * We are here because either: > > + * > > + * - we stole the lock and pi_state->owner needs updating to reflect > > + * that (@argowner == current), > > + * > > + * or: > > + * > > + * - someone stole our lock and we need to fix things to point to the > > + * new owner (@argowner == NULL). > > * > > + * Either way, we have to replace the TID in the user space variable. > > * This must be atomic as we have to preserve the owner died bit here. > > * > > * Note: We write the user space value _before_ changing the pi_state > > @@ -2334,6 +2339,42 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, > > * in the PID check in lookup_pi_state. > > */ > > retry: > > + if (!argowner) { > > + if (oldowner != current) { > > + /* > > + * We raced against a concurrent self; things are > > + * already fixed up. Nothing to do. > > + */ > > + ret = 0; > > + goto out_unlock; > > + } > > + > > + if (__rt_mutex_futex_trylock(&pi_state->pi_mutex)) { > > + /* We got the lock after all, nothing to fix. */ > > + ret = 0; > > + goto out_unlock; > > + } > > + > > + /* > > + * Since we just failed the trylock; there must be an owner. > > + */ > > + newowner = rt_mutex_owner(&pi_state->pi_mutex); > > + BUG_ON(!newowner); > > + } else { > > + WARN_ON_ONCE(argowner != current); > > + if (oldowner == current) { > > + /* > > + * We raced against a concurrent self; things are > > + * already fixed up. Nothing to do. > > + */ > > + ret = 0; > > + goto out_unlock; > > + } > > + newowner = argowner; > > + } > > + > > + newtid = task_pid_vnr(newowner) | FUTEX_WAITERS; > > ... since it is always overwritten here. > > Is that intentional? No, I think you actually spotted a bug there. We now can't set OWNER_DIED anymore, which is bad. I think the below fixes things, but let me go trawl through the various futex test things, because I think I've seen a unit test for this _somewhere_. --- kernel/futex.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kernel/futex.c b/kernel/futex.c index 8c5424dd5924..7f719d110908 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -2311,9 +2311,6 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, raw_spin_lock_irq(&pi_state->pi_mutex.wait_lock); oldowner = pi_state->owner; - /* Owner died? */ - if (!pi_state->owner) - newtid |= FUTEX_OWNER_DIED; /* * We are here because either: @@ -2374,6 +2371,9 @@ static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, } newtid = task_pid_vnr(newowner) | FUTEX_WAITERS; + /* Owner died? */ + if (!pi_state->owner) + newtid |= FUTEX_OWNER_DIED; if (get_futex_value_locked(&uval, uaddr)) goto handle_fault;