linux-kernel - Re: [GIT PULL] aio: fix sleeping while TASK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 1 Feb 2015 15:09:37 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Benjamin LaHaise <bcrl@...ck.org>
Cc:	linux-aio@...ck.org, Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] aio: fix sleeping while TASK_INTERRUPTIBLE

On Sun, Feb 1, 2015 at 2:14 PM, Benjamin LaHaise <bcrl@...ck.org> wrote:
>
> It's ugly, but it actually is revealing a bug.

If so, that patch is *still* not the right thing to do. Leave the
warning. It's better than horrible crap code that doesn't warn, but
also doesn't make any *sense*.

Because code like this is just crap:

+       int running = current->state == TASK_RUNNING;

really. It's just crazy.

It makes no sense. It's all just avoiding the warning, it's not making
the code any better. In fact, it's actively making it worse, because
now the code is literally designed to give random different behavior
depending on timing.

For all I know, there might be multiple concurrent waiters etc that
got woken up on the same wait-queue, afaik, so the "am I runnable"
question is fundamentally nonsensical, since it isn't necessarily the
same as testing the state of the ring that we're waiting for anyway.
And if it *is* the same thing (because maybe the locking does end up
guaranteeing that "running" here ends up always matching some
particular state of the aio ring), then it should still be rewritten
in terms of that ring state, not some random "am I runnable" question.

So *clearly* the code has explicitly been written for the debug test,
not to make sense. Seriously. It's the only possible reason for why it
would test "current->state".

If the code relies on some 1:1 relationship with the wakeups
(*despite* having comments currently explicitly saying that it
doesn't), then use the new "wait_woken()" interface instead, which
actually does exactly that. That one doesn't test some random state,
it tests "was I explicitly woken". Then the whole "oh, but the mutex
changed the task state" doesn't even *matter*.

Which may in the end be the same thing in practice, but at least the
code makes *sense*. In ways that that "current->state == TASK_RUNNING"
test does not.

                         Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/