Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754891Ab0F3Jzc (ORCPT ); Wed, 30 Jun 2010 05:55:32 -0400 Received: from cantor.suse.de ([195.135.220.2]:54617 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752948Ab0F3Jz3 (ORCPT ); Wed, 30 Jun 2010 05:55:29 -0400 Date: Wed, 30 Jun 2010 11:55:25 +0200 From: Michal Hocko To: Linus Torvalds Cc: Ingo Molnar , Darren Hart , Thomas Gleixner , LKML , Nick Piggin , Alexey Kuznetsov , Peter Zijlstra Subject: [PATCH] futex: futex_find_get_task remove credentails check Message-ID: <20100630095525.GA5840@tiehlicka.suse.cz> References: <4C24ED34.9040808@us.ibm.com> <4C253D32.6040304@us.ibm.com> <20100628144246.GA14201@tiehlicka.suse.cz> <20100628153214.GA24127@tiehlicka.suse.cz> <20100628155845.GC24127@tiehlicka.suse.cz> <20100628163952.GD24127@tiehlicka.suse.cz> <1277743748.3561.139.camel@laptop> <20100629084230.GE6215@tiehlicka.suse.cz> <20100630070115.GB3890@tiehlicka.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100630070115.GB3890@tiehlicka.suse.cz> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3823 Lines: 109 On Wed 30-06-10 09:01:15, Michal Hocko wrote: > On Tue 29-06-10 09:41:02, Linus Torvalds wrote: > > On Tue, Jun 29, 2010 at 1:42 AM, Michal Hocko wrote: > > > > > > futex_find_get_task is currently used (through lookup_pi_state) from two > > > contexts, futex_requeue and futex_lock_pi_atomic. While credentials check > > > makes sense in the first code path, the second one is more problematic > > > because this check requires that the PI lock holder (pid parameter) has > > > the same uid and euid as the process's euid which is trying to lock the > > > same futex (current). > > > > So exactly why does it make sense to check the credentials in the > > first code path then? > > I though that requeue needs this for security reasons (don't let requeue > process for other user), but when I thought about that again you are > right and the only what matters should be accessibility of the shared > memory. And here is the patch which does the thing. -- >From 082c5ad2c482a8e78b61b17e213e750b006176aa Mon Sep 17 00:00:00 2001 From: Michal Hocko Date: Wed, 30 Jun 2010 09:51:19 +0200 Subject: [PATCH] futex: futex_find_get_task remove credentails check futex_find_get_task is currently used (through lookup_pi_state) from two contexts, futex_requeue and futex_lock_pi_atomic. None of the paths looks it needs the credentials check, though. Different (e)uids shouldn't matter at all because the only thing that is important for shared futex is the accessibility of the shared memory. The credentail check results in glibc assert failure or process hang (if glibc is compiled without assert support) for shared robust pthread mutex with priority inheritance if a process tries to lock already held lock owned by a process with a different euid: pthread_mutex_lock.c:312: __pthread_mutex_lock_full: Assertion `(-(e)) != 3 || !robust' failed. The problem is that futex_lock_pi_atomic which is called when we try to lock already held lock checks the current holder (tid is stored in the futex value) to get the PI state. It uses lookup_pi_state which in turn gets task struct from futex_find_get_task. ESRCH is returned either when the task is not found or if credentials check fails. futex_lock_pi_atomic simply returns if it gets ESRCH. glibc code, however, doesn't expect that robust lock returns with ESRCH because it should get either success or owner died. Signed-off-by: Michal Hocko --- kernel/futex.c | 17 ++++------------- 1 files changed, 4 insertions(+), 13 deletions(-) diff --git a/kernel/futex.c b/kernel/futex.c index e7a35f1..6a3a5fa 100644 --- a/kernel/futex.c +++ b/kernel/futex.c @@ -429,20 +429,11 @@ static void free_pi_state(struct futex_pi_state *pi_state) static struct task_struct * futex_find_get_task(pid_t pid) { struct task_struct *p; - const struct cred *cred = current_cred(), *pcred; rcu_read_lock(); p = find_task_by_vpid(pid); - if (!p) { - p = ERR_PTR(-ESRCH); - } else { - pcred = __task_cred(p); - if (cred->euid != pcred->euid && - cred->euid != pcred->uid) - p = ERR_PTR(-ESRCH); - else - get_task_struct(p); - } + if (p) + get_task_struct(p); rcu_read_unlock(); @@ -564,8 +555,8 @@ lookup_pi_state(u32 uval, struct futex_hash_bucket *hb, if (!pid) return -ESRCH; p = futex_find_get_task(pid); - if (IS_ERR(p)) - return PTR_ERR(p); + if (!p) + return -ESRCH; /* * We need to look at the task state flags to figure out, -- 1.7.1 -- Michal Hocko L3 team SUSE LINUX s.r.o. Lihovarska 1060/12 190 00 Praha 9 Czech Republic -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/