Re: [PATCH/RFC] Futex mmap_sem deadlock

[Jamie Lokier <jamie@shareable.org>]

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