Re: [PATCH/RFC] Futex mmap_sem deadlock

[Olof Johansson]

On Tue, Feb 22, 2005 at 03:20:27PM -0800, Andrew Morton wrote:
> [EMAIL PROTECTED] (Olof Johansson) wrote:
> >
> > +           inc_preempt_count();
> > +           ret = get_user(curval, (int __user *)uaddr1);
> > +           dec_preempt_count();
> 
> That _should_ generate a might_sleep() warning, except it looks like we
> forgot to add a check to get_user().
> 
> It would be better to use __copy_from_user_inatomic() here, I think.

Thanks for catching that. New rev below.

-----

Some futex functions do get_user calls while holding mmap_sem for
reading. If get_user() faults, and another thread happens to be in mmap
(or somewhere else holding waiting on down_write for the same semaphore),
then do_page_fault will deadlock. Most architectures seem to be exposed
to this.

To avoid it, make sure the page is available. If not, release the
semaphore, fault it in and retry.

I also found another exposure by inspection, moving some of the code
around avoids the possible deadlock there.

Signed-off-by: Olof Johansson <[EMAIL PROTECTED]>


Index: linux-2.5/kernel/futex.c
===================================================================
--- linux-2.5.orig/kernel/futex.c       2005-02-21 16:09:38.000000000 -0600
+++ linux-2.5/kernel/futex.c    2005-02-22 17:20:22.000000000 -0600
@@ -329,6 +329,7 @@
        int ret, drop_count = 0;
        unsigned int nqueued;
 
+ retry:
        down_read(&current->mm->mmap_sem);
 
        ret = get_futex_key(uaddr1, &key1);
@@ -355,9 +356,19 @@
                   before *uaddr1.  */
                smp_mb();
 
-               if (get_user(curval, (int __user *)uaddr1) != 0) {
-                       ret = -EFAULT;
-                       goto out;
+               inc_preempt_count();
+               ret = __copy_from_user_inatomic(&curval, (int __user *)uaddr1, 
sizeof(int));
+               dec_preempt_count();
+
+               if (unlikely(ret)) {
+                       up_read(&current->mm->mmap_sem);
+                       /* Re-do the access outside the lock */
+                       ret = get_user(curval, (int __user *)uaddr1);
+
+                       if (!ret)
+                               goto retry;
+
+                       return ret;
                }
                if (curval != *valp) {
                        ret = -EAGAIN;
@@ -480,6 +491,7 @@
        int ret, curval;
        struct futex_q q;
 
+ retry:
        down_read(&current->mm->mmap_sem);
 
        ret = get_futex_key(uaddr, &q.key);
@@ -508,9 +520,21 @@
         * We hold the mmap semaphore, so the mapping cannot have changed
         * since we looked it up in get_futex_key.
         */
-       if (get_user(curval, (int __user *)uaddr) != 0) {
-               ret = -EFAULT;
-               goto out_unqueue;
+       inc_preempt_count();
+       ret = __copy_from_user_inatomic(&curval, (int __user *)uaddr, 
sizeof(int));
+       dec_preempt_count();
+       if (unlikely(ret)) {
+               up_read(&current->mm->mmap_sem);
+
+               if (!unqueue_me(&q)) /* There's a chance we got woken already */
+                       return 0;
+
+               /* Re-do the access outside the lock */
+               ret = get_user(curval, (int __user *)uaddr);
+
+               if (!ret)
+                       goto retry;
+               return ret;
        }
        if (curval != val) {
                ret = -EWOULDBLOCK;
Index: linux-2.5/mm/mempolicy.c
===================================================================
--- linux-2.5.orig/mm/mempolicy.c       2005-02-04 00:27:40.000000000 -0600
+++ linux-2.5/mm/mempolicy.c    2005-02-22 14:34:19.000000000 -0600
@@ -524,9 +524,13 @@
        } else
                pval = pol->policy;
 
-       err = -EFAULT;
+       if (vma) {
+               up_read(&current->mm->mmap_sem);
+               vma = NULL;
+       }
+
        if (policy && put_user(pval, policy))
-               goto out;
+               return -EFAULT;
 
        err = 0;
        if (nmask) {
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/