From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751927AbeAVJnj (ORCPT ); Mon, 22 Jan 2018 04:43:39 -0500 Received: from mail-qt0-f196.google.com ([209.85.216.196]:35147 "EHLO mail-qt0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751794AbeAVJnh (ORCPT ); Mon, 22 Jan 2018 04:43:37 -0500 X-Google-Smtp-Source: AH8x2251P+5lBR34QpsVKLg452n2w38b4cryqRn4xeS8TyQgNf4O1uK9o9rDLx28l5pu1uMtQpZr/ioYPaPQjgFN5Yw= MIME-Version: 1.0 In-Reply-To: <20180117152416.eazu647rhjvy4rw6@gmail.com> References: <20180117152416.eazu647rhjvy4rw6@gmail.com> From: Geert Uytterhoeven Date: Mon, 22 Jan 2018 10:43:36 +0100 X-Google-Sender-Auth: k6bGEtyXa-gRdrxWxnxHixzarnM Message-ID: Subject: Re: [GIT PULL] locking fixes To: Ingo Molnar , Peter Zijlstra Cc: Linus Torvalds , Linux Kernel Mailing List , Thomas Gleixner , "Paul E. McKenney" , Andrew Morton Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id w0M9hhWn017999 Hi Ingo, Peter, On Wed, Jan 17, 2018 at 4:24 PM, Ingo Molnar wrote: > Please pull the latest locking-urgent-for-linus git tree from: > > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-urgent-for-linus > > # HEAD: fbe0e839d1e22d88810f3ee3e2f1479be4c0aa4a futex: Prevent overflow by strengthen input validation > > Two futex fixes: a input parameters robustness fix, and futex race fixes. > Peter Zijlstra (1): > futex: Avoid violating the 10th rule of futex > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -2294,21 +2297,17 @@ static void unqueue_me_pi(struct futex_q *q) > spin_unlock(q->lock_ptr); > } > > -/* > - * Fixup the pi_state owner with the new owner. > - * > - * Must be called with hash bucket lock held and mm->sem held for non > - * private futexes. > - */ > static int fixup_pi_state_owner(u32 __user *uaddr, struct futex_q *q, > - struct task_struct *newowner) > + struct task_struct *argowner) > { > - u32 newtid = task_pid_vnr(newowner) | FUTEX_WAITERS; > struct futex_pi_state *pi_state = q->pi_state; > u32 uval, uninitialized_var(curval), newval; > - struct task_struct *oldowner; > + 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 either because we stole the rtmutex from the > - * previous highest priority waiter or we are the highest priority > - * waiter but have failed to get the rtmutex the first time. > + * 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). > * > - * We have to replace the newowner TID in the user space variable. > + * 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? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds