[PATCH 0/1] Fix to allow more correct isatty()

[PATCH 0/1] Fix to allow more correct isatty()

[Gil Pedersen]

This patch comes from an issue discovered in systemd, where it can fail to
restore a text TTY session after a GUI session is stopped when compiled
with musl libc.

The specific systemd integration issue is currently resolved in the musl
master branch by closer aligning the implementation with glibc, but the
underlying isatty() implementation is still flawed since it can return 0
(false) for something that is definitely a TTY. Essentially both musl
and glibc give up correctly handling this case on Linux due to
inadequate/buggy kernel APIs.

Thus I am proposing this patch as a solution to fix the kernel APIs. An
alternative fix could be to add a new IOCTL to specifically handle this,
but that seems overkill.

Link: https://github.com/systemd/systemd/pull/34039
Link: https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/70711
Link: https://www.openwall.com/lists/musl/2024/08/20/2
Link: https://git.musl-libc.org/cgit/musl/commit/src/unistd/isatty.c?id=c94a0c16f08894ce3be6dafb0fe80baa77a6ff2a
Link: https://sourceware.org/bugzilla/show_bug.cgi?id=32103

Gil Pedersen (1):
  tty: respond to TIOCGWINSZ when hung

 drivers/tty/tty_io.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

-- 
2.45.2

[PATCH 1/1] tty: respond to TIOCGWINSZ when hung

[Gil Pedersen]

Userspace libc implementations of the isatty() POSIX system interface
are currently unable to reliably determine if a fd is really a tty when
it is hung.

Specifically glibc libc returns the success status of a TCGETS ioctl.
This will return an incorrect result when the TTY is hung, since an EIO
is unconditionally returned. Ie. an isatty() will return 0, wrongly
indicating that something that definitely is a TTY, is not a TTY.

Userspace implementations could potentially remap EIO errors to a
success to work around this. This will likely work in 99.99% of cases,
but there is no guarantee that a TCGETS ioctl on a non-TTY fd will not
also return EIO, making the isatty() call return a false positive!

This commit enables a specific non-driver, non-ldisc, ioctl to continue
working after the TTY is hung. The TIOCGWINSZ ioctl was chosen since it
is readonly, and only access tty_struct.winsize (and its mutex), and is
already used for the isatty() implementation in musl. The glibc
implementation will need to be updated to use the TIOCGWINSZ ioctl,
either as a direct replacement, or more conservatively, as a fallback
test when the TCGETS ioctl fails with EIO.

Note that TCGETS is not available to use for this, since it is
implemented at the ldisc level, which can not be called into once the
TTY is hung.

