linux-kernel - Re: Splicing to/from a tty

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wiDX9K0aj6mQsQM54yyJTg2fkzME82aSswB2woFGmW81Q@mail.gmail.com>
Date:   Tue, 19 Jan 2021 12:24:22 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Christoph Hellwig <hch@....de>, Oliver Giles <ohw.giles@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Jiri Slaby <jirislaby@...nel.org>
Subject: Re: Splicing to/from a tty

On Tue, Jan 19, 2021 at 3:54 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> This looks sane, but I'm still missing what the goal of this is here.
> It's nice from a "don't make the ldisc do the userspace copy", point of
> view, but what is the next step in order to tie that into splice?

Ok, so here's a series of four patches that make ttys possible sources
and destinations for splice() again.

Well, the first patch is just the pipe one for sendfile() - and it's
the hacky one-liner, not the proper one that Al will hopefully add.

NOTE! I've signed off on these, because I think they are fine for
testing - but they are really meant for testing ONLY.

I'm running a kernel with these in place, so they kind of work. And
yes, I verified that sendfile() now works with a pipe or tty target. I
didn't actually check splicing _from_ a tty, nor did I check that
readv/writev now works properly, but it all LoosGood(tm) to me.

HOWEVER.

The reason these are for testing only is that

 (a) my tests are pretty limited, and I'd like the actual people who
reported this to really test them out

 (b) the new read iterator model is going to be quite horribly slow
for big pty transfers because the n_tty ldisc isn't doing the cookie
batching

 (c) I really really want Al to take a look at that iov_iter_revert()
thing in do_tty_write() (in "[PATCH 2/4] tty: implement write_iter")

Note that I'm more than happy to do (b) as a patch on top of this, but
I'd like (a) and (c) to be clarified before I do that.

> I ask as I also have reports that sysfs binary files are now failing for
> this same reason, so I need to make the same change for them and it's
> not excatly obvious what to do:

Ok, so that would require those kernfs_fop_{read,write}() functions to
also be converted to read_iter/write_iter.

That doesn't look horrendous: it's not all that dissimilar from the
two patches to do that for tty's ("tty: implement {read,write}_iter").
The seq_file part already has a iter version for reading, and the main
change to kernfs_file_direct_read() and kernfs_fop_write() is to do
that

 (a) change the arguments from file/buf/count/ppos to kiocb/iov_iter

 (b) change the copy_to/from_user() calls to copy_to/from_iter()

Note that (b) involves changing the semantics of the return value:
"copy_to/from_user()" returns the number of bytes that were *NOT*
copied, while "copy_to/from_iter()" returns the number of bytes that
WERE copied.

So the error case check does from

        if (copy_to/from_user()) **ERROR**

to

        if (copy_to/from_iter(n) != n) **ERROR**

but that is fairly straightforward.

The two "tty: implement write/read_iter" patches (patch 2 and 4) can
be used as examples. That said, I want to again stress that they
haven't seen all that much testing, and I do want Al to spray his holy
penguin pee on that iov_iter_revert() thing in patch 2.

I'm honestly not that motivated on those sysfs files: the tty layer
was an interesting test-case that I wanted to look at just because the
conversion to kernel pointers was nontrivial for the read side.

But that sysfs binary file case really isn't interesting, and just
more of a "Christoph broke it, I think he should just fix it".
Christoph?

Anyway, anybody willing to test these tty/pipe patches on the loads
that failed? Oliver?

               Linus

View attachment "0001-pipe-allow-sendfile-destination-with-splice_write.patch" of type "text/x-patch" (1311 bytes)

View attachment "0002-tty-implement-write_iter.patch" of type "text/x-patch" (5754 bytes)

View attachment "0003-tty-convert-tty_ldisc_ops-read-function-to-take-a-ke.patch" of type "text/x-patch" (21393 bytes)

View attachment "0004-tty-implement-read_iter.patch" of type "text/x-patch" (5002 bytes)