Received-SPF: pass (google.com: domain of linux-kernel+bounces-133689-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sat, 6 Apr 2024 01:37:39 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Jiri Slaby <jirislaby@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	Yicong Yang <yangyicong@hisilicon.com>,
	Tony Lindgren <tony@atomide.com>,
	kernel test robot <oliver.sang@intel.com>
Subject: Re: [PATCH v1 1/1] serial: core: Clearing the circular buffer before
 NULLifying it
Message-ID: <ZhB9M8C9IhXtJIXR@smile.fi.intel.com>
References: <20240404150034.41648-1-andriy.shevchenko@linux.intel.com>
 <f0f200b0-34dc-430b-b55e-b133faf4db44@kernel.org>
 <ZhAWIThfejjbmj8u@smile.fi.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZhAWIThfejjbmj8u@smile.fi.intel.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

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