Re: [PATCH/RFC] Futex mmap_sem deadlock

[Linus Torvalds]

On Tue, 22 Feb 2005, Jamie Lokier wrote:
> 
> A much simpler solution (and sorry for not offering it earlier,
> because Andrew Morton pointed out this bug long ago, but I was busy), is:
> 
> In futex.c:
> 
>       down_read(&current->mm->mmap_sem);
>       get_futex_key(...) etc.
>       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.

Just do

        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;
                }
                ...

and you should be ok.

No new special cases, no new abstractions. At most, we should probably 
create a "get_user_inatomic()", to 

 - make it damn obvious what we're doing, and match the explicit
   "inatomic" in the other place where we depend on this (fs/filemap.c)

 - allow the regular "get_user()" to continue to do the normal
   "might_sleep()" checks.

That's assuming we can't just make rwsem's nest nicely.

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