* [PATCH] tty/serial: do not free trasnmit buffer page under port lock
@ 2018-12-13 4:58 Sergey Senozhatsky
2018-12-13 9:17 ` Peter Zijlstra
2018-12-13 9:42 ` Petr Mladek
0 siblings, 2 replies; 3+ messages in thread
From: Sergey Senozhatsky @ 2018-12-13 4:58 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: linux-serial, linux-kernel, Sergey Senozhatsky, Andrew Morton,
Peter Zijlstra, Waiman Long, Dmitry Safonov, Steven Rostedt,
Petr Mladek
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@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Dmitry Safonov <dima@arista.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Petr Mladek <pmladek@suse.com>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] tty/serial: do not free trasnmit buffer page under port lock
2018-12-13 4:58 [PATCH] tty/serial: do not free trasnmit buffer page under port lock Sergey Senozhatsky
@ 2018-12-13 9:17 ` Peter Zijlstra
2018-12-13 9:42 ` Petr Mladek
1 sibling, 0 replies; 3+ messages in thread
From: Peter Zijlstra @ 2018-12-13 9:17 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Sergey Senozhatsky, Andrew Morton, Waiman Long, Dmitry Safonov,
Steven Rostedt, Petr Mladek
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@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Waiman Long <longman@redhat.com>
> Cc: Dmitry Safonov <dima@arista.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Petr Mladek <pmladek@suse.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] tty/serial: do not free trasnmit buffer page under port lock
2018-12-13 4:58 [PATCH] tty/serial: do not free trasnmit buffer page under port lock Sergey Senozhatsky
2018-12-13 9:17 ` Peter Zijlstra
@ 2018-12-13 9:42 ` Petr Mladek
1 sibling, 0 replies; 3+ messages in thread
From: Petr Mladek @ 2018-12-13 9:42 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Sergey Senozhatsky, Andrew Morton, Peter Zijlstra, Waiman Long,
Dmitry Safonov, Steven Rostedt
On Thu 2018-12-13 13:58:39, Sergey Senozhatsky wrote:
> LKP has hit yet another circular locking dependency between uart
> console drivers and debugobjects [1]:
>
> 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@gmail.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-12-13 9:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-13 4:58 [PATCH] tty/serial: do not free trasnmit buffer page under port lock Sergey Senozhatsky
2018-12-13 9:17 ` Peter Zijlstra
2018-12-13 9:42 ` Petr Mladek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.