lkml.org 
[lkml]   [2024]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v1 1/1] serial: core: Clearing the circular buffer before NULLifying it
From
On 04. 04. 24, 16:59, Andy Shevchenko wrote:
> The circular buffer is NULLified in uart_tty_port_shutdown()
> under the spin lock. However, the PM or other timer based callbacks
> may still trigger after this event without knowning that buffer pointer
> is not valid. Since the serial code is a bit inconsistent in checking
> the buffer state (some rely on the head-tail positions, some on the
> buffer pointer), it's better to have both aligned, i.e. buffer pointer
> to be NULL and head-tail possitions to be the same, meaning it's empty.
> This will prevent asynchronous calls to dereference NULL pointer as
> reported recently in 8250 case:
>
> BUG: kernel NULL pointer dereference, address: 00000cf5
> Workqueue: pm pm_runtime_work
> EIP: serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
> ...
> ? serial8250_tx_chars (drivers/tty/serial/8250/8250_port.c:1809)
> __start_tx (drivers/tty/serial/8250/8250_port.c:1551)
> serial8250_start_tx (drivers/tty/serial/8250/8250_port.c:1654)
> serial_port_runtime_suspend (include/linux/serial_core.h:667 drivers/tty/serial/serial_port.c:63)
> __rpm_callback (drivers/base/power/runtime.c:393)
> ? serial_port_remove (drivers/tty/serial/serial_port.c:50)
> rpm_suspend (drivers/base/power/runtime.c:447)

Yeah, I noticed start_tx() is called repeatedly after shutdown()
yesterday too. So thanks for looking into this.

And it's pretty weird. I think it's new with the runtime PM (sure, /me
reads Fixes: now). I am not sure if it is documented, but most of the
code in tty/ assumes NO ordinary ->ops (like start_tx()) are called
after shutdown(). Actually, to me it occurs like serial8250_start_tx()
should not be called in the first place. It makes no sense after all.

BTW cannot be x_char en/queued at that time too (the other check in the
if)? But again, serial8250_start_tx() should not be called after shutdown().

> The proposed change will prevent ->start_tx() to be called during
> suspend on shut down port.
>
> Fixes: 43066e32227e ("serial: port: Don't suspend if the port is still busy")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202404031607.2e92eebe-lkp@intel.com
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>
> I have got into the very similar issue while working on max3100 driver.
> I haven't checked the 8250 case, but for mine the culprit is the same
> and this patch fixes it. Hence I assume it will fix the 8250 case as
> well.
>
> drivers/tty/serial/serial_core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index a005fc06a077..ba3a674a8bbf 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1788,6 +1788,7 @@ static void uart_tty_port_shutdown(struct tty_port *port)
> * Free the transmit buffer.
> */
> uart_port_lock_irq(uport);
> + uart_circ_clear(&state->xmit);
> buf = state->xmit.buf;
> state->xmit.buf = NULL;
> uart_port_unlock_irq(uport);

--
js
suse labs


\
 
 \ /
  Last update: 2024-05-27 16:24    [W:0.082 / U:0.832 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site