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@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/319321886d97c456203d5c6a576a5480d07c3478.1635781688.git.baruch@xxxxxxxxxx
> Fixes: 761ed4a94582 ("tty: serial_core: convert uart_close to use tty_port_close")
> Cc: stable@xxxxxxxxxxxxxxx # 4.9
> Cc: Rob Herring <robh@xxxxxxxxxx>
> Signed-off-by: Johan Hovold <johan@xxxxxxxxxx>
Thanks for the analysis and root cause fix. This patch also fixes the
issue for me.
Tested-by: Baruch Siach <baruch@xxxxxxxxxx>
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@xxxxxxxxxx - tel: +972.52.368.4656, http://www.tkos.co.il -
