Re: possible deadlock in console_unlock

Re: possible deadlock in console_unlock

[Sergey Senozhatsky]

Cc-ing more people
Link: lkml.kernel.org/r/00000000000087008b056df8fbb3@google.com

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@syzkaller.appspotmail.com

Thanks a ton!

So tty ioctl is known to be printk-deadlock prone and we don't know
how to handle this in printk, as of now.

ioctl
 tty_ioctl
  tty_port->lock
   printk
    call_console_driver
     console_driver
      uart_port->lock

The problem is that call_console_driver->console_driver also can do
this thing

   uart_port->lock
    tty_wakeup
     tty_port->lock

So we can have the following:

ioctl
 tty_ioctl
  tty_port->lock
   printk
    call_console_driver
     console_driver
      uart_port->lock
       tty_wakeup
        tty_port->lock      << deadlock


But lockdep is complaining about another scenario:

> the existing dependency chain (in reverse order) is:
> 
> -> #2 (&(&port->lock)->rlock){-.-.}:
>        __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
>        _raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
>        tty_port_tty_get+0x20/0x80 drivers/tty/tty_port.c:288
>        tty_port_default_wakeup+0x15/0x40 drivers/tty/tty_port.c:47
>        tty_port_tty_wakeup+0x5d/0x70 drivers/tty/tty_port.c:390
>        uart_write_wakeup+0x44/0x60 drivers/tty/serial/serial_core.c:103
>        serial8250_tx_chars+0x4be/0xb60
> drivers/tty/serial/8250/8250_port.c:1808
>        serial8250_handle_irq.part.25+0x1ee/0x280
> drivers/tty/serial/8250/8250_port.c:1881
>        serial8250_handle_irq drivers/tty/serial/8250/8250_port.c:1867
> [inline]
>        serial8250_default_handle_irq+0xc8/0x150
> drivers/tty/serial/8250/8250_port.c:1897
>        serial8250_interrupt+0xfa/0x1d0
> drivers/tty/serial/8250/8250_core.c:125
>        __handle_irq_event_percpu+0x1c0/0xad0 kernel/irq/handle.c:149
>        handle_irq_event_percpu+0x98/0x1c0 kernel/irq/handle.c:189
>        handle_irq_event+0xa7/0x135 kernel/irq/handle.c:206
>        handle_edge_irq+0x20f/0x870 kernel/irq/chip.c:791
>        generic_handle_irq_desc include/linux/irqdesc.h:159 [inline]
>        handle_irq+0x18c/0x2e7 arch/x86/kernel/irq_64.c:77
>        do_IRQ+0x78/0x190 arch/x86/kernel/irq.c:245
>        ret_from_intr+0x0/0x1e
>        native_safe_halt+0x6/0x10 arch/x86/include/asm/irqflags.h:54
>        arch_safe_halt arch/x86/include/asm/paravirt.h:94 [inline]
>        default_idle+0xc2/0x440 arch/x86/kernel/process.c:500
>        arch_cpu_idle+0x10/0x20 arch/x86/kernel/process.c:491
>        default_idle_call+0x6d/0x90 kernel/sched/idle.c:93
>        cpuidle_idle_call kernel/sched/idle.c:153 [inline]
>        do_idle+0x395/0x560 kernel/sched/idle.c:262
>        cpu_startup_entry+0x104/0x120 kernel/sched/idle.c:368
>        start_secondary+0x42b/0x5c0 arch/x86/kernel/smpboot.c:265
>        secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:242

So this one is  IRQ ==> uart_port->lock ==>  tty_port->lock

