Linux Serial subsystem development
 help / color / mirror / Atom feed
* [PATCH v2 0/1] serial: core: Don't clobber the baud rate on hangup via B0
@ 2026-10-12  1:49 Maciej W. Rozycki
  2026-10-12  1:49 ` [PATCH v2 1/1] " Maciej W. Rozycki
  0 siblings, 1 reply; 5+ messages in thread
From: Maciej W. Rozycki @ 2026-10-12  1:49 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Hugo Villeneuve, Kees Cook, Lukas Wunner, John Ogness,
	Jiayuan Chen, Gerhard Engleder, linux-serial, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

Greg,

 As we discussed at LPC, this is an update to the fix previously posted as 
an RFC.  I have verified it with the system originally affected, running 
the console at 115200bps; see the attached console output logs for the bad 
vs fixed case.  All that needed to be done to reproduce it was to keep 
killing getty(8)s repeatedly until init(8) decided something was wrong and 
chose to put their inittab(5) entries into a snooze, leaving the line(s) 
at the wrong speed setting.

 NB this fixes the missing %B0 literal markup as a part of the doc update.  
I've concluded it's not worth splitting off into a separate patch.  Let me 
know if you think otherwise.

 Previous iteration (v1) at
<https://lore.kernel.org/r/alpine.DEB.2.21.2605242242330.1450@angie.orcam.me.uk/>.

  Maciej

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/plain; CHARSET=US-ASCII; NAME=swarm-bad.log, Size: 1526 bytes --]

^[[H^[[J\rswarm.orcam.me.uk login: \0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0ð\0þ\0\0ð\0\0ð\0þ\0þ\0\0ð\0\0ð\0þ\0\0\0À\0þ\0þ\0þ\0\0ð\0þ\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0þ\0\0ð\0\0ð\0þ\0\0\0À\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0ð\0\0\0À\0\0ð\0þ\0\0ð\0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0ð\0\0\0À\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0þ\0þ\0þ\0\0\0\0\0\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0\0ð\0\0ð\0\0\0À\0þ\0þ\0þ\0þ\0þ\0þ\0þ\0þ\0\0\0\0\0\0\0ð\0\0ð\0\0ð\0\0ð\0þ\0þ\0\0\0\0\0\0þ\0þ\0\0ð\0þ\0\0\0À\0þ\0þ\0\0ð\0þ\0\0ð\0\0\0\0\0\0\0ð\0\0ð\0\0\0À\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0\0À\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0\0\0À\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0þ\0\0ð\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0þ\0þ\0þ\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0þ\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0þ\0þ\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0\0\0\0\0\0ð\0þ\0\0\0\0\0\0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0ð\0þ\0\0ð\0\0ð\0þ\0þ\0\0ð\0\0ð\0þ\0\0\0À\0þ\0þ\0þ\0\0ð\0þ\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0þ\0\0ð\0\0ð\0þ\0\0\0À\0\0ð\0þ\0\0\0\0\0\0\0ð\0\0ð\0\0ð\0\0\0À\0\0ð\0þ\0\0ð\0þ\0þ\0þ\0\0\0À\0\0ð\0\0ð\0\0\0À\0\0ð\0\0\0\0\0\0\0ð\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0þ\0þ\0þ\0\0\0\0\0\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0\0À\0þ\0þ\0þ\0þ\0þ\0þ\0þ\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0ð\0\0ð\0þ\0þ\0\0\0\0\0\0þ\0þ\0\0ð\0þ\0\0\0À\0þ\0þ\0\0ð\0þ\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0\0\0À\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0\0À\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0\0\0À\0\0ð\0þ\0\0\0\0\0\0\0ð\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0þ\0\0ð\0\0ð\0þ\0\0\0\0\0\0\0ð\0\0ð\0þ\0þ\0þ\0\0ð\0\0\0\0\0\0\0ø\0\0ð\0þ\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0þ\0þ\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0\0\0\0\0\0ð\0þ\0\0\0\0\0\0þ\0þ\0\0\0\0\0\0þ\0\0ð\0\0ð\0þ\0\0ð\0\0ð\0þ\0þ\0\0ð\0\0ð\0þ\0\0\0À\0þ\0þ\0þ\0\0ð\0þ\0\0ð\0\0\0\0\0\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0þ\0þ\0\0\0\0\0\0\0ø\0\0ð\0þ\0þ\0þ\0þ\0þ\0þ\0þ\0\0ð\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0\0\0\0\0þ\0þ\0\0ð\0\0ð\0þ\0þ\0þ\0þ\0þ\0\0\0À\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0\0ð\0\0ð\0þ\0\0\0À\0þ\0þ\0\0\0\0\0\0\0ð\0\0ð\0þ\0\0ð\0þ\0þ\0\0ð\0þ\0þ\0\0\0\0\0\0\0ð\0\0ð\0\0\0À\0þ\0þ\0\0\0\0\0\0þ\0þ\0þ\0\0ð\0þ\0þ\0þ\0\0ð\0þ\0\0\0\0\0\0\0ø\0\0ð\0\0ð\0\0ð\0þ\0þ\0þ\0þ\0þ\0\0ð\0þ\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0ð\0þ\0\0ð\0þ\0þ\0þ\0þ\0\0ð\0þ\0\0\0À\0þ\0þ\0þ\0þ\0\0\0\0\0\0\0ð\0þ\0\0\0\0\0^[[H^[[J\rswarm.orcam.me.uk login: 

[-- Attachment #3: Type: text/plain, Size: 229 bytes --]

^[[H^[[Jswarm.orcam.me.uk login: INIT: Id "S0" respawning too fast: disabled for 5 minutes
INIT: Id "S1" respawning too fast: disabled for 5 minutes
INIT: no more processes left in this runlevel
^[[H^[[Jswarm.orcam.me.uk login: 

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

* [PATCH v2 1/1] serial: core: Don't clobber the baud rate on hangup via B0
  2026-10-12  1:49 [PATCH v2 0/1] serial: core: Don't clobber the baud rate on hangup via B0 Maciej W. Rozycki
@ 2026-10-12  1:49 ` Maciej W. Rozycki
  2026-10-12  2:10   ` sashiko-bot
  0 siblings, 1 reply; 5+ messages in thread
From: Maciej W. Rozycki @ 2026-10-12  1:49 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Hugo Villeneuve, Kees Cook, Lukas Wunner, John Ogness,
	Jiayuan Chen, Gerhard Engleder, linux-serial, linux-kernel

Requesting hangup via the B0 baud rate has this nasty side effect of 
clobbering the previously set rate for the port unless it happens to be 
9600bps or the port minimum rate if higher.  Consequently if there's a 
consumer still active at the other end, then it receives garbled data 
from any other producers outputting messages to the port, such as 
init(8) or the kernel console.

There doesn't appear to be any particular reason for this clobbering 
other than that we have been doing it since 2.1.131, so take a saner 
approach and keep track of the most recently set baud rate and then use 
it whenever hangup has been requested.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
Changes from v1, 
<https://lore.kernel.org/r/alpine.DEB.2.21.2605242242330.1450@angie.orcam.me.uk/>:

- Cache the most recently set actual baud rate in port->hup_speed and use 
  it whenever the B0 rate has been requested since the old termios may be 
  NULL or have the B0 rate recorded.
---
 drivers/tty/serial/serial_core.c |   16 +++++++++-------
 include/linux/serial_core.h      |    1 +
 2 files changed, 10 insertions(+), 7 deletions(-)

linux-serial-uart-get-baud-rate-hup.diff
Index: linux-macro/drivers/tty/serial/serial_core.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/serial_core.c
+++ linux-macro/drivers/tty/serial/serial_core.c
@@ -461,15 +461,15 @@ EXPORT_SYMBOL(uart_update_timeout);
  * @max: maximum acceptable baud rate
  *
  * Decode the termios structure into a numeric baud rate, taking account of the
- * magic 38400 baud rate (with spd_* flags), and mapping the %B0 rate to 9600
- * baud or min argument, whichever is greater.
+ * magic 38400 baud rate (with spd_* flags), and mapping the %B0 rate to the
+ * previously used rate provided by @port.
  *
  * If the new baud rate is invalid, try the @old termios setting. If it's still
  * invalid, clip to the nearest chip supported rate.
  *
- * The @termios structure is updated to reflect the baud rate we're actually
- * going to be using. Don't do this for the case where B0 is requested ("hang
- * up").
+ * The @termios and @port structures are updated to reflect the baud rate
+ * we're actually going to be using. Don't do this for the case where %B0
+ * is requested ("hang up").
  *
  * Locking: caller dependent
  */
@@ -514,10 +514,12 @@ uart_get_baud_rate(struct uart_port *por
 		 * Special case: B0 rate.
 		 */
 		if (baud == 0)
-			return max(min, 9600);
+			return port->hup_speed;
 
-		if (baud >= min && baud <= max)
+		if (baud >= min && baud <= max) {
+			port->hup_speed = baud;
 			return baud;
+		}
 
 		/*
 		 * If the range cannot be met then try again with
Index: linux-macro/include/linux/serial_core.h
===================================================================
--- linux-macro.orig/include/linux/serial_core.h
+++ linux-macro/include/linux/serial_core.h
@@ -569,6 +569,7 @@ struct uart_port {
 
 	bool			hw_stopped;		/* sw-assisted CTS flow state */
 	bool			cons_flow;		/* user specified console flow control */
+	speed_t			hup_speed;		/* speed to use for B0 rate (hang-up) */
 	unsigned int		mctrl;			/* current modem ctrl settings */
 	unsigned int		frame_time;		/* frame timing in ns */
 	unsigned int		type;			/* port type */

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

* Re: [PATCH v2 1/1] serial: core: Don't clobber the baud rate on hangup via B0
  2026-10-12  1:49 ` [PATCH v2 1/1] " Maciej W. Rozycki
