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] https://lore.kernel.org/lkml/20181211091154.GL23332@shao2-debian/T/#u > Signed-off-by: Sergey Senozhatsky <[email protected]> > Cc: Greg Kroah-Hartman <[email protected]> > Cc: Jiri Slaby <[email protected]> > Cc: Andrew Morton <[email protected]> > Cc: Peter Zijlstra <[email protected]> > Cc: Waiman Long <[email protected]> > Cc: Dmitry Safonov <[email protected]> > Cc: Steven Rostedt <[email protected]> > Cc: Petr Mladek <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]> > --- > 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 >
