Commit e579dcbf authored by john stultz's avatar john stultz Committed by Greg Kroah-Hartman

[PATCH] futex_handle_fault always fails

We found this issue last week w/ the -RT kernel, but it seems the same
issue is in mainline as well.

Basically it is possible for futex_unlock_pi to return without actually
freeing the lock.  This is due to buggy logic in the use of
futex_handle_fault() and its attempt argument in a failure case.

Looking at futex.c the logic is as follows:

1) In futex_unlock_pi() we start w/ ret=0 and we go down to the first
   futex_atomic_cmpxchg_inatomic(), where we find uval==-EFAULT.  We then
   jump to the pi_faulted label.

2) From pi_faulted: We increment attempt, unlock the sem and hit the
   retry label.

3) From the retry label, with ret still zero, we again hit EFAULT on the
   first futex_atomic_cmpxchg_inatomic(), and again goto the pi_faulted
   label.

4) Again from pi_faulted: we increment attempt and enter the
   conditional, where we call futex_handle_fault.

5) futex_handle_fault fails, and we goto the out_unlock_release_sem
   label.

6) From out_unlock_release_sem we return, and since ret is still zero,
   we return without error, while never actually unlocking the lock.

Issue #1: at the first futex_atomic_cmpxchg_inatomic() we should probably
be setting ret=-EFAULT before jumping to pi_faulted: However in our case
this doesn't really affect anything, as the glibc we're using ignores the
error value from futex_unlock_pi().

Issue #2: Look at futex_handle_fault(), its first conditional will return
-EFAULT if attempt is >= 2.  However, from the "if(attempt++)
futex_handle_fault(attempt)" logic above, we'll *never* call
futex_handle_fault when attempt is less then two.  So we never get a chance
to even try to fault the page in.

The following patch addresses these two issues by 1) Always setting ret to
-EFAULT if futex_handle_fault fails, and 2) Removing the = in
futex_handle_fault's (attempt >= 2) check.

I'm really not sure this is the right fix, but wanted to bring it up so
folks knew the issue is alive and well in the current -git tree.  From
looking at the git logs the logic was first introduced (then later copied
to other places) in the following commit almost a year ago:

http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=4732efbeb997189d9f9b04708dc26bf8613ed721;hp=5b039e681b8c5f30aac9cc04385cc94be45d0823

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Ingo Molnar <mingo@elte.hu>
Acked-by: default avatarThomas Gleixner <tglx@linutronix.de>
Signed-off-by: default avatarAndrew Morton <akpm@osdl.org>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@suse.de>
parent 6997a6fa
...@@ -297,7 +297,7 @@ static int futex_handle_fault(unsigned long address, int attempt) ...@@ -297,7 +297,7 @@ static int futex_handle_fault(unsigned long address, int attempt)
struct vm_area_struct * vma; struct vm_area_struct * vma;
struct mm_struct *mm = current->mm; struct mm_struct *mm = current->mm;
if (attempt >= 2 || !(vma = find_vma(mm, address)) || if (attempt > 2 || !(vma = find_vma(mm, address)) ||
vma->vm_start > address || !(vma->vm_flags & VM_WRITE)) vma->vm_start > address || !(vma->vm_flags & VM_WRITE))
return -EFAULT; return -EFAULT;
...@@ -747,8 +747,10 @@ futex_wake_op(u32 __user *uaddr1, u32 __user *uaddr2, ...@@ -747,8 +747,10 @@ futex_wake_op(u32 __user *uaddr1, u32 __user *uaddr2,
*/ */
if (attempt++) { if (attempt++) {
if (futex_handle_fault((unsigned long)uaddr2, if (futex_handle_fault((unsigned long)uaddr2,
attempt)) attempt)) {
ret = -EFAULT;
goto out; goto out;
}
goto retry; goto retry;
} }
...@@ -1322,9 +1324,10 @@ static int do_futex_lock_pi(u32 __user *uaddr, int detect, int trylock, ...@@ -1322,9 +1324,10 @@ static int do_futex_lock_pi(u32 __user *uaddr, int detect, int trylock,
* still holding the mmap_sem. * still holding the mmap_sem.
*/ */
if (attempt++) { if (attempt++) {
if (futex_handle_fault((unsigned long)uaddr, attempt)) if (futex_handle_fault((unsigned long)uaddr, attempt)) {
ret = -EFAULT;
goto out_unlock_release_sem; goto out_unlock_release_sem;
}
goto retry_locked; goto retry_locked;
} }
...@@ -1506,9 +1509,10 @@ static int futex_unlock_pi(u32 __user *uaddr) ...@@ -1506,9 +1509,10 @@ static int futex_unlock_pi(u32 __user *uaddr)
* still holding the mmap_sem. * still holding the mmap_sem.
*/ */
if (attempt++) { if (attempt++) {
if (futex_handle_fault((unsigned long)uaddr, attempt)) if (futex_handle_fault((unsigned long)uaddr, attempt)) {
ret = -EFAULT;
goto out_unlock; goto out_unlock;
}
goto retry_locked; goto retry_locked;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment