Re: possible deadlock in console_unlock

From: Sergey Senozhatsky
Date: Thu Jun 07 2018 - 10:01:10 EST


On (06/07/18 13:00), Petr Mladek wrote:
> > IOW
> >
> > tty ioctl
> > tty_port->lock IRQ
> > printk uart_port->lock
> > console_owner
> > uart_port->lock tty_port->rlock
>
> Great analyze!

Thanks!

> I am just afraid that there are many other locations like this.

Yep, agree. That's why I suggested the printk_safe context for
most critically important locks.

> > Another way could be - switch to printk_safe mode around that
> > kmalloc():
> >
> > __printk_safe_enter();
> > kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC);
> > __printk_safe_exit();
> >
> > Or, may be, we even can switch to printk_safe mode every time we grab
> > tty_port lock.
>
> > Perhaps something like this should be done for uart_port->lock
> > as well. Because, technically, we can have the following
>
> Yeah, we would need this basically around any lock that can be taken
> from console write() callbacks. Well, this would be needed even
> around locks that might be in a chain with a lock used in these
> callbacks (as shown by this report).

Yep. So the plan for now is to wrap the tty_port->lock. Pretty much
an automatic conversion.

Then to convert [may be some for now on] uart_port->lock. Once again,
pretty much can be done a script.

Afterwards just sit down and be humbl^W^W wait for new reports. Then
move those newly discovered unsafe locks under printk_safe context.

Basically, the same macros as we use for logbuf lock in printk.c

A bit of a lazy approach. Can't think of anything better.

I think it's finally the time to start dealing with these
"external" locks, it's been a while.

> BTW: printk_safe context might be too strict. In fact,
> printk_deferred() would be enough. We might think about
> introducing also printk_deferred context.

Could be.
The good thing about printk_safe is that printk_safe sections can nest.
I suspect there might be locks/printk_safe sections nesting at some
point. In any case, switching to a new flavor of printk_safe will be
pretty easy - just replace printk_safe_enter() with printk_foo_enter()
and the same for printk_save_exit().

I'll wait for some time, to see what people will say.
I guess we also need to check if Linus is OK with the proposed solution.

-ss