@ 2026-10-12  2:10   ` sashiko-bot
  2026-10-12 14:37     ` Maciej W. Rozycki
  0 siblings, 1 reply; 5+ messages in thread
From: sashiko-bot @ 2026-10-12  2:10 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: linux-serial

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tty: division by zero in uart_get_divisor() on uninitialized port->hup_speed
- [Medium] tty: serial: data race on port->hup_speed in uart_get_baud_rate()
--

commit 3c831138bb5fb81f1a02fc66246315af11bf6bbc
Author: Maciej W. Rozycki <macro@orcam.me.uk>

serial: core: Don't clobber the baud rate on hangup via B0

Requesting hangup via the B0 baud rate clobbers the previously set rate
unless it is 9600bps or the port minimum. This commit tracks the most
recently set baud rate in port->hup_speed and uses it whenever the B0
rate is requested.

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 319a4b427f3a3..3390e4d142127 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
[ ... ]
> @@ -514,10 +514,12 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
>  		 * Special case: B0 rate.
>  		 */
>  		if (baud == 0)
> -			return max(min, 9600);
> +			return port->hup_speed;

[Severity: High]
Does this risk a division by zero panic?

If a new struct uart_port is initialized, port->hup_speed defaults to 0.
If a B0 rate is requested before any valid rate is set (such as via
console=ttyS0,0 or an early tcsetattr with B0), this returns 0.

This 0 is then passed down to helpers like uart_get_divisor(), which
calculates the divisor as:

	quot = DIV_ROUND_CLOSEST(port->uartclk, 16 * baud);

It can also reach uart_update_timeout(), which calculates the frame time:

	port->frame_time = (unsigned int)DIV64_U64_ROUND_UP(temp, baud);

In both cases, passing a 0 baud rate will result in a divide-by-zero
hardware exception.

>  
> -		if (baud >= min && baud <= max)
> +		if (baud >= min && baud <= max) {
> +			port->hup_speed = baud;

[Severity: Medium]
Could this result in a data race on port->hup_speed?

In the standard uart_set_termios() path, uart_get_baud_rate() is called
holding only port->mutex and termios_rwsem, not port->lock.

At the same time, atomic console write paths execute asynchronously from
hardirq or NMI holding only the nbcon lock. For example:

drivers/tty/serial/8250/8250_port.c:serial8250_console_restore() {
	...
	baud = serial8250_get_baud_rate(port, &termios, NULL);
	...
}

This calls into uart_get_baud_rate() without holding port->mutex. A
concurrent tcsetattr() and atomic console write can trigger an unprotected
data race on port->hup_speed. A torn read or write on this variable could
yield a garbage baud rate and lead to hardware misconfiguration.

>  			return baud;
> +		}
>  
>  		/*
>  		 * If the range cannot be met then try again with

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/alpine.DEB.2.21.2610120150230.55767@angie.orcam.me.uk?part=1

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

* Re: [PATCH v2 1/1] serial: core: Don't clobber the baud rate on hangup via B0
  2026-10-12  2:10   ` sashiko-bot
@ 2026-10-12 14:37     ` Maciej W. Rozycki
  2026-10-13 13:16       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 5+ messages in thread
From: Maciej W. Rozycki @ 2026-10-12 14:37 UTC (permalink / raw)
  To: sashiko-reviews
  Cc: Greg Kroah-Hartman, Jiri Slaby, Hugo Villeneuve, Kees Cook,
	Lukas Wunner, John Ogness, Jiayuan Chen, Gerhard Engleder,
	linux-serial, linux-kernel

On Mon, 12 Oct 2026, sashiko-bot@kernel.org wrote:

> > @@ -514,10 +514,12 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
> >  		 * Special case: B0 rate.
> >  		 */
> >  		if (baud == 0)
> > -			return max(min, 9600);
> > +			return port->hup_speed;
> 
> [Severity: High]
> Does this risk a division by zero panic?
> 
> If a new struct uart_port is initialized, port->hup_speed defaults to 0.
> If a B0 rate is requested before any valid rate is set (such as via
> console=ttyS0,0 or an early tcsetattr with B0), this returns 0.

 Yeah, this has glimpsed in my mind and then I lost it.  Indeed we need to 
