mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] futex: futex_find_get_task remove credentails check
@ 2010-07-08 12:51 Michal Hocko
  2010-07-08 13:22 ` Ingo Molnar
  2010-07-28 23:57 ` [stable] " Greg KH
  0 siblings, 2 replies; 11+ messages in thread
From: Michal Hocko @ 2010-07-08 12:51 UTC (permalink / raw)
  To: stable; +Cc: Thomas Gleixner, Ingo Molnar, Darren Hart, LKML

Hi stable team,
could you consider including the following patch (Linus tree commit:
7a0ea09ad5352efce8fe79ed853150449903b9f5).

The original discussion which led to this commit can be found at
http://lkml.org/lkml/2010/6/23/52.

In short:
The original pi locking implementation (since it got to the kernel)
contains a credential check (in futex_find_get_task) if we want to
create a PI state for already held lock. This test fails if the lock
owner has a different (e)uid than the process for which we want to
create the state.
The lock operation then fails with ESRCH which is the error code 
which is returned if the process holding a lock doesn't exist. 
The userspace (glibc) doesn't expect this behavior for shared robust PI
futexes and fails with an assert or hang the task in the end-less loop.

The test case is attached in the referenced thread.

The credential test, which is removed by this patch, doesn't look
correct and it limits the functionality without any good reason. There
are no security consequences as well because the only thing that should
matter in the shared futex-es is accessibility to the shared memory.

The patch applies as is on top of Vanilla 2.6.32, but let me know if you
want to base it on top of the any of the stable trees.


---
>From 7a0ea09ad5352efce8fe79ed853150449903b9f5 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
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 <mhocko@suse.cz>
Acked-by: Darren Hart <dvhltc@us.ibm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Nick Piggin <npiggin@suse.de>
Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 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

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: futex: race in lock and unlock&exit for robust futex with PI?
@ 2010-06-25 17:53 Darren Hart
  2010-06-25 23:35 ` Darren Hart
  0 siblings, 1 reply; 11+ messages in thread
From: Darren Hart @ 2010-06-25 17:53 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Thomas Gleixner, Peter Zijlstra, LKML, Nick Piggin,
	Alexey Kuznetsov, Linus Torvalds

On 06/25/2010 01:27 AM, Michal Hocko wrote:
> On Thu 24-06-10 19:42:50, Darren Hart wrote:
>> On 06/23/2010 02:13 AM, Michal Hocko wrote:
>>> Hi,
>
> Hi,
>
>>
>> Hi Michal,
>>
>> Thanks for reporting the issue and providing a testcase.
>>
>>>
>>> attached you can find a simple test case which fails quite easily on the
>>> following glibc assert:
>>> "SharedMutexTest: pthread_mutex_lock.c:289: __pthread_mutex_lock:
>>>    Assertion `(-(e)) != 3 || !robust' failed." "
>>
>> I've run runSimple.sh in a tight loop for a couple hours (about 2k
>> iterations so far) and haven't seen anything other than "Here we go"
>> printed to the console.
>
> Maybe a higher load on CPUs would help (busy loop on other CPUs).

Must have been a build issue. I can reproduce _something_ now. Within 10 
iterations of runSimple.sh the test hangs. ps shows all the simple 
processes sitting in pause.

(gdb) bt
#0  0x0000003c0060e030 in __pause_nocancel () from /lib64/libpthread.so.0
#1  0x0000003c006085fc in __pthread_mutex_lock_full ()
    from /lib64/libpthread.so.0
#2  0x0000000000400cd6 in main (argc=1, argv=0x7fffc016e508) at simple.c:101

There is only one call to pause* in pthread_mutex_lock.c: (line ~316):

	/* ESRCH can happen only for non-robust PI mutexes where
	   the owner of the lock died.  */
	assert (INTERNAL_SYSCALL_ERRNO (e, __err) != ESRCH || !robust);

	/* Delay the thread indefinitely.  */
	while (1)
	  pause_not_cancel ();

Right now I'm thinking that NDEBUG is set in my build for whatever 
reason, but I think I'm seeing the same issue you are. I'll review the 
futex code and prepare a trace patch and see if I can reproduce with that.

Note: confirmed, the glibc rpm has -DNDEBUG=1

--
Darren

