On Wed 2023-01-11 17:23:55, Petr Mladek wrote:
> On Wed 2023-01-11 15:57:10, John Ogness wrote:
> > Several mutexes are taken while setting up console serial ports. In
> > particular, the tty_port->mutex and @console_mutex are taken:
> >
> > serial_pnp_probe
> > serial8250_register_8250_port
> > uart_add_one_port (locks tty_port->mutex)
> > uart_configure_port
> > register_console (locks @console_mutex)
> >
> > In order to synchronize kgdb's tty_find_polling_driver() with
> > register_console(), commit 6193bc90849a ("tty: serial: kgdboc:
> > synchronize tty_find_polling_driver() and register_console()") takes
> > the @console_mutex. However, this leads to the following call chain
> > (with locking):
> >
> > platform_probe
> > kgdboc_probe
> > configure_kgdboc (locks @console_mutex)
> > tty_find_polling_driver
> > uart_poll_init (locks tty_port->mutex)
> > uart_set_options
> >
> > This is clearly deadlock potential due to the reverse lock ordering.
>
> Great catch!
>
> > Since uart_set_options() requires holding @console_mutex in order to
> > serialize early initialization of the serial-console lock, take the
> > @console_mutex in uart_poll_init() instead of configure_kgdboc().
> >
> > Since configure_kgdboc() was using @console_mutex for safe traversal
> > of the console list, change it to use the SRCU iterator instead.
> >
> > Add comments to uart_set_options() kerneldoc mentioning that it
> > requires holding @console_mutex (aka the console_list_lock).
> >
> > Fixes: 6193bc90849a ("tty: serial: kgdboc: synchronize tty_find_polling_driver() and register_console()")
> > Signed-off-by: John Ogness <john.ogness@xxxxxxxxxxxxx>
>
> > --- a/drivers/tty/serial/kgdboc.c
> > +++ b/drivers/tty/serial/kgdboc.c
> > @@ -189,15 +190,6 @@ static int configure_kgdboc(void)
> > if (kgdboc_register_kbd(&cptr))
> > goto do_register;
> >
> > - /*
> > - * tty_find_polling_driver() can call uart_set_options()
> > - * (via poll_init) to configure the uart. Take the console_list_lock
> > - * in order to synchronize against register_console(), which can also
> > - * configure the uart via uart_set_options(). This also allows safe
> > - * traversal of the console list.
> > - */
> > - console_list_lock();
> > -
> > p = tty_find_polling_driver(cptr, &tty_line);
> > if (!p) {
> > console_list_unlock();
>
> This console_list_unlock() should be removed here as well.
This seems to be the only problem. Otherwise, the patch looks good to
me.
After removing the superfluous console_list_unlock():
Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>
John, would you mind to send v2, please?
Best Regards,
Petr