Signed-off-by: Gil Pedersen <gpdev@gpost.dk>
---
 drivers/tty/tty_io.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 9771072da177..678fcc9b8264 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -157,6 +157,8 @@ static long tty_compat_ioctl(struct file *file, unsigned int cmd,
 static int __tty_fasync(int fd, struct file *filp, int on);
 static int tty_fasync(int fd, struct file *filp, int on);
 static void release_tty(struct tty_struct *tty, int idx);
+static long hung_up_tty_ioctl(struct file *file, unsigned int cmd,
+				unsigned long arg);
 
 /**
  * free_tty_struct - free a disused tty
@@ -433,16 +435,10 @@ static __poll_t hung_up_tty_poll(struct file *filp, poll_table *wait)
 	return EPOLLIN | EPOLLOUT | EPOLLERR | EPOLLHUP | EPOLLRDNORM | EPOLLWRNORM;
 }
 
-static long hung_up_tty_ioctl(struct file *file, unsigned int cmd,
-		unsigned long arg)
-{
-	return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
-}
-
 static long hung_up_tty_compat_ioctl(struct file *file,
 				     unsigned int cmd, unsigned long arg)
 {
-	return cmd == TIOCSPGRP ? -ENOTTY : -EIO;
+	return hung_up_tty_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
 }
 
 static int hung_up_tty_fasync(int fd, struct file *file, int on)
@@ -2817,6 +2813,25 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	return retval;
 }
 
+static long hung_up_tty_ioctl(struct file *file, unsigned int cmd,
+		unsigned long arg)
+{
+	struct tty_struct *tty = file_tty(file);
+	struct tty_struct *real_tty;
+	void __user *p = (void __user *)arg;
+
+	real_tty = tty_pair_get_tty(tty);
+
+	switch (cmd) {
+	case TIOCGWINSZ:
+	return tiocgwinsz(real_tty, p);
+	case TIOCSPGRP:
+		return -ENOTTY;
+	}
+
+	return -EIO;
+}
+
 #ifdef CONFIG_COMPAT
 
 struct serial_struct32 {
-- 
2.45.2

Re: [PATCH 0/1] Fix to allow more correct isatty()

[nerdopolis]

On Thursday, November 21, 2024 6:12:53 AM EST Gil Pedersen wrote:
> This patch comes from an issue discovered in systemd, where it can fail to
> restore a text TTY session after a GUI session is stopped when compiled
> with musl libc.
> 
> The specific systemd integration issue is currently resolved in the musl
> master branch by closer aligning the implementation with glibc, but the
> underlying isatty() implementation is still flawed since it can return 0
> (false) for something that is definitely a TTY. Essentially both musl
> and glibc give up correctly handling this case on Linux due to
> inadequate/buggy kernel APIs.
> 
> Thus I am proposing this patch as a solution to fix the kernel APIs. An
> alternative fix could be to add a new IOCTL to specifically handle this,
> but that seems overkill.
> 
This is pretty cool as it fixes the /dev/console-is-a-disconnected-/dev/ttyS0
issue I found this year
https://github.com/systemd/systemd/pull/33690
and

https://lore.kernel.org/all/2669238.7s5MMGUR32@nerdopolis2/
namely on vt-less kernels that now have /dev/console as /dev/ttyS0 instead of
/dev/tty0 or a virtual device.


So yeah, when you have hardware and you've been using vt-enabled kernels
on it for years (where your /dev/ttyS0 doesn't even have an rs232 port, let
alone connected to anything), and then when you upgrade to a vt-less kernel,
sometime in the future, because of systemd's isatty() check failing, and
refusing to write to /dev/console, Plymouth's new log display facility in turn
gets far fewer logs from systemd, because of the ioctl failure in isatty()


MY idea for this solution was that kernels turning off CONFIG_VT_CONSOLE could
then turn on a CONFIG_NULL_TTY_CONSOLE that would get selected at the same time
the VT console would have, so that if distros choose to do so, the virtual
/dev/ttynull device to always work for User Space. (that way kernels that never
had CONFIG_VT_CONSOLE turned on won't have a behavior change as well)


(the one benefit to my idea is that Plymouth goes into text mode logging when
the console device is /dev/ttyS0, so Desktop distros would need to add a new
"plymouth.graphical" to their grub commands to get the splash to work again,
however when the console device is /dev/ttynull it automatically assumes that
the user wants a splash screen.) This is more specific to Plymouth though


However testing this patch and it also works, and this won't need a new
configuration change.
> Link: https://github.com/systemd/systemd/pull/34039
> Link: https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/70711
> Link: https://www.openwall.com/lists/musl/2024/08/20/2
> Link: https://git.musl-libc.org/cgit/musl/commit/src/unistd/isatty.c?id=c94a0c16f08894ce3be6dafb0fe80baa77a6ff2a
> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=32103
> 
> Gil Pedersen (1):
>   tty: respond to TIOCGWINSZ when hung
> 
>  drivers/tty/tty_io.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> 

Re: [PATCH 1/1] tty: respond to TIOCGWINSZ when hung

[Greg Kroah-Hartman]

On Thu, Nov 21, 2024 at 12:12:54PM +0100, Gil Pedersen wrote:
> Userspace libc implementations of the isatty() POSIX system interface
> are currently unable to reliably determine if a fd is really a tty when
> it is hung.
> 
> Specifically glibc libc returns the success status of a TCGETS ioctl.
> This will return an incorrect result when the TTY is hung, since an EIO
> is unconditionally returned. Ie. an isatty() will return 0, wrongly
> indicating that something that definitely is a TTY, is not a TTY.
> 
> Userspace implementations could potentially remap EIO errors to a
> success to work around this. This will likely work in 99.99% of cases,
> but there is no guarantee that a TCGETS ioctl on a non-TTY fd will not
> also return EIO, making the isatty() call return a false positive!
> 
> This commit enables a specific non-driver, non-ldisc, ioctl to continue
> working after the TTY is hung. The TIOCGWINSZ ioctl was chosen since it
> is readonly, and only access tty_struct.winsize (and its mutex), and is
> already used for the isatty() implementation in musl. The glibc
> implementation will need to be updated to use the TIOCGWINSZ ioctl,
> either as a direct replacement, or more conservatively, as a fallback
> test when the TCGETS ioctl fails with EIO.

This is a fun "hack", yes, but now you are encoding an odd "side affect"
into the system that everyone is going to rely on, well, eventually rely
on.  What code needs to be changed in userspace to determine this?

Why not just have a new ioctl that tells you if the tty really is hung
or not?  Why does isatty() need to know this, does POSIX require it?
And if it does, what does it say the ioctl command should be?

thanks,

greg k-h

Re: [PATCH 1/1] tty: respond to TIOCGWINSZ when hung

[Gil Pedersen]

> On 23 Dec 2024, at 18.56, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> On Thu, Nov 21, 2024 at 12:12:54PM +0100, Gil Pedersen wrote:
>> Userspace libc implementations of the isatty() POSIX system interface
>> are currently unable to reliably determine if a fd is really a tty when
>> it is hung.
>> 
>> Specifically glibc libc returns the success status of a TCGETS ioctl.
>> This will return an incorrect result when the TTY is hung, since an EIO
>> is unconditionally returned. Ie. an isatty() will return 0, wrongly
>> indicating that something that definitely is a TTY, is not a TTY.
>> 
>> Userspace implementations could potentially remap EIO errors to a
>> success to work around this. This will likely work in 99.99% of cases,
>> but there is no guarantee that a TCGETS ioctl on a non-TTY fd will not
>> also return EIO, making the isatty() call return a false positive!
>> 
>> This commit enables a specific non-driver, non-ldisc, ioctl to continue
>> working after the TTY is hung. The TIOCGWINSZ ioctl was chosen since it
>> is readonly, and only access tty_struct.winsize (and its mutex), and is
>> already used for the isatty() implementation in musl. The glibc
>> implementation will need to be updated to use the TIOCGWINSZ ioctl,
>> either as a direct replacement, or more conservatively, as a fallback
>> test when the TCGETS ioctl fails with EIO.
> 
> This is a fun "hack", yes, but now you are encoding an odd "side affect"
> into the system that everyone is going to rely on, well, eventually rely
> on.  What code needs to be changed in userspace to determine this?

The patch can definitely be considered a hack, but viewed with another
lens: a bugfix.

There is no specific reason that the call should return an EIO on hung
terminals, so making it always return the current value could be
considered more correct. POSIX tcgetwinsize(), which this ioctl maps
to, does not consider hung terminals, and expects it to return suitable
values whenever possible.

Userspace implementations will have to reconsider their handling of an
EIO error, as the isatty() call could still return an EIO if calling
into a non-TTY device. Unconditionally mapping it to a success, like
isatty_safe() in systemd, would be an error. Supporting both versions
would require a runtime check to determine which variant is used, where
the legacy version would accept the risk of a "wrong" EIO, while the
new version would treat it as a proper error.

> Why not just have a new ioctl that tells you if the tty really is hung
> or not?  Why does isatty() need to know this, does POSIX require it?
> And if it does, what does it say the ioctl command should be?

isatty() should not need to know if the TTY is hung, and besides cannot
safely call any ioctl to check this before it knows that it is indeed a
TTY. POSIX does not seem to include the concept of hung terminals.

A case could be made for introducing a new ioctl though, but it would
need a more generic approach, like the BSD FIODTYPE ioctl that exposes
a d_type property on chardev & block driver interfaces. If implemented
before calling into the VFS layer, it could make the isatty() call 100%
safe (on kernels that support the ioctl). Additionally, this would mean
that it can never return EIO, which makes userspace adaptions simpler,
since it can know that any returned EIO means that it is running on an
unpatched/legacy kernel and/or libc.

/Gil

Link: https://pubs.opengroup.org/onlinepubs/9799919799/
Link: https://github.com/systemd/systemd/blob/83c0b95f63417a36e67305fe9ad16a89ed53ef52/src/basic/terminal-util.c#L63-L79

Re: [PATCH 1/1] tty: respond to TIOCGWINSZ when hung

[Greg Kroah-Hartman]

On Tue, Jan 07, 2025 at 12:44:28PM +0100, Gil Pedersen wrote:
> > On 23 Dec 2024, at 18.56, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 
> > On Thu, Nov 21, 2024 at 12:12:54PM +0100, Gil Pedersen wrote:
> >> Userspace libc implementations of the isatty() POSIX system interface
> >> are currently unable to reliably determine if a fd is really a tty when
> >> it is hung.
> >> 
> >> Specifically glibc libc returns the success status of a TCGETS ioctl.
> >> This will return an incorrect result when the TTY is hung, since an EIO
> >> is unconditionally returned. Ie. an isatty() will return 0, wrongly
> >> indicating that something that definitely is a TTY, is not a TTY.
> >> 
> >> Userspace implementations could potentially remap EIO errors to a
> >> success to work around this. This will likely work in 99.99% of cases,
> >> but there is no guarantee that a TCGETS ioctl on a non-TTY fd will not
> >> also return EIO, making the isatty() call return a false positive!
> >> 
> >> This commit enables a specific non-driver, non-ldisc, ioctl to continue
> >> working after the TTY is hung. The TIOCGWINSZ ioctl was chosen since it
> >> is readonly, and only access tty_struct.winsize (and its mutex), and is
> >> already used for the isatty() implementation in musl. The glibc
> >> implementation will need to be updated to use the TIOCGWINSZ ioctl,
> >> either as a direct replacement, or more conservatively, as a fallback
> >> test when the TCGETS ioctl fails with EIO.
> > 
> > This is a fun "hack", yes, but now you are encoding an odd "side affect"
> > into the system that everyone is going to rely on, well, eventually rely
> > on.  What code needs to be changed in userspace to determine this?
> 
> The patch can definitely be considered a hack, but viewed with another
> lens: a bugfix.

All hacks could be viewed that way :)

> There is no specific reason that the call should return an EIO on hung
> terminals, so making it always return the current value could be
> considered more correct. POSIX tcgetwinsize(), which this ioctl maps
> to, does not consider hung terminals, and expects it to return suitable
> values whenever possible.

There's no specific reason, but we are stuck with what we have today as
that is how things work.  I'm more worried about making this change and
then nothing ever changes in userspace.  And userspace would never
"know" if it could or could not rely on this change, as some
necro-enterprise-systems never update their kernel.

> Userspace implementations will have to reconsider their handling of an
> EIO error, as the isatty() call could still return an EIO if calling
> into a non-TTY device. Unconditionally mapping it to a success, like
> isatty_safe() in systemd, would be an error. Supporting both versions
> would require a runtime check to determine which variant is used, where
> the legacy version would accept the risk of a "wrong" EIO, while the
> new version would treat it as a proper error.

How would such a runtime check work?  Do you have working patches for
existing userspace programs that want to know this that shows how this
all works?

We can't take api changes without a working userspace user, you know
that...

> > Why not just have a new ioctl that tells you if the tty really is hung
> > or not?  Why does isatty() need to know this, does POSIX require it?
> > And if it does, what does it say the ioctl command should be?
> 
> isatty() should not need to know if the TTY is hung, and besides cannot
> safely call any ioctl to check this before it knows that it is indeed a
> TTY. POSIX does not seem to include the concept of hung terminals.
> 
> A case could be made for introducing a new ioctl though, but it would
> need a more generic approach, like the BSD FIODTYPE ioctl that exposes
> a d_type property on chardev & block driver interfaces. If implemented
> before calling into the VFS layer, it could make the isatty() call 100%
> safe (on kernels that support the ioctl). Additionally, this would mean
> that it can never return EIO, which makes userspace adaptions simpler,
> since it can know that any returned EIO means that it is running on an
> unpatched/legacy kernel and/or libc.

Yes, that's why I suggested a new ioctl.

thanks,

greg k-h