keep the fallback 9600/min logic after all for the initial setup.  Easily 
fixed since it's not in the hot path.

> >  
> > -		if (baud >= min && baud <= max)
> > +		if (baud >= min && baud <= max) {
> > +			port->hup_speed = baud;
> 
> [Severity: Medium]
> Could this result in a data race on port->hup_speed?
> 
> In the standard uart_set_termios() path, uart_get_baud_rate() is called
> holding only port->mutex and termios_rwsem, not port->lock.
> 
> At the same time, atomic console write paths execute asynchronously from
> hardirq or NMI holding only the nbcon lock. For example:
> 
> drivers/tty/serial/8250/8250_port.c:serial8250_console_restore() {
> 	...
> 	baud = serial8250_get_baud_rate(port, &termios, NULL);
> 	...
> }
> 
> This calls into uart_get_baud_rate() without holding port->mutex. A
> concurrent tcsetattr() and atomic console write can trigger an unprotected
> data race on port->hup_speed. A torn read or write on this variable could
> yield a garbage baud rate and lead to hardware misconfiguration.

 This seems like the only exception across all the serial drivers in the 
use of uart_get_baud_rate().  Since this is always called to restore the 
most recent baud rate setting only, perhaps the path of least resistance 
would be just to refrain from updating port->hup_speed if called with the 
port lock held?

 Otherwise it would require rewriting all the 60+ call sites to use the 
port lock to guard the whole sequence from a call to uart_get_baud_rate() 
through to the write of the settings obtained to the port registers, which 
seems an overkill to me and asking for trouble with some odd hardware.

 I'll experiment with the idea outlined or will be happy with any hints 
otherwise.

  Maciej

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

* Re: [PATCH v2 1/1] serial: core: Don't clobber the baud rate on hangup via B0
  2026-10-12 14:37     ` Maciej W. Rozycki
@ 2026-10-13 13:16       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 5+ messages in thread
From: Greg Kroah-Hartman @ 2026-10-13 13:16 UTC (permalink / raw)
  To: Maciej W. Rozycki
  Cc: sashiko-reviews, Jiri Slaby, Hugo Villeneuve, Kees Cook,
	Lukas Wunner, John Ogness, Jiayuan Chen, Gerhard Engleder,
	linux-serial, linux-kernel

On Mon, Oct 12, 2026 at 03:37:01PM +0100, Maciej W. Rozycki wrote:
> On Mon, 12 Oct 2026, sashiko-bot@kernel.org wrote:
> 
> > > @@ -514,10 +514,12 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
> > >  		 * Special case: B0 rate.
> > >  		 */
> > >  		if (baud == 0)
> > > -			return max(min, 9600);
> > > +			return port->hup_speed;
> > 
> > [Severity: High]
> > Does this risk a division by zero panic?
> > 
> > If a new struct uart_port is initialized, port->hup_speed defaults to 0.
> > If a B0 rate is requested before any valid rate is set (such as via
> > console=ttyS0,0 or an early tcsetattr with B0), this returns 0.
> 
>  Yeah, this has glimpsed in my mind and then I lost it.  Indeed we need to 
> keep the fallback 9600/min logic after all for the initial setup.  Easily 
> fixed since it's not in the hot path.
> 
> > >  
> > > -		if (baud >= min && baud <= max)
> > > +		if (baud >= min && baud <= max) {
> > > +			port->hup_speed = baud;
> > 
> > [Severity: Medium]
> > Could this result in a data race on port->hup_speed?
> > 
> > In the standard uart_set_termios() path, uart_get_baud_rate() is called
> > holding only port->mutex and termios_rwsem, not port->lock.
> > 
> > At the same time, atomic console write paths execute asynchronously from
> > hardirq or NMI holding only the nbcon lock. For example:
> > 
> > drivers/tty/serial/8250/8250_port.c:serial8250_console_restore() {
> > 	...
> > 	baud = serial8250_get_baud_rate(port, &termios, NULL);
> > 	...
> > }
> > 
> > This calls into uart_get_baud_rate() without holding port->mutex. A
> > concurrent tcsetattr() and atomic console write can trigger an unprotected
> > data race on port->hup_speed. A torn read or write on this variable could
> > yield a garbage baud rate and lead to hardware misconfiguration.
> 
>  This seems like the only exception across all the serial drivers in the 
> use of uart_get_baud_rate().  Since this is always called to restore the 
> most recent baud rate setting only, perhaps the path of least resistance 
> would be just to refrain from updating port->hup_speed if called with the 
> port lock held?
> 
>  Otherwise it would require rewriting all the 60+ call sites to use the 
> port lock to guard the whole sequence from a call to uart_get_baud_rate() 
> through to the write of the settings obtained to the port registers, which 
> seems an overkill to me and asking for trouble with some odd hardware.
> 
>  I'll experiment with the idea outlined or will be happy with any hints 
> otherwise.

That idea seems sane, thanks for working on this.

greg k-h

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

end of thread, other threads:[~2026-10-13 13:16 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-12  1:49 [PATCH v2 0/1] serial: core: Don't clobber the baud rate on hangup via B0 Maciej W. Rozycki
2026-10-12  1:49 ` [PATCH v2 1/1] " Maciej W. Rozycki
2026-10-12  2:10   ` sashiko-bot
2026-10-12 14:37     ` Maciej W. Rozycki
2026-10-13 13:16       ` Greg Kroah-Hartman

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