>
>>
>> I had to add -D_GNU_SOURCE to get it to build on my system (RHEL5.2
>> + 2.6.34). Perhaps this is just a difference in the toolchain.
>
> I assume that you got PTHREAD_PRIO_INHERIT undeclared error, don't you?
> I have hacked around that by #define __USE_UNIX98 which worked on Debian
> and OpenSuse. But you are right _GNU_SOURCE is definitely better
> solution.
>
>>
>>> AFAIU, this assertion says that futex syscall cannot fail with ESRCH
>>> for robust futex because it should either succeed or fail with
>>> EOWNERDEAD.
>>
>> I'll have to think on that and review the libc source. We do need to
>> confirm that the assert is even doing the right thing.
>
> Sure. I have looked through the glibc lock implementation and it makes
> quite a good sense to me. A robust lock should never return with ESRCH.
>
>>
>>>
>>> We have seen this problem on SLES11 and SLES11SP1 but I was able to
>>> reproduce it with the 2.6.34 kernel as well.
>>
>> What kind of system are you seeing this on? I've been running on a
>> 4-way x86_64 blade.
>
> * Debian (squeeze/sid) with
> - Intel(R) Core(TM)2 CPU T5600 @ 1.83GHz
> - kernel: vanilla 2.6.34
> - glibc: 2.11.1-3
> - i386
>
> * OpenSuse 11.2 with
> - Intel(R) Core(TM)2 Duo CPU E4500 @ 2.20GHz
> - kernel: distribution 2.6.31.12-0.2-desktop
> - glibc: 2.10.1-10.5.1
> - i386
>
> * SLES11SP1
> - Dual-Core AMD Opteron(tm) Processor 1218
> - kernel: distribution 2.6.32.12-0.3-default
> - glibc: 2.11.1-0.17.4
> - x86_64
>
> Each box shows a different number of asserts during 10 iterations.
>
>>
>>> The test case is quite easy.
>>>
>>> Executed with a parameter it creates a test file and initializes shared,
>>> robust pthread mutex (optionaly compile time configured with priority
>>> inheritance) backed by the mmapped test file. Without a parameter it
>>> mmaps the file and just locks, unlocks mutex and checks for EOWNERDEAD
>>> (this should never happen during the test as the process never dies with
>>> the lock held) in the loop.
>>
>> Have you found the PI parameter to be required for reproducing the
>> error? From the comments below I'm assuming so... just want to be
>> sure.
>
> Yes. If you comment out USE_PI variable in the script the problem is not
> shown at all.
>
>>
>>>
>>> If I run this application for multiple users in parallel I can see the
>>> above assertion. However, if priority inheritance is turned off then
>>> there is no problem. I am not able to reproduce also if the test case is
>>> run under a single user.
>>>
>>> I am using the attached runSimple.sh script to run the test case like
>>> this:
>>>
>>> rm test.file simple
>>> for i in `seq 10`
>>> do
>>> 	sh runSimple.sh
>>> done
>>>
>>> To disable IP just comment out USE_PI variable in the script.
>>> You need to change USER1 and USER2 variables to match you system. You
>>> will need to run the script as root if you do not set any special
>>> setting to run su on behalf of those users.
>>>
>>> I have tried to look at futex_{un}lock_pi but it is really hard to
>>> understand.
>>
>> *grin* tell me about it...
>>
>> See Documentation/pi-futex.txt if you haven't already.
>
> Will do.
>
>>
>>> I assume that lookup_pi_state is the one which sets ESRCH
>>> after it is not able to find the pid of the current owner.
>>>
>>> This would suggest that we are racing with the unlock of the current
>>> lock holder but I don't see how is this possible as both lock and unlock
>>> paths hold fshared lock for all operations over the lock value. I have
>>> noticed that the lock path drops fshared if the current holder is dying
>>> but then it retries the whole process again.
>>>
>>> Any advice would be highly appreciated.
>>
>> If I can reproduce this I should be able to get some trace points in
>> there to get a better idea of the execution path leading up to the
>> problem.
>
> Please make sure that you run the test case with two different users. I
> couldn't reproduce the issue with a single user.
>
> If you have some ideas about patches which I could try then just pass it
> to me.
>
>>
>> This would be a great time to have those futex fault injection patches...
>>
>>
>> --
>> Darren Hart
>> IBM Linux Technology Center
>> Real-Time Linux Team
>
> Thanks for looking into it.


-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-07-29  0:16 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-07-08 12:51 [PATCH] futex: futex_find_get_task remove credentails check Michal Hocko
2010-07-08 13:22 ` Ingo Molnar
2010-07-12 10:20   ` Thomas Gleixner
2010-07-28 23:57 ` [stable] " Greg KH
  -- strict thread matches above, loose matches on Subject: below --
2010-06-25 17:53 futex: race in lock and unlock&exit for robust futex with PI? Darren Hart
2010-06-25 23:35 ` Darren Hart
2010-06-28 14:42   ` Michal Hocko
2010-06-28 15:32     ` Michal Hocko
2010-06-28 15:58       ` Michal Hocko
2010-06-28 16:39         ` Michal Hocko
2010-06-28 16:49           ` Peter Zijlstra
2010-06-29  8:42             ` [PATCH] futex: futex_find_get_task make credentials check conditional Michal Hocko
2010-06-29 16:41               ` Linus Torvalds
2010-06-30  7:01                 ` Michal Hocko
2010-06-30  9:55                   ` [PATCH] futex: futex_find_get_task remove credentails check Michal Hocko
2010-06-30 16:43                     ` Darren Hart
2010-07-08  9:28                       ` Michal Hocko
2010-07-08  9:32                         ` Ingo Molnar
2010-07-08  9:39                           ` Michal Hocko
2010-07-08  9:43                             ` Peter Zijlstra
2010-07-08  9:50                               ` Michal Hocko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome