Re: [PATCH/RFC] Futex mmap_sem deadlock

[Jamie Lokier]

Linus Torvalds wrote:
> >     queue_me(...) etc.
> >     current->flags |= PF_MMAP_SEM;             <- new
> >     ret = get_user(...);
> >     current->flags &= PF_MMAP_SEM;             <- new
> >     /* the rest */
> 
> That is uglee. 
> 
> We really have this already, and it's called "current->preempt". It 
> handles any lock at all, and doesn't add yet another special case to all 
> the architectures.

Ooh, I didn't know current->preempt did that (been away).

>       repeat:
>               down_read(&current->mm->mmap_sem);
>               get_futex_key(...) etc.
>               queue_me(...) etc.
>               inc_preempt_count();
>               ret = get_user(...);
>               dec_preempt_count();
>               if (unlikely(ret)) {
>                       up_read(&current->mm->mmap_sem);
>                       /* Re-do the access outside the lock */
>                       ret = get_user(...);
>                       if (!ret)
>                               goto repeat;
>                       return ret;
>               }

That would work.  I like it. :)

Page faults will enter the fault handler twice (i.e. slower), but
that's not really a disadvantage, because a program always references
the memory just before calling futex_wait anyway.  A fault is rare.

There is one small but important error: the "return ret" mustn't just
return.  It must call unqueue_me(&q) just like the code at out_unqueue,
_including_ the conditional "ret = 0", but _excluding_ the up_read().

Alternatively, since it's a rare case, just shuffle the loop around:

                down_read(&current->mm->mmap_sem);
        repeat:
                get_futex_key(...) etc.
                queue_me(...) etc.
                inc_preempt_count();
                ret = get_user(...);
                dec_preempt_count();
                if (unlikely(ret)) {
                        up_read(&current->mm->mmap_sem);
                        /* Re-do the access outside the lock */
                        ret = get_user(...);
                        down_read(&current->mm->mmap_sem);
                        if (!ret)
                                goto repeat;
                        goto out_unqueue;
                }

-- Jamie
-
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/