User: Password:
|
|
Subscribe / Log in / New account

Re: [PATCH/RFC] Futex mmap_sem deadlock

From:  Linus Torvalds <torvalds-AT-osdl.org>
To:  Jamie Lokier <jamie-AT-shareable.org>
Subject:  Re: [PATCH/RFC] Futex mmap_sem deadlock
Date:  Tue, 22 Feb 2005 13:30:27 -0800 (PST)
Cc:  Andrew Morton <akpm-AT-osdl.org>, Olof Johansson <olof-AT-austin.ibm.com>, linux-kernel-AT-vger.kernel.org, rusty-AT-rustcorp.com.au
Archive-link:  Article, Thread



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


(Log in to post comments)


Copyright © 2005, Eklektix, Inc.
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds