Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
References: <20211108085431.12637-1-johan@kernel.org>
User-agent: mu4e 1.6.9; emacs 27.1
From:   Baruch Siach <baruch@tkos.co.il>
To:     Johan Hovold <johan@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org, Rob Herring <robh@kernel.org>
Subject: Re: [PATCH] serial: core: fix transmit-buffer reset and memleak
Date:   Mon, 08 Nov 2021 11:15:56 +0200
In-reply-to: <20211108085431.12637-1-johan@kernel.org>
Message-ID: <87mtmfuhtx.fsf@tarshish>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi Johan,

On Mon, Nov 08 2021, Johan Hovold wrote:
> Commit 761ed4a94582 ("tty: serial_core: convert uart_close to use
> tty_port_close") converted serial core to use tty_port_close() but
> failed to notice that the transmit buffer still needs to be freed on
> final close.
>
> Not freeing the transmit buffer means that the buffer is no longer
> cleared on next open so that any ioctl() waiting for the buffer to drain
> might wait indefinitely (e.g. on termios changes) or that stale data can
> end up being transmitted in case tx is restarted.
>
> Furthermore, the buffer of any port that has been opened would leak on
> driver unbind.
>
> Note that the port lock is held when clearing the buffer pointer due to
> the ldisc race worked around by commit a5ba1d95e46e ("uart: fix race
> between uart_put_char() and uart_shutdown()").
>
> Also note that the tty-port shutdown() callback is not called for
> console ports so it is not strictly necessary to free the buffer page
> after releasing the lock (cf. d72402145ace ("tty/serial: do not free
> trasnmit buffer page under port lock")).
>
> Reported-by: Baruch Siach <baruch@tkos.co.il>
> Link: https://lore.kernel.org/r/319321886d97c456203d5c6a576a5480d07c3478.1635781688.git.baruch@tkos.co.il
> Fixes: 761ed4a94582 ("tty: serial_core: convert uart_close to use tty_port_close")
> Cc: stable@vger.kernel.org      # 4.9
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Thanks for the analysis and root cause fix. This patch also fixes the
issue for me.

Tested-by: Baruch Siach <baruch@tkos.co.il>

baruch

> ---
>  drivers/tty/serial/serial_core.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 0e2e35ab64c7..58834698739c 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1542,6 +1542,7 @@ static void uart_tty_port_shutdown(struct tty_port *port)
>  {
>  	struct uart_state *state = container_of(port, struct uart_state, port);
>  	struct uart_port *uport = uart_port_check(state);
> +	char *buf;
>  
>  	/*
>  	 * At this point, we stop accepting input.  To do this, we
> @@ -1563,8 +1564,18 @@ static void uart_tty_port_shutdown(struct tty_port *port)
>  	 */
>  	tty_port_set_suspended(port, 0);
>  
> -	uart_change_pm(state, UART_PM_STATE_OFF);
> +	/*
> +	 * Free the transmit buffer.
> +	 */
> +	spin_lock_irq(&uport->lock);
> +	buf = state->xmit.buf;
> +	state->xmit.buf = NULL;
> +	spin_unlock_irq(&uport->lock);
>  
> +	if (buf)
> +		free_page((unsigned long)buf);
> +
> +	uart_change_pm(state, UART_PM_STATE_OFF);
>  }
>  
>  static void uart_wait_until_sent(struct tty_struct *tty, int timeout)


-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -