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

[Akira Patafio <kokokoala4211@gmail.com>]

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.

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. 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")
Signed-off-by: Akira Patafio <kokokoala4211@gmail.com>
---
The pre-fix EIO transition was reproduced on an Android 5.10.240 device.
The new selftest cross-compiles for arm64 with -Werror. I have not booted
a kernel with this patch yet.

 drivers/tty/pty.c                             |  32 ++-
 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, 220 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..3cc1494 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -46,12 +46,29 @@ static DEFINE_MUTEX(devpts_mutex);
 
 static void pty_close(struct tty_struct *tty, struct file *filp)
 {
+	struct tty_file_private *priv = filp->private_data;
+	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->pty_opened)
+			return;
+		/* tty->count includes opens that have not succeeded yet. */
+		spin_lock(&tty->files_lock);
+		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)
 			return;
-		if (tty->count > 2)
+		if (tty_io_error(tty))
 			return;
 	}
 	set_bit(TTY_IO_ERROR, &tty->flags);
@@ -223,14 +240,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 (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);
+	}
 	return 0;
 
 out:
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 4856903..e0b5f97 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -186,6 +186,7 @@ int tty_alloc_file(struct file *file)
 	priv = kmalloc_obj(*priv);
 	if (!priv)
 		return -ENOMEM;
