Re: [GIT PULL] aio: fix sleeping while TASK_INTERRUPTIBLE

[Benjamin LaHaise <bcrl@kvack.org>]

On Sun, Feb 01, 2015 at 03:33:25PM -0800, Linus Torvalds wrote:
> On Sun, Feb 1, 2015 at 2:14 PM, Benjamin LaHaise <bcrl@kvack.org> wrote:
> >
> > It's ugly, but it actually is revealing a bug.  Spurious wake ups caused
> > by the task already being added to ctx->wait when calling into mutex_lock()
> > could inadvertently cause things to go wrong.  I can envision there being
> > code invoked that possibly expects a 1-1 relationship between sleeps and
> > wake ups, which being on the additional wait queue might break.
> 
> So I'm looking at it, and I don't see it.
> 
> One side uses wait_event_interruptible_hrtimeout(), which waits for
> the return value (or the timeout), and it doesn't matter how many
> times it gets woken up, regardless of what it's waiting for. If it
> gets extra wakeups, it will just go through the loop again.
> 
> The other side is just a plain aio_read_events() ->
> aio_read_events_ring(), and that one just reads as many events as it
> can, unless some error happens.
> 
> In other words, it really looks like the warning is spurious, and the
> comments about how the semaphore could cause it to loop around but it
> all works look entirely correct.
> 
> So no, I don't see it revealing a bug at all. All I see is a spurious warning.
> 
> What's the bug you think could happen?

The bug would be in code that gets run via mutex_lock(), kmap(), or (more 
likely) in the random mm or filesystem code that could be invoked via 
copy_to_user().

If someone has code that works like the following inside of, say, a filesystem 
that's doing i/o somewhere:

static void foo_done(struct foo *foo)
{
	/* stuff err somewhere */
	wake_up(foo->task);
}

void some_random_code_in_some_fs()
{
	struct foo *foo;

	/* setup the foo */
	foo->task = current;
	set_current_state(TASK_UNINTERRUPTIBLE);
	/* send the foo on to do some other work */
	schedule();
	/* foo_done should have been called at this point. */
}

When the task in question can receive spurious wake ups, there is the 
possibility that schedule() ends up returning without foo_done() having 
been called, which is not the case normally (that is, there should be a 
one to one relationship between the wake_up and schedule returning in 
this case).  While I don't immediately see any code that relies on this, 
I'm not convinced that every possible code path that can be invoked 
(especially via copy_to_user()) does not rely on these semantics.  Maybe 
I'm just being paranoid, but this is one of the concerns I raised when 
this issue came forth.  Nobody has addressed it yet, though.

		-ben

>                          Linus

-- 
"Thought is the essence of where you are now."