[tty,v1,0/8] synchronize UART_IER access against console write

Hi,

In preparation for making the 8250 serial driver the first driver to
support the upcoming atomic consoles [0], its console write()
callback (serial8250_console_write) was evaluated. For this callback
of the 8250 driver there are two critical writes to the UART_IER
register: once to disable all interrupts before transmitting a line
of text, and again after transmit to re-enable the previously enabled
interrupts. These two writes are performed under a single
synchronized section protected by the port lock.

I then checked all other access to UART_IER in the 8250 driver to see
if they always occurred under the port lock. If not, it would be
possible that the console write() callback could overwrite or restore
incorrect values to UART_IER. This is illustrated in the commit
message of the first patch.

Indeed several call sites were discovered where UART_IER is accessed
without the port lock. This series adds the missing locking in order
to ensure UART_IER access is always synchronized against the console
write() callback.

For call sites where UART_IER access was already performed under the
port lock, this series adds code comments and (when appropriate)
lockdep notation to help catch any future issues that may creep in.

Note that some of the new usage of port lock is not strictly
necessary, because (for example) the console is disabled before it
is suspended. However, these are not hot paths and by taking the port
lock it simplifies the synchronization semantics for UART_IER to
allow general lockdep usage.

Also note that none of these patches have been tagged for stable. The
possible stable candidates do include Fixes tags. But since the fixes
are not based on real-world reports, it probably is not necessary to
backport them.

John Ogness

[0] https://lore.kernel.org/lkml/20230302195618.156940-1-john.ogness@linutronix.de

John Ogness (8):
  serial: 8250: lock port in startup() callbacks
  serial: core: lock port for stop_rx() in uart_suspend_port()
  serial: 8250: lock port for stop_rx() in omap8250_irq()
  serial: core: lock port for start_rx() in uart_resume_port()
  serial: 8250: lock port for rx_dma() callback
  serial: 8250: lock port for omap8250_restore_regs()
  serial: 8250: lock port for UART_IER access in omap8250_irq()
  serial: 8250: synchronize and annotate UART_IER access

 drivers/tty/serial/8250/8250.h              |  6 ++
 drivers/tty/serial/8250/8250_aspeed_vuart.c |  3 +
 drivers/tty/serial/8250/8250_bcm7271.c      |  4 ++
 drivers/tty/serial/8250/8250_exar.c         |  4 ++
 drivers/tty/serial/8250/8250_mtk.c          |  9 +++
 drivers/tty/serial/8250/8250_omap.c         | 41 +++++++++++-
 drivers/tty/serial/8250/8250_port.c         | 71 ++++++++++++++++++++-
 drivers/tty/serial/serial_core.c            | 10 ++-
 8 files changed, 141 insertions(+), 7 deletions(-)


base-commit: d5b3d02d0b107345f2a6ecb5b06f98356f5c97ab

* John Ogness <john.ogness@linutronix.de> [230525 09:34]:
> omap8250_irq() accesses UART_IER. This register is modified twice
> by each console write (serial8250_console_write()) under the port
> lock. omap8250_irq() must also take the port lock to guanentee
> synchronized access to UART_IER.
> 
> Since the port lock is already being taken for the stop_rx() callback
> and since it is safe to call cancel_delayed_work() while holding the
> port lock, simply extend the port lock region to include UART_IER
> access.
> 
> Fixes: 1fe0e1fa3209 ("serial: 8250_omap: Handle optional overrun-throttle-ms property")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Tony Lindgren <tony@atomide.com>

> ---
>  drivers/tty/serial/8250/8250_omap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 34939462fd69..3225c95fde1d 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -659,17 +659,18 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
>  	if ((lsr & UART_LSR_OE) && up->overrun_backoff_time_ms > 0) {
>  		unsigned long delay;
>  
> +		/* Synchronize UART_IER access against the console. */
> +		spin_lock(&port->lock);
>  		up->ier = port->serial_in(port, UART_IER);
>  		if (up->ier & (UART_IER_RLSI | UART_IER_RDI)) {
> -			spin_lock(&port->lock);
>  			port->ops->stop_rx(port);
> -			spin_unlock(&port->lock);
>  		} else {
>  			/* Keep restarting the timer until
>  			 * the input overrun subsides.
>  			 */
>  			cancel_delayed_work(&up->overrun_backoff);
>  		}
> +		spin_unlock(&port->lock);
>  
>  		delay = msecs_to_jiffies(up->overrun_backoff_time_ms);
>  		schedule_delayed_work(&up->overrun_backoff, delay);
> -- 
> 2.30.2
>