diff mbox series

[v2,08/11] tty: splice_read: disable

Message ID 4dec932dcd027aa5836d70a6d6bedd55914c84c2.1703126594.git.nabijaczleweli@nabijaczleweli.xyz
State New
Headers
Series Avoid unprivileged splice(file->)/(->socket) pipe exclusion |

Commit Message

Ahelenia Ziemiańska Dec. 21, 2023, 3:09 a.m. UTC 
  We request non-blocking I/O in the generic copy_splice_read, but
"the tty layer doesn't actually honor the IOCB_NOWAIT flag for
various historical reasons.". This means that a tty->pipe splice
will happily sleep with the pipe locked forever, and any process
trying to take it (due to an open/read/write/&c.) will enter
uninterruptible sleep.

This also masks inconsistent wake-ups (usually every second line)
when splicing from ttys in icanon mode.

Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wimmqG_wvSRtMiKPeGGDL816n65u=Mq2+H3-=uM2U6FmA@mail.gmail.com/
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 drivers/tty/tty_io.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Greg KH Dec. 21, 2023, 8:10 a.m. UTC | #1 
On Thu, Dec 21, 2023 at 04:09:10AM +0100, Ahelenia Ziemiańska wrote:
> We request non-blocking I/O in the generic copy_splice_read, but
> "the tty layer doesn't actually honor the IOCB_NOWAIT flag for
> various historical reasons.". This means that a tty->pipe splice
> will happily sleep with the pipe locked forever, and any process
> trying to take it (due to an open/read/write/&c.) will enter
> uninterruptible sleep.
> 
> This also masks inconsistent wake-ups (usually every second line)
> when splicing from ttys in icanon mode.
> 
> Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wimmqG_wvSRtMiKPeGGDL816n65u=Mq2+H3-=uM2U6FmA@mail.gmail.com/
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> ---

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Jiri Slaby Jan. 3, 2024, 11:36 a.m. UTC | #2 
On 21. 12. 23, 4:09, Ahelenia Ziemiańska wrote:
> We request non-blocking I/O in the generic copy_splice_read, but
> "the tty layer doesn't actually honor the IOCB_NOWAIT flag for
> various historical reasons.". This means that a tty->pipe splice
> will happily sleep with the pipe locked forever, and any process
> trying to take it (due to an open/read/write/&c.) will enter
> uninterruptible sleep.
> 
> This also masks inconsistent wake-ups (usually every second line)
> when splicing from ttys in icanon mode.
> 
> Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wimmqG_wvSRtMiKPeGGDL816n65u=Mq2+H3-=uM2U6FmA@mail.gmail.com/
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> ---
>   drivers/tty/tty_io.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 06414e43e0b5..50c2957a9c7f 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -465,7 +465,6 @@ static const struct file_operations tty_fops = {
>   	.llseek		= no_llseek,
>   	.read_iter	= tty_read,
>   	.write_iter	= tty_write,
> -	.splice_read	= copy_splice_read,
>   	.splice_write	= iter_file_splice_write,

This and the other patch effectively reverts dd78b0c483e33 and 
9bb48c82aced0. I.e. it breaks "things". Especially:

commit 9bb48c82aced07698a2d08ee0f1475a6c4f6b266
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue Jan 19 11:41:16 2021 -0800

     tty: implement write_iter

     This makes the tty layer use the .write_iter() function instead of the
     traditional .write() functionality.

     That allows writev(), but more importantly also makes it possible to
     enable .splice_write() for ttys, reinstating the "splice to tty"
     functionality that was lost in commit 36e2c7421f02 ("fs: don't allow
     splice read/write without explicit ops").

     Fixes: 36e2c7421f02 ("fs: don't allow splice read/write without 
explicit ops")


What are those "things" doing that "splice to tty", I don't recall and 
the commit message above ^^^ does not spell that out. Linus?

thanks,
Linus Torvalds Jan. 3, 2024, 7:14 p.m. UTC | #3 
On Wed, 3 Jan 2024 at 03:36, Jiri Slaby <jirislaby@kernel.org> wrote:
>
> What are those "things" doing that "splice to tty", I don't recall and
> the commit message above ^^^ does not spell that out. Linus?

It's some annoying SSL VPN thing that splices to pppd:

   https://lore.kernel.org/all/C8KER7U60WXE.25UFD8RE6QZQK@oguc/

and I'd be happy to try to limit splice to tty's to maybe just the one
case that pppd uses.

So I don't think we should remove splice_write for tty's entirely, but
maybe we can limit it to only the case that the VPN thing used.

I never saw the original issue personally, and I think only Oliver
reported it, so cc'ing Oliver.

Maybe that VPN thing already has the pty in non-blocking mode, for
example, and we could make the tty splicing fail for any blocking op?

                Linus
Oliver Giles Jan. 3, 2024, 9:34 p.m. UTC | #4 
On Wed, Jan 3 2024 at 11:14:59 -08:00:00, Linus Torvalds 
<torvalds@linux-foundation.org> wrote:
> 
> It's some annoying SSL VPN thing that splices to pppd:
> 
>    https://lore.kernel.org/all/C8KER7U60WXE.25UFD8RE6QZQK@oguc/

I'm happy to report that that particular SSL VPN tool is no longer 
around.
And it had anyway grown a fall-back-to-read/write in case splice() 
fails.
So at least from my perspective, no objections to splice-to-tty going 
away
altogether.

> and I'd be happy to try to limit splice to tty's to maybe just the one
> case that pppd uses.

To be exact, pppd is just providing a pty with which other (now all 
extinct?)
applications can do nefarious things.

> Maybe that VPN thing already has the pty in non-blocking mode, for
> example, and we could make the tty splicing fail for any blocking op?

FWIW, the SSL VPN tool did indeed have the pty in non-blocking mode.

Oliver
Linus Torvalds Jan. 3, 2024, 9:57 p.m. UTC | #5 
On Wed, 3 Jan 2024 at 13:34, Oliver Giles <ohw.giles@gmail.com> wrote:
>
> I'm happy to report that that particular SSL VPN tool is no longer
> around.

Ahh, well that simplifies things and we can then just remove the tty
splice support again.

Of course, maybe then somebody else will report on some other odd
user, but ... fingers crossed.

                Linus
diff mbox series

Patch

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 06414e43e0b5..50c2957a9c7f 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -465,7 +465,6 @@  static const struct file_operations tty_fops = {
 	.llseek		= no_llseek,
 	.read_iter	= tty_read,
 	.write_iter	= tty_write,
-	.splice_read	= copy_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.poll		= tty_poll,
 	.unlocked_ioctl	= tty_ioctl,
@@ -480,7 +479,6 @@  static const struct file_operations console_fops = {
 	.llseek		= no_llseek,
 	.read_iter	= tty_read,
 	.write_iter	= redirected_tty_write,
-	.splice_read	= copy_splice_read,
 	.splice_write	= iter_file_splice_write,
 	.poll		= tty_poll,
 	.unlocked_ioctl	= tty_ioctl,