Re: [PATCH] tty/serial: do not free trasnmit buffer page under port lock

[Peter Zijlstra]

On Thu, Dec 13, 2018 at 01:58:39PM +0900, Sergey Senozhatsky wrote:
> LKP has hit yet another circular locking dependency between uart
> console drivers and debugobjects [1]:
> 
>      CPU0                                    CPU1
> 
>                                             rhltable_init()
>                                              __init_work()
>                                               debug_object_init
>      uart_shutdown()                          /* db->lock */
>       /* uart_port->lock */                    debug_print_object()
>        free_page()                              printk()
>                                                  call_console_drivers()
>         debug_check_no_obj_freed()                /* uart_port->lock */
>          /* db->lock */
>           debug_print_object()
> 
> So there are two dependency chains:
> 	uart_port->lock -> db->lock
> And
> 	db->lock -> uart_port->lock
> 
> This particular circular locking dependency can be addressed in several
> ways:
> 
> a) One way would be to move debug_print_object() out of db->lock scope
>    and, thus, break the db->lock -> uart_port->lock chain.
> b) Another one would be to free() transmit buffer page out of db->lock
>    in UART code; which is what this patch does.
> 
> It makes sense to apply a) and b) independently: there are too many things
> going on behind free(), none of which depend on uart_port->lock.
> 
> The patch fixes transmit buffer page free() in uart_shutdown() and,
> additionally, in uart_port_startup() (as was suggested by Dmitry Safonov).
> 
> [1] 20181211091154.GL23332@shao2-debian/T/#u">https://lore.kernel.org/lkml/20181211091154.GL23332@shao2-debian/T/#u
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Jiri Slaby <jslaby@xxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Waiman Long <longman@xxxxxxxxxx>
> Cc: Dmitry Safonov <dima@xxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Petr Mladek <pmladek@xxxxxxxx>

Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>

> ---
>  drivers/tty/serial/serial_core.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c439a5a1e6c0..d4cca5bdaf1c 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -205,10 +205,15 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
>  	if (!state->xmit.buf) {
>  		state->xmit.buf = (unsigned char *) page;
>  		uart_circ_clear(&state->xmit);
> +		uart_port_unlock(uport, flags);
>  	} else {
> +		uart_port_unlock(uport, flags);
> +		/*
> +		 * Do not free() the page under the port lock, see
> +		 * uart_shutdown().
> +		 */
>  		free_page(page);
>  	}
> -	uart_port_unlock(uport, flags);
>  
>  	retval = uport->ops->startup(uport);
>  	if (retval == 0) {
> @@ -268,6 +273,7 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
>  	struct uart_port *uport = uart_port_check(state);
>  	struct tty_port *port = &state->port;
>  	unsigned long flags = 0;
> +	char *xmit_buf = NULL;
>  
>  	/*
>  	 * Set the TTY IO error marker
> @@ -298,14 +304,18 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
>  	tty_port_set_suspended(port, 0);
>  
>  	/*
> -	 * Free the transmit buffer page.
> +	 * Do not free() the transmit buffer page under the port lock since
> +	 * this can create various circular locking scenarios. For instance,
> +	 * console driver may need to allocate/free a debug object, which
> +	 * can endup in printk() recursion.
>  	 */
>  	uart_port_lock(state, flags);
> -	if (state->xmit.buf) {
> -		free_page((unsigned long)state->xmit.buf);
> -		state->xmit.buf = NULL;
> -	}
> +	xmit_buf = state->xmit.buf;
> +	state->xmit.buf = NULL;
>  	uart_port_unlock(uport, flags);
> +
> +	if (xmit_buf)
> +		free_page((unsigned long)xmit_buf);
>  }
>  
>  /**
> -- 
> 2.20.0
>