linux-kernel - Re: UART/TTY console deadlock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 30 Jun 2020 14:22:39 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Raul Rangel <rrangel@...gle.com>,
        Tony Lindgren <tony@...mide.com>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        kurt@...utronix.de, "S, Shirish" <Shirish.S@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        John Ogness <john.ogness@...utronix.de>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: UART/TTY console deadlock

On Tue 2020-06-30 19:55:12, Sergey Senozhatsky wrote:
> On (20/06/30 12:21), Petr Mladek wrote:
> > Anyway, I believe that this is a false positive. If I get it correctly
> > serial8250_do_startup() must be called before the serial port could
> > be registered as a console. It means that it could not be called
> > from inside printk().
> 
> >From my understanding, I'm afraid we are talking about actual deadlock
> here, not about false positive report. Quoting the original email:

IMHO, it is not clear. The log contains lockdep report. It does not mean
the deadlock really happened. The lockdep reports cyclic dependency but it
is not able to say if the related code chains might be called at the
same time in reality.

And this combination has signs of false positives. As I said, I think
that the serial console could not be registered by printk() before
serial8250_do_startup() has finished.

Also there are some unrelated messages after the last backtrace.

Finally. we would be able to see the messages only when panic() was
called and oops_in_progress set. And there is nothing about panic().


>  : We are trying an S3 suspend stress test and occasionally while
>  : entering S3 we get a console deadlock.

Raul, have the system really died or have you just seen the lockdep
report, please?


> [..]
> 
> > >  drivers/tty/serial/8250/8250_port.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > > index d64ca77d9cfa..ad30991e1b3b 100644
> > > --- a/drivers/tty/serial/8250/8250_port.c
> > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > @@ -2275,6 +2275,11 @@ int serial8250_do_startup(struct uart_port *port)
> > >  
> > >  	if (port->irq && !(up->port.flags & UPF_NO_THRE_TEST)) {
> > >  		unsigned char iir1;
> > > +		bool irq_shared = up->port.irqflags & IRQF_SHARED;
> > > +
> > > +		if (irq_shared)
> > > +			disable_irq_nosync(port->irq);
> > > +
> > >  		/*
> > >  		 * Test for UARTs that do not reassert THRE when the
> > >  		 * transmitter is idle and the interrupt has already
> > > @@ -2284,8 +2289,6 @@ int serial8250_do_startup(struct uart_port *port)
> > >  		 * allow register changes to become visible.
> > >  		 */
> > >  		spin_lock_irqsave(&port->lock, flags);
> > > -		if (up->port.irqflags & IRQF_SHARED)
> > > -			disable_irq_nosync(port->irq);
> > >  
> > >  		wait_for_xmitr(up, UART_LSR_THRE);
> > >  		serial_port_out_sync(port, UART_IER, UART_IER_THRI);
> > > @@ -2297,9 +2300,9 @@ int serial8250_do_startup(struct uart_port *port)
> > >  		iir = serial_port_in(port, UART_IIR);
> > >  		serial_port_out(port, UART_IER, 0);
> > >  
> > > -		if (port->irqflags & IRQF_SHARED)
> > > -			enable_irq(port->irq);
> > >  		spin_unlock_irqrestore(&port->lock, flags);
> > > +		if (irq_shared)
> > > +			enable_irq(port->irq);
> > >  
> > >  		/*
> > >  		 * If the interrupt is not reasserted, or we otherwise
> > 
> > I think that it might be safe but I am not 100% sure, sigh.
> 
> Yeah, I'm not 100%, but I'd give it a try.

I do not feel brave enough to ack it today. But I am all for trying it
if anyone more familiar with the code is fine with it.

Best Regards,
Petr