Received-SPF: pass (google.com: domain of linux-kernel+bounces-135621-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 8 Apr 2024 18:45:06 +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: <ZhQRAhsGfdikDGEG@smile.fi.intel.com>
References: <20240404150034.41648-1-andriy.shevchenko@linux.intel.com>
 <f0f200b0-34dc-430b-b55e-b133faf4db44@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f0f200b0-34dc-430b-b55e-b133faf4db44@kernel.org>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

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.

So, with PM autosuspend the [PM] callback can be called in to cases:
- port is open and busy, but PM is not informed (yet) of it
- port is closed while PM timer is still counting

The Fixes seems about the first case (so we need to call Tx there).
The second one probably can be fixed properly with PM barrier.

This fix is just against the oops AFAIU the bug report and my own case.

-- 
With Best Regards,
Andy Shevchenko