Re: [PATCH v1 1/1] serial: core: Clearing the circular buffer before NULLifying it

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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@google.com/T/

-- 
With Best Regards,
Andy Shevchenko