+	priv->pty_opened = false;
 
 	file->private_data = priv;
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 0a46e40..c44d11f 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -247,6 +247,7 @@ struct tty_file_private {
 	struct tty_struct *tty;
 	struct file *file;
 	struct list_head list;
+	bool pty_opened;
 };
 
 /**
diff --git a/tools/testing/selftests/filesystems/.gitignore b/tools/testing/selftests/filesystems/.gitignore
index 9eb185f..5cbbfca 100644
--- a/tools/testing/selftests/filesystems/.gitignore
+++ b/tools/testing/selftests/filesystems/.gitignore
@@ -6,4 +6,5 @@ file_stressor
 anon_inode_test
 kernfs_test
 idmapped_tmpfile
+pty_locked_open
 ustat_test
diff --git a/tools/testing/selftests/filesystems/Makefile b/tools/testing/selftests/filesystems/Makefile
index 03be337..b92a4f1 100644
--- a/tools/testing/selftests/filesystems/Makefile
+++ b/tools/testing/selftests/filesystems/Makefile
@@ -2,7 +2,7 @@
 
 CFLAGS += $(KHDR_INCLUDES)
 TEST_GEN_PROGS := devpts_pts file_stressor anon_inode_test kernfs_test fclog ustat_test
-TEST_GEN_PROGS += idmapped_tmpfile
+TEST_GEN_PROGS += idmapped_tmpfile pty_locked_open
 TEST_GEN_PROGS_EXTENDED := dnotify_test
 
 include ../lib.mk
diff --git a/tools/testing/selftests/filesystems/pty_locked_open.c b/tools/testing/selftests/filesystems/pty_locked_open.c
new file mode 100644
index 0000000..eba661c
--- /dev/null
+++ b/tools/testing/selftests/filesystems/pty_locked_open.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <poll.h>
+#include <stdio.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+#include <asm/ioctls.h>
+
+#include "kselftest.h"
+
+#ifndef TIOCGPTPEER
+int main(void)
+{
+	ksft_exit_skip("TIOCGPTPEER is unavailable\n");
+}
+#else
+static int expect_no_master_hup(int master)
+{
+	struct pollfd pfd = { .fd = master, .events = POLLIN | POLLOUT };
+	int ret;
+
+	ret = poll(&pfd, 1, 0);
+	if (ret < 0 || (pfd.revents & POLLHUP)) {
+		ksft_print_msg("master poll returned %d (revents %#x)\n",
+			       ret, pfd.revents);
+		return -1;
+	}
+	return 0;
+}
+
+static int expect_master_hup(int master)
+{
+	struct pollfd pfd = { .fd = master, .events = POLLIN | POLLOUT };
+	int ret;
+
+	ret = poll(&pfd, 1, 0);
+	if (ret != 1 || !(pfd.revents & POLLHUP)) {
+		ksft_print_msg("master poll returned %d (revents %#x), expected HUP\n",
+			       ret, pfd.revents);
+		return -1;
+	}
+	return 0;
+}
+
+static int expect_locked_peer_rejected(int master)
+{
+	int fd;
+
+	errno = 0;
+	fd = ioctl(master, TIOCGPTPEER, O_RDWR | O_NOCTTY | O_CLOEXEC);
+	if (fd >= 0) {
+		ksft_print_msg("locked slave unexpectedly opened\n");
+		close(fd);
+		return -1;
+	}
+	if (errno != EIO) {
+		ksft_print_msg("locked slave open returned errno %d, expected EIO\n",
+			       errno);
+		return -1;
+	}
+	return 0;
+}
+
+static int transfer_byte(int master, int slave, char value)
+{
+	struct pollfd pfd = { .fd = master, .events = POLLIN };
+	char received;
+	ssize_t ret;
+
+	ret = write(slave, &value, 1);
+	if (ret != 1) {
+		ksft_print_msg("slave write returned %zd (errno %d)\n", ret, errno);
+		return -1;
+	}
+
+	ret = poll(&pfd, 1, 1000);
+	if (ret != 1 || !(pfd.revents & POLLIN)) {
+		ksft_print_msg("master poll returned %zd (revents %#x)\n",
+			       ret, pfd.revents);
+		return -1;
+	}
+
+	ret = read(master, &received, 1);
+	if (ret != 1 || received != value) {
+		ksft_print_msg("master read returned %zd (value %#x)\n",
+			       ret, ret == 1 ? (unsigned char)received : 0);
+		return -1;
+	}
+
+	return 0;
+}
+
+int main(void)
+{
+	int master = -1, slave_a = -1, slave_b = -1;
+	int locked = 1, unlocked = 0;
+	int ret = KSFT_FAIL;
+
+	ksft_print_header();
+	ksft_set_plan(1);
+
+	master = open("/dev/ptmx", O_RDWR | O_NOCTTY | O_CLOEXEC);
+	if (master < 0) {
+		ksft_print_msg("cannot open /dev/ptmx: %d\n", errno);
+		goto out;
+	}
+	if (expect_no_master_hup(master) ||
+	    expect_locked_peer_rejected(master) ||
+	    expect_no_master_hup(master))
+		goto out;
+
+	if (ioctl(master, TIOCSPTLCK, &unlocked) < 0) {
+		ksft_print_msg("cannot unlock slave: %d\n", errno);
+		goto out;
+	}
+	slave_a = ioctl(master, TIOCGPTPEER, O_RDWR | O_NOCTTY | O_CLOEXEC);
+	if (slave_a < 0) {
+		ksft_print_msg("cannot open slave A: %d\n", errno);
+		goto out;
+	}
+	if (transfer_byte(master, slave_a, 'A'))
+		goto out;
+	if (ioctl(master, TIOCSPTLCK, &locked) < 0) {
+		ksft_print_msg("cannot relock slave: %d\n", errno);
+		goto out;
+	}
+	if (expect_locked_peer_rejected(master) ||
+	    transfer_byte(master, slave_a, 'B'))
+		goto out;
+	if (ioctl(master, TIOCSPTLCK, &unlocked) < 0) {
+		ksft_print_msg("cannot unlock slave: %d\n", errno);
+		goto out;
+	}
+	slave_b = ioctl(master, TIOCGPTPEER, O_RDWR | O_NOCTTY | O_CLOEXEC);
+	if (slave_b < 0) {
+		ksft_print_msg("cannot open slave B: %d\n", errno);
+		goto out;
+	}
+	if (transfer_byte(master, slave_a, 'C') ||
+	    transfer_byte(master, slave_b, 'D'))
+		goto out;
+
+	if (ioctl(master, TIOCSPTLCK, &locked) < 0) {
+		ksft_print_msg("cannot relock slave: %d\n", errno);
+		goto out;
+	}
+	if (transfer_byte(master, slave_a, 'E') ||
+	    transfer_byte(master, slave_b, 'F'))
+		goto out;
+
+	if (expect_locked_peer_rejected(master) ||
+	    transfer_byte(master, slave_a, 'G') ||
+	    transfer_byte(master, slave_b, 'H'))
+		goto out;
+	if (close(slave_b)) {
+		ksft_print_msg("cannot close slave B: %d\n", errno);
+		slave_b = -1;
+		goto out;
+	}
+	slave_b = -1;
+	if (expect_no_master_hup(master) ||
+	    transfer_byte(master, slave_a, 'I'))
+		goto out;
+	if (close(slave_a)) {
+		ksft_print_msg("cannot close slave A: %d\n", errno);
+		slave_a = -1;
+		goto out;
+	}
+	slave_a = -1;
+	if (expect_master_hup(master))
+		goto out;
+	ret = KSFT_PASS;
+
+out:
+	if (slave_b >= 0)
+		close(slave_b);
+	if (slave_a >= 0)
+		close(slave_a);
+	if (master >= 0)
+		close(master);
+	ksft_test_result(ret == KSFT_PASS,
+			 "locked slave open preserves master and existing slave I/O\n");
+	ksft_exit(ret == KSFT_PASS);
+}
+#endif
-- 
2.50.1