linux-kernel - Re: [PATCH 0/2] pipe: Fixes [ver #2]

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 19 Dec 2019 08:35:46 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     David Howells <dhowells@...hat.com>
Cc:     Josh Triplett <josh@...htriplett.org>,
        Konstantin Khlebnikov <khlebnikov@...dex-team.ru>,
        Akemi Yagi <toracat@...epo.org>, DJ Delorie <dj@...hat.com>,
        David Sterba <dsterba@...e.cz>,
        Eric Biggers <ebiggers@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...nel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH 0/2] pipe: Fixes [ver #2]

On Wed, Dec 18, 2019 at 11:56 PM David Howells <dhowells@...hat.com> wrote:
>
> I looked at splitting the waitqueue in to two, but it makes poll tricky.

No, it's actually trivial for poll.

The thing is, poll can happily just add two wait-queues to the
poll_table. In my conversion, I just did

-       poll_wait(filp, &pipe->wait, wait);
+       if (filp->f_mode & FMODE_READ)
+               poll_wait(filp, &pipe->rd_wait, wait);
+       if (filp->f_mode & FMODE_WRITE)
+               poll_wait(filp, &pipe->wr_wait, wait);

which changes the unconditional "add one" to two conditional adds.
They _could_ have been unconditional too, but why add unnecessary
wakeups? So it only really does it twice on named pipes (if opened for
reading and writing).

It's _unusual_ to add two wait-queues for a single poll, but it's not
wrong. The tty layer has always done it - exactly because it has
separate wait queues for reading and writing. Some other drivers do
too. Sometimes there's a separate wait queue for errors, sometimes
there are multiple wait-queues because there are events from the
"subsystem" and there are other events from the "device". I think
sound does the latter, for example.

And no, I don't particularly like the FMODE_READ/WRITE testing above -
it would be better to pass in the polling mask and ask "are we waiting
for polling for reading or writing?" instead of asking whether the
file descriptor was opened for read or write, but hey, it is what it
is.

Sadly, "poll()" doesn't really get passed the bitmask of what is being
waited on (it's there in the poll_tabvle "_key" field, but I don't
want to have the pipe code look into those kinds of details.

So the named pipe case could be improved, but it's not like anybody
really cares. Nobody uses named pipes any more (and few people ever
did). So I didn't worry about it.

            Linus