[PATCH 0/3] tty: n_hdlc: fix hangup handling

[PATCH 0/3] tty: n_hdlc: fix hangup handling

[Johan Hovold]

An LLM incorrectly flagged three hangup issues in n_hdlc as leading to
hangups stuck in uninterruptible sleep.

This used to be the case before commit c96cf923a98d ("tty: Don't block
on IO when ldisc change is pending") which incidentally fixed the stuck
hangup issues by turning tty_io_nonblock() into an indirect hangup
check.

Since then read() and write() can return -EAGAIN also for files opened
in blocking mode on ldisc changes, which looks questionable. This change
was added to address a regression introduced by a series adding missing
locking to tty_reopen() but should probably be reconsidered. [1]

This series addresses the underlying hangup issues and incorrect -EAGAIN
on hangup meanwhile.

Johan


[1] https://lore.kernel.org/all/alpine.LRH.2.02.1809191330590.19146@file01.intranet.prod.int.rdu2.redhat.com/


Johan Hovold (3):
  tty: n_hdlc: fix missing write() hangup check
  tty: n_hdlc: fix read() hangup race
  tty: n_hdlc: fix read() hangup check

 drivers/tty/n_hdlc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

-- 
2.55.0

[PATCH 1/3] tty: n_hdlc: fix missing write() hangup check

[Johan Hovold]

Add the missing hangup check to the n_hdlc write() wait loop so that
hangups are detected and -EIO is returned as expected.

Note that before commit c96cf923a98d ("tty: Don't block on IO when ldisc
change is pending") a task hanging up the tty would get stuck in
uninterruptible sleep when there are blocked writers, but since then
write() incorrectly returns -EAGAIN instead.

The false-positive stuck hangup issue was flagged by an LLM.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/n_hdlc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
index 895b850a5950..0fbc44968a44 100644
--- a/drivers/tty/n_hdlc.c
+++ b/drivers/tty/n_hdlc.c
@@ -529,6 +529,11 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);
 
+		if (tty_hung_up_p(file)) {
+			error = -EIO;
+			break;
+		}
+
 		tbuf = n_hdlc_buf_get(&n_hdlc->tx_free_buf_list);
 		if (tbuf)
 			break;
-- 
2.55.0

Re: [PATCH 1/3] tty: n_hdlc: fix missing write() hangup check

[sashiko-bot]

> Add the missing hangup check to the n_hdlc write() wait loop so that
> hangups are detected and -EIO is returned as expected.
> 
> Note that before commit c96cf923a98d ("tty: Don't block on IO when ldisc
> change is pending") a task hanging up the tty would get stuck in
> uninterruptible sleep when there are blocked writers, but since then
> write() incorrectly returns -EAGAIN instead.
> 
> The false-positive stuck hangup issue was flagged by an LLM.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Johan Hovold <johan@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028132049.728457-1-johan@kernel.org?part=1

[PATCH 2/3] tty: n_hdlc: fix read() hangup race

[Johan Hovold]

Make sure to set the task state before checking the break conditions in
the read() wait loop to avoid missing a racing wakeup.

Note that before commit c96cf923a98d ("tty: Don't block on IO when ldisc
change is pending") a racing task hanging up the tty could get stuck in
uninterruptible sleep when there are blocked readers, but since then
read() incorrectly returns -EAGAIN instead.

The false-positive stuck hangup issue was flagged by an LLM.

Fixes: 1035b63d3c6f ("n_hdlc: fix read and write locking")
Cc: stable@vger.kernel.org	# 2.6.38
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/n_hdlc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
index 0fbc44968a44..02676ae9b112 100644
--- a/drivers/tty/n_hdlc.c
+++ b/drivers/tty/n_hdlc.c
@@ -431,6 +431,8 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
 	add_wait_queue(&tty->read_wait, &wait);
 
 	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+
 		if (test_bit(TTY_OTHER_CLOSED, &tty->flags)) {
 			ret = -EIO;
 			break;
@@ -438,8 +440,6 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
 		if (tty_hung_up_p(file))
 			break;
 
-		set_current_state(TASK_INTERRUPTIBLE);
-
 		rbuf = n_hdlc_buf_get(&n_hdlc->rx_buf_list);
 		if (rbuf)
 			break;
-- 
2.55.0

Re: [PATCH 2/3] tty: n_hdlc: fix read() hangup race

[sashiko-bot]

> Make sure to set the task state before checking the break conditions in
> the read() wait loop to avoid missing a racing wakeup.
> 
> Note that before commit c96cf923a98d ("tty: Don't block on IO when ldisc
> change is pending") a racing task hanging up the tty could get stuck in
> uninterruptible sleep when there are blocked readers, but since then
> read() incorrectly returns -EAGAIN instead.
> 
> The false-positive stuck hangup issue was flagged by an LLM.
> 
> Fixes: 1035b63d3c6f ("n_hdlc: fix read and write locking")
> Cc: stable@vger.kernel.org	# 2.6.38
> Signed-off-by: Johan Hovold <johan@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028132049.728457-1-johan@kernel.org?part=2

[PATCH 3/3] tty: n_hdlc: fix read() hangup check

[Johan Hovold]

When a tty is hung up, the file ops are generally set to
hung_up_tty_fops, but this is not the case for, for example,
/dev/console.

Make sure to check the TTY_HUPPING flag so that a hangup is detected and
read() returns zero also with /dev/console (as is done for n_tty [1]).

Note that before commit c96cf923a98d ("tty: Don't block on IO when ldisc
change is pending") a task hanging up the tty would get stuck in
uninterruptible sleep when there are blocked readers, but since then
read() incorrectly returns -EAGAIN instead.

The false-positive stuck hangup issue was flagged by an LLM.

[1] 28b0f8a6962a ("tty: make n_tty_read() always abort if hangup is in progress")

Fixes: 1035b63d3c6f ("n_hdlc: fix read and write locking")
Cc: stable@vger.kernel.org	# 2.6.38
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/n_hdlc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
index 02676ae9b112..1a3a276f2db3 100644
--- a/drivers/tty/n_hdlc.c
+++ b/drivers/tty/n_hdlc.c
@@ -437,7 +437,8 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
 			ret = -EIO;
 			break;
 		}
-		if (tty_hung_up_p(file))
+
+		if (tty_hung_up_p(file) || test_bit(TTY_HUPPING, &tty->flags))
 			break;
 
 		rbuf = n_hdlc_buf_get(&n_hdlc->rx_buf_list);
-- 
2.55.0

Re: [PATCH 3/3] tty: n_hdlc: fix read() hangup check

[sashiko-bot]

> When a tty is hung up, the file ops are generally set to
> hung_up_tty_fops, but this is not the case for, for example,
> /dev/console.
> 
> Make sure to check the TTY_HUPPING flag so that a hangup is detected and
> read() returns zero also with /dev/console (as is done for n_tty [1]).
> 
> Note that before commit c96cf923a98d ("tty: Don't block on IO when ldisc
> change is pending") a task hanging up the tty would get stuck in
> uninterruptible sleep when there are blocked readers, but since then
> read() incorrectly returns -EAGAIN instead.
> 
> The false-positive stuck hangup issue was flagged by an LLM.
> 
> [1] 28b0f8a6962a ("tty: make n_tty_read() always abort if hangup is in progress")
> 
> Fixes: 1035b63d3c6f ("n_hdlc: fix read and write locking")
> Cc: stable@vger.kernel.org	# 2.6.38
> Signed-off-by: Johan Hovold <johan@kernel.org>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028132049.728457-1-johan@kernel.org?part=3