> -> #1 (&port_lock_key){-.-.}:
>        __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
>        _raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
>        serial8250_console_write+0x8d5/0xb00
> drivers/tty/serial/8250/8250_port.c:3230
>        univ8250_console_write+0x5f/0x70
> drivers/tty/serial/8250/8250_core.c:590
>        call_console_drivers kernel/printk/printk.c:1718 [inline]
>        console_unlock+0xac2/0x1100 kernel/printk/printk.c:2395
>        vprintk_emit+0x6ad/0xdd0 kernel/printk/printk.c:1907
>        vprintk_default+0x28/0x30 kernel/printk/printk.c:1947
>        vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:379
>        printk+0x9e/0xba kernel/printk/printk.c:1980
>        register_console+0x7e7/0xc00 kernel/printk/printk.c:2714
>        univ8250_console_init+0x3f/0x4b
> drivers/tty/serial/8250/8250_core.c:685
>        console_init+0x6d9/0xa38 kernel/printk/printk.c:2798
>        start_kernel+0x608/0x92d init/main.c:661
>        x86_64_start_reservations+0x29/0x2b arch/x86/kernel/head64.c:452
>        x86_64_start_kernel+0x76/0x79 arch/x86/kernel/head64.c:433
>        secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:242

This one is console_owner/console_sem ==> uart_port->lock


> -> #0 (console_owner){-...}:
>        lock_acquire+0x1dc/0x520 kernel/locking/lockdep.c:3924
>        console_lock_spinning_enable kernel/printk/printk.c:1581 [inline]
>        console_unlock+0x5ef/0x1100 kernel/printk/printk.c:2392
>        vprintk_emit+0x6ad/0xdd0 kernel/printk/printk.c:1907
>        vprintk_default+0x28/0x30 kernel/printk/printk.c:1947
>        vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:379
>        printk+0x9e/0xba kernel/printk/printk.c:1980
>        fail_dump lib/fault-inject.c:44 [inline]
>        should_fail+0x97a/0xbcd lib/fault-inject.c:149
>        __should_failslab+0x124/0x180 mm/failslab.c:32
>        should_failslab+0x9/0x14 mm/slab_common.c:1522
>        slab_pre_alloc_hook mm/slab.h:423 [inline]
>        slab_alloc mm/slab.c:3378 [inline]
>        __do_kmalloc mm/slab.c:3716 [inline]
>        __kmalloc+0x63/0x760 mm/slab.c:3727
>        kmalloc include/linux/slab.h:517 [inline]
>        tty_buffer_alloc drivers/tty/tty_buffer.c:170 [inline]
>        __tty_buffer_request_room+0x2d2/0x7f0 drivers/tty/tty_buffer.c:268
>        tty_insert_flip_string_fixed_flag+0x8d/0x1f0
> drivers/tty/tty_buffer.c:313
>        tty_insert_flip_string include/linux/tty_flip.h:37 [inline]
>        pty_write+0x12c/0x1f0 drivers/tty/pty.c:121
>        tty_put_char+0x129/0x150 drivers/tty/tty_io.c:2865
>        __process_echoes+0x4d9/0x8d0 drivers/tty/n_tty.c:703
>        process_echoes+0xfc/0x170 drivers/tty/n_tty.c:781
>        n_tty_set_termios+0xb56/0xe80 drivers/tty/n_tty.c:1837
>        tty_set_termios+0x7a0/0xac0 drivers/tty/tty_ioctl.c:341
>        set_termios+0x41e/0x7d0 drivers/tty/tty_ioctl.c:414
>        tty_mode_ioctl+0x871/0xb50 drivers/tty/tty_ioctl.c:781
>        n_tty_ioctl_helper+0x54/0x3b0 drivers/tty/tty_ioctl.c:940
>        n_tty_ioctl+0x54/0x320 drivers/tty/n_tty.c:2441
>        tty_ioctl+0x5e1/0x1870 drivers/tty/tty_io.c:2655
>        vfs_ioctl fs/ioctl.c:46 [inline]
>        file_ioctl fs/ioctl.c:500 [inline]
>        do_vfs_ioctl+0x1cf/0x16f0 fs/ioctl.c:684
>        __do_compat_sys_ioctl fs/compat_ioctl.c:1483 [inline]
>        __se_compat_sys_ioctl fs/compat_ioctl.c:1407 [inline]
>        __ia32_compat_sys_ioctl+0x43e/0x640 fs/compat_ioctl.c:1407
>        do_syscall_32_irqs_on arch/x86/entry/common.c:323 [inline]
>        do_fast_syscall_32+0x345/0xf9b arch/x86/entry/common.c:394
>        entry_SYSENTER_compat+0x70/0x7f arch/x86/entry/entry_64_compat.S:139

This one is tty IOCTL ==> tty_port->lock ==> console_owner/console_sem ==> uart_port->lock

>  Possible unsafe locking scenario:
> 
>        CPU0                    CPU1
>        ----                    ----
>   lock(&(&port->lock)->rlock);
>                                lock(&port_lock_key);
>                                lock(&(&port->lock)->rlock);
>   lock(console_owner);


IOW

    tty ioctl
    tty_port->lock		IRQ
    printk			uart_port->lock
    console_owner
    uart_port->lock		tty_port->rlock


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;

---


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();

This will redirect all printk()-s from kmalloc() to a special per-CPU
buffer, which will be flushed later from a safe context (irq work).


Or, may be, we even can switch to printk_safe mode every time we grab
tty_port lock.

#define tty_port_lock_irqsave(l,f)				\
	do {							\
		spin_lock_irqsave((l), f);			\
		__printk_safe_enter();				\
	} while (0)

#define tty_port_unlock_irqrestore(l,f)				\
	do {							\
		__printk_safe_exit();				\
		spin_lock_irqrestore((l),f);			\
	} while (0)

This will require some "automatic" replacement of all port->lock
operation in drivers/tty/*.

Perhaps something like this should be done for uart_port->lock
as well. Because, technically, we can have the following

	IRQ
	uart_port->lock
	tty_wakeup
	printk
	 call_console_drivers
	  console_driver
	   uart_port->lock               << deadlock

Which is totally possible.
E.g. tty_port_default_wakeup()->tty_port_tty_get()->refcount_inc()->WARN_ONCE()

Any opinions?

	-ss

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@google.com
> 
> 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@syzkaller.appspotmail.com
> 
> 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

Re: possible deadlock in console_unlock

[Tetsuo Handa]

On 2018/06/07 20:00, Petr Mladek wrote:
>> ---
>>
>>  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.
> 
I haven't tried the reproducer with that change. But isn't __GFP_NOWARN
ignored by fail_dump() (and thus printk() from fault injection still occurs)?

By the way, this reproducer is tricky. It needs to run like ./a.out
followed by "while :; do fg; done" because it always stops by a signal.

Re: possible deadlock in console_unlock

[Sergey Senozhatsky]

On (06/07/18 20:40), Tetsuo Handa wrote:
> >> 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.
> > 
> I haven't tried the reproducer with that change. But isn't __GFP_NOWARN
> ignored by fail_dump() (and thus printk() from fault injection still occurs)?

Thanks for the info. Need to check it [I didn't know that GFP_NOWARN
meant GFP_WARN_ME_SOMETIMES]. If this is the case then we have just one
option left - printk_safe contexts for TTY/UART locks.

	-ss

Re: possible deadlock in console_unlock

[Sergey Senozhatsky]

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

Re: possible deadlock in console_unlock

[Petr Mladek]

On Thu 2018-06-07 23:01:00, Sergey Senozhatsky wrote:
> On (06/07/18 13:00), Petr Mladek wrote:
> > > 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.

Same here.

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

We could allow nesting. It is just a matter of how many bits we
reserve for it in printk_context variable.

In each case, I would like to keep the printk_safe context usage
at minimum. It has its own problems caused by limited per-cpu buffers
and the need to flush them. It is basically needed only to prevent
deadlocks related to logbuf_lock.

Best Regards,
Petr

Re: possible deadlock in console_unlock

[Sergey Senozhatsky]

On (06/08/18 10:18), Petr Mladek wrote:
[..]
> > 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().
> 
> We could allow nesting. It is just a matter of how many bits we
> reserve for it in printk_context variable.
[..]
> In each case, I would like to keep the printk_safe context usage
> at minimum. It has its own problems caused by limited per-cpu buffers
> and the need to flush them.

May be. Every new printk_safe flavour comes with increasing memory
usage. We already have a bunch of pages pinned [and mostly unused]
to every CPU for printk_nmi and printk_safe. I'm a little unsure if
we have enough reasons to pin yet another bunch of pages to every
CPU. After all printk_safe is not used very commonly, so all in all
I think we should be fine with printk_safe buffers for the time being.
We always can introduce new printk_safe mode later.

> It is basically needed only to prevent deadlocks related to logbuf_lock.

I wouldn't say that we need printk_safe for logbuf_lock only.
printk_safe helps us to avoid deadlocks on:

- logbuf_lock spin_lock
- console_sem ->lock spin_lock
- console_owner spin_lock
- scheduler ->pi_lock spin_lock
- and probably something else.

	-ss

Re: possible deadlock in console_unlock

[Petr Mladek]

On Fri 2018-06-15 17:38:04, Sergey Senozhatsky wrote:
> On (06/08/18 10:18), Petr Mladek wrote:
> [..]
> > > 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().
> > 
> > We could allow nesting. It is just a matter of how many bits we
> > reserve for it in printk_context variable.
> [..]
> > In each case, I would like to keep the printk_safe context usage
> > at minimum. It has its own problems caused by limited per-cpu buffers
> > and the need to flush them.
> 
> May be. Every new printk_safe flavour comes with increasing memory
> usage.

This must be a misunderstanding. My intention was to introduce
printk_deferred() context. Where any printk() called in this
context would behave like printk_deferred(). It does not need
any extra buffers.

IMHO, this problem is similar to the problems that we solve
in scheduler and timer code. The cure might be the same.
I just suggest to introduce a context to make our life easier.


> > It is basically needed only to prevent deadlocks related to logbuf_lock.
> 
> I wouldn't say that we need printk_safe for logbuf_lock only.
> printk_safe helps us to avoid deadlocks on:
> 
> - logbuf_lock spin_lock

logbuf_lock is already guarded by printk_safe context everywhere.


> - console_sem ->lock spin_lock
> - console_owner spin_lock
> - scheduler ->pi_lock spin_lock
> - and probably something else.

printk_deferred should be enough for others. Or do I miss anything?

Best Regards,
Petr

Re: possible deadlock in console_unlock

[Sergey Senozhatsky]

On (06/19/18 10:04), Petr Mladek wrote:
> > > 
> > > We could allow nesting. It is just a matter of how many bits we
> > > reserve for it in printk_context variable.
> > [..]
> > > In each case, I would like to keep the printk_safe context usage
> > > at minimum. It has its own problems caused by limited per-cpu buffers
> > > and the need to flush them.
> > 
> > May be. Every new printk_safe flavour comes with increasing memory
> > usage.
> 
> This must be a misunderstanding. My intention was to introduce
> printk_deferred() context. Where any printk() called in this
> context would behave like printk_deferred(). It does not need
> any extra buffers.

Ah, got it. Yes, this can work.

	-ss

Re: possible deadlock in console_unlock

[Tetsuo Handa]

FTR, this topic seems to be moved to
https://lkml.kernel.org/r/ebc01f4f-5b1d-4f8a-1d0d-463d5218ee45@huawei.com .

Re: possible deadlock in console_unlock

[syzbot]

syzbot has bisected this bug to:

commit b6da31b2c07c46f2dcad1d86caa835227a16d9ff
Author: DaeRyong Jeong <threeearcat@gmail.com>
Date:   Mon Apr 30 15:27:04 2018 +0000

     tty: Fix data race in tty_insert_flip_string_fixed_flag

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=12d9bc0ee00000
start commit:   0ad39cb3 Merge tag 'kconfig-v4.18' of git://git.kernel.org..
git tree:       upstream
final crash:    https://syzkaller.appspot.com/x/report.txt?x=11d9bc0ee00000
console output: https://syzkaller.appspot.com/x/log.txt?x=16d9bc0ee00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=b9a1f3aa8b8ddd16
dashboard link: https://syzkaller.appspot.com/bug?extid=43e93968b964e369db0b
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=14c89b9f800000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=167f596f800000

Reported-by: syzbot+43e93968b964e369db0b@syzkaller.appspotmail.com
Fixes: b6da31b2c07c ("tty: Fix data race in  
tty_insert_flip_string_fixed_flag")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection