diff mbox series

[v2] serial: Lock console when calling into driver before registration

Message ID 20240226192329.3281301-1-pcc@google.com
State Superseded
Headers show
Series [v2] serial: Lock console when calling into driver before registration | expand

Commit Message

Peter Collingbourne Feb. 26, 2024, 7:23 p.m. UTC 
During the handoff from earlycon to the real console driver, we have
two separate drivers operating on the same device concurrently. In the
case of the 8250 driver these concurrent accesses cause problems due
to the driver's use of banked registers, controlled by LCR.DLAB. It is
possible for the setup(), config_port(), pm() and set_mctrl() callbacks
to set DLAB, which can cause the earlycon code that intends to access
TX to instead access DLL, leading to missed output and corruption on
the serial line due to unintended modifications to the baud rate.

In particular, for setup() we have:

univ8250_console_setup()
-> serial8250_console_setup()
-> uart_set_options()
-> serial8250_set_termios()
-> serial8250_do_set_termios()
-> serial8250_do_set_divisor()

For config_port() we have:

serial8250_config_port()
-> autoconfig()

For pm() we have:

serial8250_pm()
-> serial8250_do_pm()
-> serial8250_set_sleep()

For set_mctrl() we have (for some devices):

serial8250_set_mctrl()
-> omap8250_set_mctrl()
-> __omap8250_set_mctrl()

To avoid such problems, let's make it so that the console is locked
during pre-registration calls to these callbacks, which will prevent
the earlycon driver from running concurrently.

Remove the partial solution to this problem in the 8250 driver
that locked the console only during autoconfig_irq(), as this would
result in a deadlock with the new approach. The console continues
to be locked during autoconfig_irq() because it can only be called
through uart_configure_port().

Although this patch introduces more locking than strictly necessary
(and in particular it also locks during the call to rs485_config()
which is not affected by this issue as far as I can tell), it follows
the principle that it is the responsibility of the generic console
code to manage the earlycon handoff by ensuring that earlycon and real
console driver code cannot run concurrently, and not the individual
drivers.

Signed-off-by: Peter Collingbourne <pcc@google.com>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
Link: https://linux-review.googlesource.com/id/I7cf8124dcebf8618e6b2ee543fa5b25532de55d8
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
---
 drivers/tty/serial/8250/8250_port.c |  6 ------
 drivers/tty/serial/serial_core.c    | 12 ++++++++++++
 kernel/printk/printk.c              | 21 ++++++++++++++++++---
 3 files changed, 30 insertions(+), 9 deletions(-)

Comments

Greg Kroah-Hartman March 2, 2024, 8:48 p.m. UTC | #1 
On Mon, Feb 26, 2024 at 11:23:26AM -0800, Peter Collingbourne wrote:
> During the handoff from earlycon to the real console driver, we have
> two separate drivers operating on the same device concurrently. In the
> case of the 8250 driver these concurrent accesses cause problems due
> to the driver's use of banked registers, controlled by LCR.DLAB. It is
> possible for the setup(), config_port(), pm() and set_mctrl() callbacks
> to set DLAB, which can cause the earlycon code that intends to access
> TX to instead access DLL, leading to missed output and corruption on
> the serial line due to unintended modifications to the baud rate.
> 
> In particular, for setup() we have:
> 
> univ8250_console_setup()
> -> serial8250_console_setup()
> -> uart_set_options()
> -> serial8250_set_termios()
> -> serial8250_do_set_termios()
> -> serial8250_do_set_divisor()
> 
> For config_port() we have:
> 
> serial8250_config_port()
> -> autoconfig()
> 
> For pm() we have:
> 
> serial8250_pm()
> -> serial8250_do_pm()
> -> serial8250_set_sleep()
> 
> For set_mctrl() we have (for some devices):
> 
> serial8250_set_mctrl()
> -> omap8250_set_mctrl()
> -> __omap8250_set_mctrl()
> 
> To avoid such problems, let's make it so that the console is locked
> during pre-registration calls to these callbacks, which will prevent
> the earlycon driver from running concurrently.
> 
> Remove the partial solution to this problem in the 8250 driver
> that locked the console only during autoconfig_irq(), as this would
> result in a deadlock with the new approach. The console continues
> to be locked during autoconfig_irq() because it can only be called
> through uart_configure_port().
> 
> Although this patch introduces more locking than strictly necessary
> (and in particular it also locks during the call to rs485_config()
> which is not affected by this issue as far as I can tell), it follows
> the principle that it is the responsibility of the generic console
> code to manage the earlycon handoff by ensuring that earlycon and real
> console driver code cannot run concurrently, and not the individual
> drivers.
> 
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Reviewed-by: John Ogness <john.ogness@linutronix.de>
> Link: https://linux-review.googlesource.com/id/I7cf8124dcebf8618e6b2ee543fa5b25532de55d8

