Received-SPF: pass (google.com: domain of linux-kernel+bounces-134311-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
CC: <yangyicong@hisilicon.com>, Jiri Slaby <jirislaby@kernel.org>, 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
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Greg Kroah-Hartman
	<gregkh@linuxfoundation.org>, <linux-kernel@vger.kernel.org>,
	<linux-serial@vger.kernel.org>
References: <20240404150034.41648-1-andriy.shevchenko@linux.intel.com>
From: Yicong Yang <yangyicong@huawei.com>
Message-ID: <b3fd1077-b49e-d99b-9cd1-c41bd244f290@huawei.com>
Date: Sun, 7 Apr 2024 17:49:19 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.5.1
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240404150034.41648-1-andriy.shevchenko@linux.intel.com>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit

Hi Andy,

On 2024/4/4 22: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)
> 
> The proposed change will prevent ->start_tx() to be called during
> suspend on shut down port.
> 

Just saw the issue and thanks for your timely fix. I didn't got a board with 8250 and sorry for
didn't found this issue.

FYI, I checked device_shutdown() and seems it called pm_runtime_barrier() for waiting all
the scheduled RPM callbacks finished and keep the device in resume state. So ideally there
shouldn't be any pending requests later since we handled them before shutdown?

There's someone encountered the same issue in shutdown() due to runtime pm and fixed it in
	af8db1508f2c ("PM / driver core: disable device's runtime PM during shutdown")
patch above seems to still have some problem and later fixed by:
	fe6b91f47080 ("PM / Driver core: leave runtime PM enabled during system shutdown")

But seems the handling in the driver core doesn't cover the case here..

> 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);
>