Re: [PATCH v2] tty: pty: preserve open slaves after rejected locked open

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Oct 02, 2026 at 04:24:55PM -0400, Akira Patafio wrote:
> Opening a locked PTY slave fails with EIO, but the failure path sets
> TTY_IO_ERROR on the shared slave tty. Previously opened slave files then
> fail I/O even though their master is still open.

Ok, in digging a lot, I think this is "as intended".  You MUST call
unlockpt() in userspace to unlock a locked pty, and if you don't do
that, this all fails as you have seen.

Where in the POSIX spec say that if you attempt to open a locked PTY,
that it's allowed to keep working?  You are in control of the pty, so
it's not like this is some other user doing this lock/unlock stuff,
right?  What do other Unixes do if you attempt to do this type of thing?

And what about the pty tests in the LTP test suite, did you run them to
verify that your change didn't actually break anything there?

> tty_open() releases a file after the slave open callback fails. Merely
> skipping TTY_IO_ERROR on the rejected open is insufficient: pty_close()
> can mistake the failed file for the last slave and close the master.
> Counting tty references is also insufficient because a real last close
> can race the failed open and its release.
> 
> Track which slave files opened successfully. Ignore failed files during
> close, and mark the master peer closed only when the last successful
> slave file closes. In-kernel callers may pass NULL instead of a file;
> retain the count-based close behavior for those callers. A selftest
> checks locked opens with zero, one, and two existing slave files, plus
> ordinary last-slave close behavior.
> 
> Fixes: 699390354da6 ("pty: Ignore slave pty close() if never successfully opened")
> Assisted-by: LLM
> Signed-off-by: Akira Patafio <kokokoala4211@gmail.com>
> ---
> Changes in v2:
> - Preserve the existing NULL-file callback behavior for in-kernel TTY users.
> - Add the Assisted-by tag requested in review.
> 
> Tests:
> - The selftest failed on an unpatched Linux 6.12.69 kernel at the
>   pre-existing slave write with errno EIO (TAP fail:1).
> - Built the patched 7.3.0-rc5 x86_64 kernel with legacy PTYs and Speakup
>   enabled, without compiler warnings. Booted it in QEMU/HVF and ran the
>   selftest in the guest: TAP pass:1 fail:0, exit status 0.
> 
>  drivers/tty/pty.c                             |  34 +++-
>  drivers/tty/tty_io.c                          |   1 +
>  include/linux/tty.h                           |   1 +
>  .../testing/selftests/filesystems/.gitignore  |   1 +
>  tools/testing/selftests/filesystems/Makefile  |   2 +-
>  .../selftests/filesystems/pty_locked_open.c   | 187 ++++++++++++++++++
>  6 files changed, 222 insertions(+), 4 deletions(-)
>  create mode 100644 tools/testing/selftests/filesystems/pty_locked_open.c
> 
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index cc7f709..4024c5d 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -46,12 +46,31 @@ static DEFINE_MUTEX(devpts_mutex);
>  
>  static void pty_close(struct tty_struct *tty, struct file *filp)
>  {
> +	struct tty_file_private *priv = filp ? filp->private_data : NULL;
> +	struct tty_file_private *other;
> +	bool another_open = false;
> +
>  	if (tty->driver->subtype == PTY_TYPE_MASTER)
>  		WARN_ON(tty->count > 1);
>  	else {
> -		if (tty_io_error(tty))
> +		/* tty_release() also calls close after an unsuccessful open. */
> +		if (priv && !priv->pty_opened)
> +			return;
> +		/* In-kernel users have no per-file open state. */
> +		if (!priv && tty->count > 2)
>  			return;
> -		if (tty->count > 2)
> +		/* tty->count includes opens that have not succeeded yet. */
> +		spin_lock(&tty->files_lock);
> +		if (priv)
> +			priv->pty_opened = false;
> +		list_for_each_entry(other, &tty->tty_files, list) {
> +			if (other->pty_opened) {
> +				another_open = true;
> +				break;
> +			}
> +		}
> +		spin_unlock(&tty->files_lock);
> +		if (another_open || tty_io_error(tty))
>  			return;
>  	}
>  	set_bit(TTY_IO_ERROR, &tty->flags);
> @@ -223,14 +242,23 @@ static int pty_open(struct tty_struct *tty, struct file *filp)
>  
>  	if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
>  		goto out;
> +	/* A rejected open must not disrupt already-open slave descriptors. */
>  	if (test_bit(TTY_PTY_LOCK, &tty->link->flags))
> -		goto out;
> +		return -EIO;
>  	if (tty->driver->subtype == PTY_TYPE_SLAVE && tty->link->count != 1)
>  		goto out;
>  
>  	clear_bit(TTY_IO_ERROR, &tty->flags);
>  	clear_bit(TTY_OTHER_CLOSED, &tty->link->flags);
>  	set_bit(TTY_THROTTLED, &tty->flags);
> +	/* Record a successful open before another slave can close. */
> +	if (filp && tty->driver->subtype == PTY_TYPE_SLAVE) {
> +		struct tty_file_private *priv = filp->private_data;
> +
> +		spin_lock(&tty->files_lock);
> +		priv->pty_opened = true;
> +		spin_unlock(&tty->files_lock);

It's kind of obvious that this is LLM-generated code as it loves to add
random new booleans and doesn't know about properly locking logic in the
kernel.

Think about why you are attempting to use a lock here, and yet the rest
of the function didn't use a lock at all, doesn't that feel "odd" to
you?

Be _VERY_ careful when using a LLM to create a patch like this...

thanks,

greg k-h