Why is a link to a gerrit review with no context other than this same
commit needed here?

> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> ---
>  drivers/tty/serial/8250/8250_port.c |  6 ------
>  drivers/tty/serial/serial_core.c    | 12 ++++++++++++
>  kernel/printk/printk.c              | 21 ++++++++++++++++++---
>  3 files changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 8ca061d3bbb9..1d65055dde27 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1329,9 +1329,6 @@ static void autoconfig_irq(struct uart_8250_port *up)
>  		inb_p(ICP);
>  	}
>  
> -	if (uart_console(port))
> -		console_lock();
> -
>  	/* forget possible initially masked and pending IRQ */
>  	probe_irq_off(probe_irq_on());
>  	save_mcr = serial8250_in_MCR(up);
> @@ -1371,9 +1368,6 @@ static void autoconfig_irq(struct uart_8250_port *up)
>  	if (port->flags & UPF_FOURPORT)
>  		outb_p(save_ICP, ICP);
>  
> -	if (uart_console(port))
> -		console_unlock();
> -
>  	port->irq = (irq > 0) ? irq : 0;
>  }
>  
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index d6a58a9e072a..ff85ebd3a007 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2608,7 +2608,12 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
>  			port->type = PORT_UNKNOWN;
>  			flags |= UART_CONFIG_TYPE;
>  		}
> +		/* Synchronize with possible boot console. */
> +		if (uart_console(port))
> +			console_lock();
>  		port->ops->config_port(port, flags);
> +		if (uart_console(port))
> +			console_unlock();
>  	}
>  
>  	if (port->type != PORT_UNKNOWN) {
> @@ -2616,6 +2621,10 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
>  
>  		uart_report_port(drv, port);
>  
> +		/* Synchronize with possible boot console. */
> +		if (uart_console(port))
> +			console_lock();
> +
>  		/* Power up port for set_mctrl() */
>  		uart_change_pm(state, UART_PM_STATE_ON);
>  
> @@ -2632,6 +2641,9 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
>  
>  		uart_rs485_config(port);
>  
> +		if (uart_console(port))
> +			console_unlock();
> +
>  		/*
>  		 * If this driver supports console, and it hasn't been
>  		 * successfully registered yet, try to re-register it.
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index f2444b581e16..f51e4e5a869d 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3263,6 +3263,21 @@ static int __init keep_bootcon_setup(char *str)
>  
>  early_param("keep_bootcon", keep_bootcon_setup);
>  
> +static int console_call_setup(struct console *newcon, char *options)
> +{
> +	int err;
> +
> +	if (!newcon->setup)
> +		return 0;
> +
> +	/* Synchronize with possible boot console. */
> +	console_lock();
> +	err = newcon->setup(newcon, options);
> +	console_unlock();
> +
> +	return err;
> +}
> +
>  /*
>   * This is called by register_console() to try to match
>   * the newly registered console with any of the ones selected
> @@ -3298,8 +3313,8 @@ static int try_enable_preferred_console(struct console *newcon,
>  			if (_braille_register_console(newcon, c))
>  				return 0;
>  
> -			if (newcon->setup &&
> -			    (err = newcon->setup(newcon, c->options)) != 0)
> +			err = console_call_setup(newcon, c->options);
> +			if (err != 0)

Didn't checkpatch complain about this?  It should be:
			if (err)
right?


>  				return err;
>  		}
>  		newcon->flags |= CON_ENABLED;
> @@ -3325,7 +3340,7 @@ static void try_enable_default_console(struct console *newcon)
>  	if (newcon->index < 0)
>  		newcon->index = 0;
>  
> -	if (newcon->setup && newcon->setup(newcon, NULL) != 0)
> +	if (console_call_setup(newcon, NULL) != 0)
>  		return;

No way to pass an error back here?

thanks,

greg k-h
Peter Collingbourne March 4, 2024, 9:44 p.m. UTC | #2 
On Sat, Mar 2, 2024 at 12:48 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Feb 26, 2024 at 11:23:26AM -0800, Peter Collingbourne wrote:
> > During the handoff from earlycon to the real console driver, we have
> > two separate drivers operating on the same device concurrently. In the
> > case of the 8250 driver these concurrent accesses cause problems due
> > to the driver's use of banked registers, controlled by LCR.DLAB. It is
> > possible for the setup(), config_port(), pm() and set_mctrl() callbacks
> > to set DLAB, which can cause the earlycon code that intends to access
> > TX to instead access DLL, leading to missed output and corruption on
> > the serial line due to unintended modifications to the baud rate.
> >
> > In particular, for setup() we have:
> >
> > univ8250_console_setup()
> > -> serial8250_console_setup()
> > -> uart_set_options()
> > -> serial8250_set_termios()
> > -> serial8250_do_set_termios()
> > -> serial8250_do_set_divisor()
> >
> > For config_port() we have:
> >
> > serial8250_config_port()
> > -> autoconfig()
> >
> > For pm() we have:
> >
> > serial8250_pm()
> > -> serial8250_do_pm()
> > -> serial8250_set_sleep()
> >
> > For set_mctrl() we have (for some devices):
> >
> > serial8250_set_mctrl()
> > -> omap8250_set_mctrl()
> > -> __omap8250_set_mctrl()
> >
> > To avoid such problems, let's make it so that the console is locked
> > during pre-registration calls to these callbacks, which will prevent
> > the earlycon driver from running concurrently.
> >
> > Remove the partial solution to this problem in the 8250 driver
> > that locked the console only during autoconfig_irq(), as this would
> > result in a deadlock with the new approach. The console continues
> > to be locked during autoconfig_irq() because it can only be called
> > through uart_configure_port().
> >
> > Although this patch introduces more locking than strictly necessary
> > (and in particular it also locks during the call to rs485_config()
> > which is not affected by this issue as far as I can tell), it follows
> > the principle that it is the responsibility of the generic console
> > code to manage the earlycon handoff by ensuring that earlycon and real
> > console driver code cannot run concurrently, and not the individual
> > drivers.
> >
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > Reviewed-by: John Ogness <john.ogness@linutronix.de>
> > Link: https://linux-review.googlesource.com/id/I7cf8124dcebf8618e6b2ee543fa5b25532de55d8
>
> Why is a link to a gerrit review with no context other than this same
> commit needed here?

I usually add that to my patches so that the progression of the patch
can be tracked. See:
https://lore.kernel.org/all/CAMn1gO53G6-sZE8RiLAD2uERbW1XtvyZbRonNGbHonzD058yAA@mail.gmail.com/
https://lore.kernel.org/all/CAHk-=whFbgy4RXG11c_=S7O-248oWmwB_aZOcWzWMVh3w7=RCw@mail.gmail.com/

> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/tty/serial/8250/8250_port.c |  6 ------
> >  drivers/tty/serial/serial_core.c    | 12 ++++++++++++
> >  kernel/printk/printk.c              | 21 ++++++++++++++++++---
> >  3 files changed, 30 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 8ca061d3bbb9..1d65055dde27 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -1329,9 +1329,6 @@ static void autoconfig_irq(struct uart_8250_port *up)
> >               inb_p(ICP);
> >       }
> >
> > -     if (uart_console(port))
> > -             console_lock();
> > -
> >       /* forget possible initially masked and pending IRQ */
> >       probe_irq_off(probe_irq_on());
> >       save_mcr = serial8250_in_MCR(up);
> > @@ -1371,9 +1368,6 @@ static void autoconfig_irq(struct uart_8250_port *up)
> >       if (port->flags & UPF_FOURPORT)
> >               outb_p(save_ICP, ICP);
> >
> > -     if (uart_console(port))
> > -             console_unlock();
> > -
> >       port->irq = (irq > 0) ? irq : 0;
> >  }
> >
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index d6a58a9e072a..ff85ebd3a007 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -2608,7 +2608,12 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
> >                       port->type = PORT_UNKNOWN;
> >                       flags |= UART_CONFIG_TYPE;
> >               }
> > +             /* Synchronize with possible boot console. */
> > +             if (uart_console(port))
> > +                     console_lock();
> >               port->ops->config_port(port, flags);
> > +             if (uart_console(port))
> > +                     console_unlock();
> >       }
> >
> >       if (port->type != PORT_UNKNOWN) {
> > @@ -2616,6 +2621,10 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
> >
> >               uart_report_port(drv, port);
> >
> > +             /* Synchronize with possible boot console. */
> > +             if (uart_console(port))
> > +                     console_lock();
> > +
> >               /* Power up port for set_mctrl() */
> >               uart_change_pm(state, UART_PM_STATE_ON);
> >
> > @@ -2632,6 +2641,9 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
> >
> >               uart_rs485_config(port);
> >
> > +             if (uart_console(port))
> > +                     console_unlock();
> > +
> >               /*
> >                * If this driver supports console, and it hasn't been
> >                * successfully registered yet, try to re-register it.
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index f2444b581e16..f51e4e5a869d 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3263,6 +3263,21 @@ static int __init keep_bootcon_setup(char *str)
> >
> >  early_param("keep_bootcon", keep_bootcon_setup);
> >
> > +static int console_call_setup(struct console *newcon, char *options)
> > +{
> > +     int err;
> > +
> > +     if (!newcon->setup)
> > +             return 0;
> > +
> > +     /* Synchronize with possible boot console. */
> > +     console_lock();
> > +     err = newcon->setup(newcon, options);
> > +     console_unlock();
> > +
> > +     return err;
> > +}
> > +
> >  /*
> >   * This is called by register_console() to try to match
> >   * the newly registered console with any of the ones selected
> > @@ -3298,8 +3313,8 @@ static int try_enable_preferred_console(struct console *newcon,
> >                       if (_braille_register_console(newcon, c))
> >                               return 0;
> >
> > -                     if (newcon->setup &&
> > -                         (err = newcon->setup(newcon, c->options)) != 0)
> > +                     err = console_call_setup(newcon, c->options);
> > +                     if (err != 0)
>
> Didn't checkpatch complain about this?  It should be:
>                         if (err)
> right?

No, it didn't complain. (It complained about the pre-existing
assignment within an if, which is why I moved it out, but not about
that.) Looks like if (err) is more popular, so I changed it in v3.

> >                               return err;
> >               }
> >               newcon->flags |= CON_ENABLED;
> > @@ -3325,7 +3340,7 @@ static void try_enable_default_console(struct console *newcon)
> >       if (newcon->index < 0)
> >               newcon->index = 0;
> >
> > -     if (newcon->setup && newcon->setup(newcon, NULL) != 0)
> > +     if (console_call_setup(newcon, NULL) != 0)
> >               return;
>
> No way to pass an error back here?

The only caller, register_console(), ignores errors from this
function. Arguably it shouldn't, but that's a pre-existing issue.

Peter
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 8ca061d3bbb9..1d65055dde27 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1329,9 +1329,6 @@  static void autoconfig_irq(struct uart_8250_port *up)
 		inb_p(ICP);
 	}
 
-	if (uart_console(port))
-		console_lock();
-
 	/* forget possible initially masked and pending IRQ */
 	probe_irq_off(probe_irq_on());
 	save_mcr = serial8250_in_MCR(up);
