On Fri, Apr 05, 2024 at 06:17:54PM +0300, Andy Shevchenko wrote: > On Fri, Apr 05, 2024 at 07:25:03AM +0200, Jiri Slaby wrote: > > 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(). > > Yes, and I have no clue how we can check this as startup can be called again > and so on. The PM callback is timer based AFAIU, meaning it may happen at any > time. > > But do you agree that this patch has value on its own? FWIW, https://lore.kernel.org/all/0000000000009e2dd805ffc595a3@xxxxxxxxxx/T/ -- With Best Regards, Andy Shevchenko
