Re: [PATCH v2 5/6] serial: 8250_omap: set priv->line before enabling the IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] serial: 8250_omap: data race on priv->line assignment
--

commit 5147cae847c38d599a30c92bc983c41140a1abad
Author: Jiri Slaby (SUSE) <jirislaby@kernel.org>

serial: 8250_omap: set priv->line before enabling the IRQ

This commit fixes an issue where the IRQ handler could dereference an unset
priv->line if an interrupt occurs before omap8250_probe() stores it. It
stores port->line into priv->line in omap_8250_startup() before enabling
the IRQ to ensure the handler always sees a valid line number.

Cc: stable@vger.kernel.org

> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 3a1473bf54d0f..137175aa4183a 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -711,6 +711,9 @@ static int omap_8250_startup(struct uart_port *port)
>  	struct uart_8250_dma *dma = &priv->omap8250_dma;
>  	int ret;
>  
> +	/* ISR and runtime PM need the line; probe sets it only after registration */
> +	priv->line = port->line;
> +

[Severity: Low]
Does this new assignment introduce a data race regression on the unannotated
priv->line variable?

In omap8250_probe(), serial8250_register_8250_port() is called, which
exposes the TTY device to userspace. A concurrent userspace open() can then
immediately trigger omap_8250_startup() on another CPU, which executes this
new assignment:

    priv->line = port->line;

Meanwhile, the original omap8250_probe() execution resumes and writes to the
same variable:

    priv->line = ret;

This can result in a write-write data race between omap_8250_startup() and
omap8250_probe(). Furthermore, since omap_8250_startup() also enables the IRQ,
omap8250_irq() can fire and perform an unannotated read:

    struct uart_8250_port *up = serial8250_get_port(priv->line);

While functionally benign since both paths write the same aligned value, this
creates unannotated read-write and write-write races that constitute
undefined behavior under C11/LKMM and will trigger KCSAN warnings.

Could WRITE_ONCE() and READ_ONCE() be used to safely handle these concurrent
accesses?

>  #ifdef CONFIG_PM
>  	up->capabilities |= UART_CAP_RPM;
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791873876.git.jirislaby@kernel.org?part=5