Message-ID: <2025011041-refocus-operator-f48e@gregkh>
Date: Fri, 10 Jan 2025 15:52:13 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Gil Pedersen <gpdev@...st.dk>
Cc: linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	Jiri Slaby <jirislaby@...nel.org>
Subject: Re: [PATCH 1/1] tty: respond to TIOCGWINSZ when hung

On Tue, Jan 07, 2025 at 12:44:28PM +0100, Gil Pedersen wrote:
> > On 23 Dec 2024, at 18.56, Greg Kroah-Hartman <gregkh@...uxfoundation.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

Powered by blists - more mailing lists