[PATCH v3 1/4] serial: revert guards in uart_wait_modem

Linux kernel -stable discussions
 help / color / mirror / Atom feed

* [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status()
       [not found] <20260907064418.92953-1-johan@kernel.org>
@ 2026-09-07  6:44 ` Johan Hovold
  2026-09-07  6:47   ` Jiri Slaby
  2026-09-07  6:44 ` [PATCH v3 2/4] serial: fix ioctl hangup race Johan Hovold
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Johan Hovold @ 2026-09-07  6:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: linux-serial, linux-kernel, Johan Hovold, stable

Mixing scope-based and regular cleanup is discouraged and
uart_port_deref() used by uart_wait_modem_status() falls in the latter
category.

Revert the premature guard conversion in preparation for fixing a hangup
race.

Fixes: 56609c050051 ("serial: serial_core: use guard()s")
Cc: stable@vger.kernel.org	# 6.18
Cc: Jiri Slaby (SUSE) <jirislaby@kernel.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 95774b0f1484..1553bc6cbe7b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1213,15 +1213,16 @@ static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)
 	uport = uart_port_ref(state);
 	if (!uport)
 		return -EIO;
-	scoped_guard(uart_port_lock_irq, uport) {
-		memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
-		uart_enable_ms(uport);
-	}
+	uart_port_lock_irq(uport);
+	memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
+	uart_enable_ms(uport);
+	uart_port_unlock_irq(uport);
 
 	add_wait_queue(&port->delta_msr_wait, &wait);
 	for (;;) {
-		scoped_guard(uart_port_lock_irq, uport)
-			memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
+		uart_port_lock_irq(uport);
+		memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
+		uart_port_unlock_irq(uport);
 
 		set_current_state(TASK_INTERRUPTIBLE);
 
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status()
  2026-09-07  6:44 ` [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status() Johan Hovold
@ 2026-09-07  6:47   ` Jiri Slaby
  2026-09-07  6:51     ` Jiri Slaby
  0 siblings, 1 reply; 7+ messages in thread
From: Jiri Slaby @ 2026-09-07  6:47 UTC (permalink / raw)
  To: Johan Hovold, Greg Kroah-Hartman; +Cc: linux-serial, linux-kernel, stable

On 07. 09. 26, 8:44, Johan Hovold wrote:
> Mixing scope-based and regular cleanup is discouraged and
> uart_port_deref() used by uart_wait_modem_status() falls in the latter
> category.
> 
> Revert the premature guard conversion in preparation for fixing a hangup
> race.

It's not premature. Just convert uart_port_ref() to guards too. Gotos suck.

> Fixes: 56609c050051 ("serial: serial_core: use guard()s")
> Cc: stable@vger.kernel.org	# 6.18
> Cc: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/tty/serial/serial_core.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 95774b0f1484..1553bc6cbe7b 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1213,15 +1213,16 @@ static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)
>   	uport = uart_port_ref(state);
>   	if (!uport)
>   		return -EIO;
> -	scoped_guard(uart_port_lock_irq, uport) {
> -		memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
> -		uart_enable_ms(uport);
> -	}
> +	uart_port_lock_irq(uport);
> +	memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
> +	uart_enable_ms(uport);
> +	uart_port_unlock_irq(uport);
>   
>   	add_wait_queue(&port->delta_msr_wait, &wait);
>   	for (;;) {
> -		scoped_guard(uart_port_lock_irq, uport)
> -			memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
> +		uart_port_lock_irq(uport);
> +		memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
> +		uart_port_unlock_irq(uport);
>   
>   		set_current_state(TASK_INTERRUPTIBLE);
>   


-- 
js
suse labs

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status()
  2026-09-07  6:47   ` Jiri Slaby
@ 2026-09-07  6:51     ` Jiri Slaby
  2026-09-07  7:37       ` Johan Hovold
  0 siblings, 1 reply; 7+ messages in thread
From: Jiri Slaby @ 2026-09-07  6:51 UTC (permalink / raw)
  To: Johan Hovold, Greg Kroah-Hartman; +Cc: linux-serial, linux-kernel, stable

On 07. 09. 26, 8:47, Jiri Slaby wrote:
> On 07. 09. 26, 8:44, Johan Hovold wrote:
>> Mixing scope-based and regular cleanup is discouraged and
>> uart_port_deref() used by uart_wait_modem_status() falls in the latter
>> category.
>>
>> Revert the premature guard conversion in preparation for fixing a hangup
>> race.
> 
> It's not premature. Just convert uart_port_ref() to guards too. Gotos suck.

(Or don't use gotos and I will.)

-- 
js
suse labs


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status()
  2026-09-07  6:51     ` Jiri Slaby
@ 2026-09-07  7:37       ` Johan Hovold
  0 siblings, 0 replies; 7+ messages in thread
From: Johan Hovold @ 2026-09-07  7:37 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel, stable

On Mon, Sep 07, 2026 at 08:51:26AM +0200, Jiri Slaby wrote:
> On 07. 09. 26, 8:47, Jiri Slaby wrote:
> > On 07. 09. 26, 8:44, Johan Hovold wrote:
> >> Mixing scope-based and regular cleanup is discouraged and
> >> uart_port_deref() used by uart_wait_modem_status() falls in the latter
> >> category.
> >>
> >> Revert the premature guard conversion in preparation for fixing a hangup
> >> race.
> > 
> > It's not premature. Just convert uart_port_ref() to guards too. Gotos suck.
> 
> (Or don't use gotos and I will.)

I guess you meant "don't use guards" here. :)

Gotos don't suck. Obscure driver-specific guard constructs do, though.

So I'm with netdev here, who discourage its use. [1]

I haven't missed your fondness of them, though, and if you want to
continue that path I think it's best if you do it as a follow-on change
(e.g. to facilitate backporting and as it is a larger change to the
driver, unrelated to the fix).

Johan


[1] https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 2/4] serial: fix ioctl hangup race
       [not found] <20260907064418.92953-1-johan@kernel.org>
  2026-09-07  6:44 ` [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status() Johan Hovold
@ 2026-09-07  6:44 ` Johan Hovold
  2026-09-07  6:44 ` [PATCH v3 3/4] serial: fix TIOCMIWAIT race Johan Hovold
  2026-09-07  6:44 ` [PATCH v3 4/4] serial: abort TIOCMIWAIT on hangup Johan Hovold
  3 siblings, 0 replies; 7+ messages in thread
From: Johan Hovold @ 2026-09-07  6:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: linux-serial, linux-kernel, Johan Hovold, stable

The tty ioctls can race with hangup and end up calling into a tty
driver for a device that is already gone or powered down.

Add the missing checks to make sure the port has not been hung up before
accessing the hardware to avoid issues like kernel panic due to
unclocked accesses.

Note that TIOCGSERIAL, TIOCGICOUNT do not access hardware and are
therefore not affected by the race.

Fixes: 04f378b198da ("tty: BKL pushdown")
Cc: stable@vger.kernel.org	# 2.6.26
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 1553bc6cbe7b..4213fa3dc988 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -896,7 +896,7 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
 	upf_t old_flags, new_flags;
 	int retval;
 
-	if (!uport)
+	if (!uport || tty_io_error(tty))
 		return -EIO;
 
 	new_port = new_info->port;
@@ -1119,7 +1119,7 @@ static int uart_break_ctl(struct tty_struct *tty, int break_state)
 	guard(mutex)(&port->mutex);
 
 	uport = uart_port_check(state);
-	if (!uport)
+	if (!uport || tty_io_error(tty))
 		return -EIO;
 
 	if (uport->type != PORT_UNKNOWN && uport->ops->break_ctl)
@@ -1144,7 +1144,7 @@ static int uart_do_autoconfig(struct tty_struct *tty, struct uart_state *state)
 	 */
 	scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &port->mutex) {
 		uport = uart_port_check(state);
-		if (!uport)
+		if (!uport || tty_io_error(tty))
 			return -EIO;
 
 		if (tty_port_users(port) != 1)
@@ -1199,7 +1199,7 @@ static void uart_enable_ms(struct uart_port *uport)
  * FIXME: This wants extracting into a common all driver implementation
  * of TIOCMWAIT using tty_port.
  */
-static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)
+static int uart_wait_modem_status(struct tty_struct *tty, struct uart_state *state, unsigned long arg)
 {
 	struct uart_port *uport;
 	struct tty_port *port = &state->port;
@@ -1213,11 +1213,21 @@ static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)
 	uport = uart_port_ref(state);
 	if (!uport)
 		return -EIO;
+
+	mutex_lock(&port->mutex);
+	if (tty_io_error(tty)) {
+		mutex_unlock(&port->mutex);
+		ret = -EIO;
+		goto out_deref;
+	}
+
 	uart_port_lock_irq(uport);
 	memcpy(&cprev, &uport->icount, sizeof(struct uart_icount));
 	uart_enable_ms(uport);
 	uart_port_unlock_irq(uport);
 
+	mutex_unlock(&port->mutex);
+
 	add_wait_queue(&port->delta_msr_wait, &wait);
 	for (;;) {
 		uart_port_lock_irq(uport);
@@ -1246,6 +1256,7 @@ static int uart_wait_modem_status(struct uart_state *state, unsigned long arg)
 	}
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&port->delta_msr_wait, &wait);
+out_deref:
 	uart_port_deref(uport);
 
 	return ret;
@@ -1568,7 +1579,7 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
 
 	/* This should only be used when the hardware is present. */
 	if (cmd == TIOCMIWAIT)
-		return uart_wait_modem_status(state, arg);
+		return uart_wait_modem_status(tty, state, arg);
 
 	/* rs485_config requires more locking than others */
 	if (cmd == TIOCSRS485)
@@ -1647,7 +1658,7 @@ static void uart_set_termios(struct tty_struct *tty,
 	guard(mutex)(&state->port.mutex);
 
 	uport = uart_port_check(state);
-	if (!uport)
+	if (!uport || tty_io_error(tty))
 		return;
 
 	/*
@@ -1799,7 +1810,14 @@ static void uart_wait_until_sent(struct tty_struct *tty, int timeout)
 	 * 'timeout' / 'expire' give us the maximum amount of time
 	 * we wait.
 	 */
-	while (!port->ops->tx_empty(port)) {
+	for (;;) {
+		mutex_lock(&state->port.mutex);
+		if (tty_io_error(tty) || port->ops->tx_empty(port)) {
+			mutex_unlock(&state->port.mutex);
+			break;
+		}
+		mutex_unlock(&state->port.mutex);
+
 		msleep_interruptible(jiffies_to_msecs(char_time));
 		if (signal_pending(current))
 			break;
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 3/4] serial: fix TIOCMIWAIT race
       [not found] <20260907064418.92953-1-johan@kernel.org>
  2026-09-07  6:44 ` [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status() Johan Hovold
  2026-09-07  6:44 ` [PATCH v3 2/4] serial: fix ioctl hangup race Johan Hovold
@ 2026-09-07  6:44 ` Johan Hovold
  2026-09-07  6:44 ` [PATCH v3 4/4] serial: abort TIOCMIWAIT on hangup Johan Hovold
  3 siblings, 0 replies; 7+ messages in thread
From: Johan Hovold @ 2026-09-07  6:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: linux-serial, linux-kernel, Johan Hovold, sashiko-bot, stable

The task state must be updated before checking the wakeup condition to
avoid missing a racing modem status update.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/r/20260904115542.E20D11F00A3D@smtp.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 4213fa3dc988..c2c2c8fe1a4c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1230,12 +1230,12 @@ static int uart_wait_modem_status(struct tty_struct *tty, struct uart_state *sta
 
 	add_wait_queue(&port->delta_msr_wait, &wait);
 	for (;;) {
+		set_current_state(TASK_INTERRUPTIBLE);
+
 		uart_port_lock_irq(uport);
 		memcpy(&cnow, &uport->icount, sizeof(struct uart_icount));
 		uart_port_unlock_irq(uport);
 
-		set_current_state(TASK_INTERRUPTIBLE);
-
 		if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
 		    ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
 		    ((arg & TIOCM_CD)  && (cnow.dcd != cprev.dcd)) ||
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 4/4] serial: abort TIOCMIWAIT on hangup
       [not found] <20260907064418.92953-1-johan@kernel.org>
                   ` (2 preceding siblings ...)
  2026-09-07  6:44 ` [PATCH v3 3/4] serial: fix TIOCMIWAIT race Johan Hovold
@ 2026-09-07  6:44 ` Johan Hovold
  3 siblings, 0 replies; 7+ messages in thread
From: Johan Hovold @ 2026-09-07  6:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: linux-serial, linux-kernel, Johan Hovold, sashiko-bot, stable

Add the missing hangup check to the TIOCMIWAIT wait loop to avoid going
back to sleep (indefinitely) after being woken on hangup.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: sashiko-bot@kernel.org
Link: https://lore.kernel.org/r/20260904115542.E20D11F00A3D@smtp.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/serial_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c2c2c8fe1a4c..45d160743ea0 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1244,6 +1244,11 @@ static int uart_wait_modem_status(struct tty_struct *tty, struct uart_state *sta
 			break;
 		}
 
+		if (tty_io_error(tty)) {
+			ret = -EIO;
+			break;
+		}
+
 		schedule();
 
 		/* see if a signal did it */
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-09-07  7:38 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260907064418.92953-1-johan@kernel.org>
2026-09-07  6:44 ` [PATCH v3 1/4] serial: revert guards in uart_wait_modem_status() Johan Hovold
2026-09-07  6:47   ` Jiri Slaby
2026-09-07  6:51     ` Jiri Slaby
2026-09-07  7:37       ` Johan Hovold
2026-09-07  6:44 ` [PATCH v3 2/4] serial: fix ioctl hangup race Johan Hovold
2026-09-07  6:44 ` [PATCH v3 3/4] serial: fix TIOCMIWAIT race Johan Hovold
2026-09-07  6:44 ` [PATCH v3 4/4] serial: abort TIOCMIWAIT on hangup Johan Hovold

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox