Re: possible deadlock in console_unlock

[Petr Mladek]

On Thu 2018-06-07 14:10:19, Sergey Senozhatsky wrote:
> Cc-ing more people
> Link: lkml.kernel.org/r/00000000000087008b056df8fbb3@xxxxxxxxxx
> 
> On (06/06/18 06:17), syzbot wrote:
> > Hello,
> > 
> > syzbot found the following crash on:
> > 
> > HEAD commit:    af6c5d5e01ad Merge branch 'for-4.18' of git://git.kernel.o..
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=173d93ef800000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=12ff770540994680
> > dashboard link: https://syzkaller.appspot.com/bug?extid=43e93968b964e369db0b
> > compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
> > userspace arch: i386
> > syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=16e00bb7800000
> > 
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+43e93968b964e369db0b@xxxxxxxxxxxxxxxxxxxxxxxxx
> 
> IOW
> 
>     tty ioctl
>     tty_port->lock		IRQ
>     printk			uart_port->lock
>     console_owner
>     uart_port->lock		tty_port->rlock

Great analyze!


> The simplest thing to do [not necessarily the right one, tho]
> would be to break the IOCTL ==> tty_port->lock ==> printk ==> uart_port->lock
> chain.
> 
> E.g. by adding __GFP_NOWARN
> 
> ---
> 
>  drivers/tty/tty_buffer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index c996b6859c5e..71958ef6a831 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -167,7 +167,8 @@ static struct tty_buffer *tty_buffer_alloc(struct tty_port *port, size_t size)
>  	   have queued and recycle that ? */
>  	if (atomic_read(&port->buf.mem_used) > port->buf.mem_limit)
>  		return NULL;
> -	p = kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC);
> +	p = kmalloc(sizeof(struct tty_buffer) + 2 * size,
> +			GFP_ATOMIC | __GFP_NOWARN);
>  	if (p == NULL)
>  		return NULL;
> 
> ---

This looks like the most simple solution for this particular problem.
I am just afraid that there are many other locations like this.


> 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).

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

Best Regards,
Petr