[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20181213091733.GD5289@hirez.programming.kicks-ass.net>
Date: Thu, 13 Dec 2018 10:17:33 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jslaby@...e.com>, linux-serial@...r.kernel.org,
linux-kernel@...r.kernel.org,
Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Waiman Long <longman@...hat.com>,
Dmitry Safonov <dima@...sta.com>,
Steven Rostedt <rostedt@...dmis.org>,
Petr Mladek <pmladek@...e.com>
Subject: Re: [PATCH] tty/serial: do not free trasnmit buffer page under port
lock
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 <sergey.senozhatsky@...il.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Jiri Slaby <jslaby@...e.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Waiman Long <longman@...hat.com>
> Cc: Dmitry Safonov <dima@...sta.com>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Petr Mladek <pmladek@...e.com>
Acked-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
> 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
>
Powered by blists - more mailing lists