@@ -1371,9 +1368,6 @@  static void autoconfig_irq(struct uart_8250_port *up)
 	if (port->flags & UPF_FOURPORT)
 		outb_p(save_ICP, ICP);
 
-	if (uart_console(port))
-		console_unlock();
-
 	port->irq = (irq > 0) ? irq : 0;
 }
 
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index d6a58a9e072a..ff85ebd3a007 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2608,7 +2608,12 @@  uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 			port->type = PORT_UNKNOWN;
 			flags |= UART_CONFIG_TYPE;
 		}
+		/* Synchronize with possible boot console. */
+		if (uart_console(port))
+			console_lock();
 		port->ops->config_port(port, flags);
+		if (uart_console(port))
+			console_unlock();
 	}
 
 	if (port->type != PORT_UNKNOWN) {
@@ -2616,6 +2621,10 @@  uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 
 		uart_report_port(drv, port);
 
+		/* Synchronize with possible boot console. */
+		if (uart_console(port))
+			console_lock();
+
 		/* Power up port for set_mctrl() */
 		uart_change_pm(state, UART_PM_STATE_ON);
 
@@ -2632,6 +2641,9 @@  uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 
 		uart_rs485_config(port);
 
+		if (uart_console(port))
+			console_unlock();
+
 		/*
 		 * If this driver supports console, and it hasn't been
 		 * successfully registered yet, try to re-register it.
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index f2444b581e16..f51e4e5a869d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3263,6 +3263,21 @@  static int __init keep_bootcon_setup(char *str)
 
 early_param("keep_bootcon", keep_bootcon_setup);
 
+static int console_call_setup(struct console *newcon, char *options)
+{
+	int err;
+
+	if (!newcon->setup)
+		return 0;
+
+	/* Synchronize with possible boot console. */
+	console_lock();
+	err = newcon->setup(newcon, options);
+	console_unlock();
+
+	return err;
+}
+
 /*
  * This is called by register_console() to try to match
  * the newly registered console with any of the ones selected
@@ -3298,8 +3313,8 @@  static int try_enable_preferred_console(struct console *newcon,
 			if (_braille_register_console(newcon, c))
 				return 0;
 
-			if (newcon->setup &&
-			    (err = newcon->setup(newcon, c->options)) != 0)
+			err = console_call_setup(newcon, c->options);
+			if (err != 0)
 				return err;
 		}
 		newcon->flags |= CON_ENABLED;
@@ -3325,7 +3340,7 @@  static void try_enable_default_console(struct console *newcon)
 	if (newcon->index < 0)
 		newcon->index = 0;
 
-	if (newcon->setup && newcon->setup(newcon, NULL) != 0)
+	if (console_call_setup(newcon, NULL) != 0)
 		return;
 
 	newcon->flags |= CON_ENABLED;