Messages in this thread | | | Date | Fri, 5 Apr 2024 07:25:03 +0200 | Subject | Re: [PATCH v1 1/1] serial: core: Clearing the circular buffer before NULLifying it | From | Jiri Slaby <> |
| |
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
| |