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@xxxxxxxxx>
Closes: https://lore.kernel.org/oe-lkp/202404031607.2e92eebe-lkp@xxxxxxxxx
Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
---
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
