Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1310265 > unrolled thread

Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: tty: deadlock between n_tracerouter_receivebuf and  flush_to_ldisc One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-01-15 17:40 +0100
    Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc Dmitry Vyukov <dvyukov@google.com> - 2016-01-15 18:30 +0100
      Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc Dmitry Vyukov <dvyukov@google.com> - 2016-01-20 10:40 +0100
        Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc Peter Zijlstra <peterz@infradead.org> - 2016-01-20 12:50 +0100
          Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc Dmitry Vyukov <dvyukov@google.com> - 2016-01-20 13:00 +0100
            Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc Peter Zijlstra <peterz@infradead.org> - 2016-01-20 13:10 +0100
              Re: tty: deadlock between n_tracerouter_receivebuf and  flush_to_ldisc One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-01-20 16:00 +0100
                Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc Dmitry Vyukov <dvyukov@google.com> - 2016-01-20 16:20 +0100
                  Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc Peter Zijlstra <peterz@infradead.org> - 2016-01-20 17:40 +0100
    Re: tty: deadlock between n_tracerouter_receivebuf and  flush_to_ldisc J Freyensee <james_p_freyensee@linux.intel.com> - 2016-01-20 03:10 +0100

#1310265 — Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc

On Fri, 15 Jan 2016 08:51:39 +0100
Dmitry Vyukov <dvyukov@google.com> wrote:

> On Wed, Dec 30, 2015 at 11:44 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> > Hello,
> >
> > I've got the following lock order inversion report on
> > 8513342170278468bac126640a5d2d12ffbff106 (Dec 28). Unfortunately I
> > failed to find a reproducer for it.
> 
> Ping. This is the most frequent failure during my testing now. J
> Freyensee, you seem to wrote drivers/tty/n_tracerouter.c. Please take
> a look. Thanks.

This driver hasn't changed since forever, nor does it look like a bug in
n_tracerouter.

This first two threads of execution is for the old line discipline so
don't occur at the same time as executing tracerouter.

This is opening the port with the tracerouter discipline as you change
line discipline. The core code locks the tty and then takes the ldisc
lock. Closes the old ldisc, opens the new, opening the new takes the
router lock.

	(tty_lock, tty_ldisc_lock, termios_sem, route_lock)


> > -> #1 (&port->buf.lock/1){+.+...}:
> >        [<ffffffff813f0acf>] lock_acquire+0x19f/0x3c0
> > kernel/locking/lockdep.c:3585
> >        [<     inline     >] __mutex_lock_common kernel/locking/mutex.c:518
> >        [<ffffffff85c86ba6>] mutex_lock_nested+0xb6/0x940
> > kernel/locking/mutex.c:618
> >        [<ffffffff82bc77ff>] n_tracerouter_open+0x1f/0x210
> > drivers/tty/n_tracerouter.c:75
> >        [<ffffffff82bacd78>] tty_ldisc_open.isra.2+0x78/0xd0
> > drivers/tty/tty_ldisc.c:447
> >        [<ffffffff82bad43a>] tty_set_ldisc+0x1ca/0xa30
> > drivers/tty/tty_ldisc.c:567
> >        [<     inline     >] tiocsetd drivers/tty/tty_io.c:2650
> >        [<ffffffff82b9483a>] tty_ioctl+0xb2a/0x2160 drivers/tty/tty_io.c:2883
> >        [<     inline     >] vfs_ioctl fs/ioctl.c:43
> >        [<ffffffff81750d41>] do_vfs_ioctl+0x681/0xe40 fs/ioctl.c:607
> >        [<     inline     >] SYSC_ioctl fs/ioctl.c:622
> >        [<ffffffff8175158f>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:613
> >        [<ffffffff85c8eb36>] entry_SYSCALL_64_fastpath+0x16/0x7a
> > arch/x86/entry/entry_64.S:185

This is data being received by the new line discipline.
	
	(buf->lock, routelock)

> >
> > -> #0 (routelock){+.+...}:
> >        [<     inline     >] check_prev_add kernel/locking/lockdep.c:1853
> >        [<     inline     >] check_prevs_add kernel/locking/lockdep.c:1958
> >        [<     inline     >] validate_chain kernel/locking/lockdep.c:2144
> >        [<ffffffff813ee02e>] __lock_acquire+0x345e/0x3ca0
> > kernel/locking/lockdep.c:3206
> >        [<ffffffff813f0acf>] lock_acquire+0x19f/0x3c0
> > kernel/locking/lockdep.c:3585
> >        [<     inline     >] __mutex_lock_common kernel/locking/mutex.c:518
> >        [<ffffffff85c86ba6>] mutex_lock_nested+0xb6/0x940
> > kernel/locking/mutex.c:618
> >        [<ffffffff82bc77c0>] n_tracerouter_receivebuf+0x20/0x40
> > drivers/tty/n_tracerouter.c:175
> >        [<     inline     >] receive_buf drivers/tty/tty_buffer.c:454
> >        [<ffffffff82bafc34>] flush_to_ldisc+0x584/0x7f0
> > drivers/tty/tty_buffer.c:517
> >        [<ffffffff8133833c>] process_one_work+0x76c/0x13e0
> > kernel/workqueue.c:2030
> >        [<ffffffff81339093>] worker_thread+0xe3/0xe90 kernel/workqueue.c:2162
> >        [<ffffffff8134b63f>] kthread+0x23f/0x2d0 drivers/block/aoe/aoecmd.c:1303
> >        [<ffffffff85c8eeef>] ret_from_fork+0x3f/0x70
> > arch/x86/entry/entry_64.S:468
> >

The lock inversion the log shows seems to be a problem in n_tty not in
the tracerouter code.

However that seems to be

tty_set_ldisc to N_TTY
	takes tty lock
	takes tty_ldisc lock
	down termios_rwsem (R)
	tty->ops->set_ldisc
	down termios rwsem (W)

verus

receive_driver_bufs
	buf->lock
	termios rwsem (R)


which also looks fine
	
So I think it's just a false report caused by other changes in the tty
layer, and nothing to do with the tracerouter code at all.

Alan

[toc] | [next] | [standalone]

#1310298 — Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc

On Fri, Jan 15, 2016 at 5:33 PM, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
> On Fri, 15 Jan 2016 08:51:39 +0100
> Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> On Wed, Dec 30, 2015 at 11:44 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> > Hello,
>> >
>> > I've got the following lock order inversion report on
>> > 8513342170278468bac126640a5d2d12ffbff106 (Dec 28). Unfortunately I
>> > failed to find a reproducer for it.
>>
>> Ping. This is the most frequent failure during my testing now. J
>> Freyensee, you seem to wrote drivers/tty/n_tracerouter.c. Please take
>> a look. Thanks.
>
> This driver hasn't changed since forever, nor does it look like a bug in
> n_tracerouter.
>
> This first two threads of execution is for the old line discipline so
> don't occur at the same time as executing tracerouter.
>
> This is opening the port with the tracerouter discipline as you change
> line discipline. The core code locks the tty and then takes the ldisc
> lock. Closes the old ldisc, opens the new, opening the new takes the
> router lock.
>
>         (tty_lock, tty_ldisc_lock, termios_sem, route_lock)
>
>
>> > -> #1 (&port->buf.lock/1){+.+...}:
>> >        [<ffffffff813f0acf>] lock_acquire+0x19f/0x3c0
>> > kernel/locking/lockdep.c:3585
>> >        [<     inline     >] __mutex_lock_common kernel/locking/mutex.c:518
>> >        [<ffffffff85c86ba6>] mutex_lock_nested+0xb6/0x940
>> > kernel/locking/mutex.c:618
>> >        [<ffffffff82bc77ff>] n_tracerouter_open+0x1f/0x210
>> > drivers/tty/n_tracerouter.c:75
>> >        [<ffffffff82bacd78>] tty_ldisc_open.isra.2+0x78/0xd0
>> > drivers/tty/tty_ldisc.c:447
>> >        [<ffffffff82bad43a>] tty_set_ldisc+0x1ca/0xa30
>> > drivers/tty/tty_ldisc.c:567
>> >        [<     inline     >] tiocsetd drivers/tty/tty_io.c:2650
>> >        [<ffffffff82b9483a>] tty_ioctl+0xb2a/0x2160 drivers/tty/tty_io.c:2883
>> >        [<     inline     >] vfs_ioctl fs/ioctl.c:43
>> >        [<ffffffff81750d41>] do_vfs_ioctl+0x681/0xe40 fs/ioctl.c:607
>> >        [<     inline     >] SYSC_ioctl fs/ioctl.c:622
>> >        [<ffffffff8175158f>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:613
>> >        [<ffffffff85c8eb36>] entry_SYSCALL_64_fastpath+0x16/0x7a
>> > arch/x86/entry/entry_64.S:185
>
> This is data being received by the new line discipline.
>
>         (buf->lock, routelock)
>
>> >
>> > -> #0 (routelock){+.+...}:
>> >        [<     inline     >] check_prev_add kernel/locking/lockdep.c:1853
>> >        [<     inline     >] check_prevs_add kernel/locking/lockdep.c:1958
>> >        [<     inline     >] validate_chain kernel/locking/lockdep.c:2144
>> >        [<ffffffff813ee02e>] __lock_acquire+0x345e/0x3ca0
>> > kernel/locking/lockdep.c:3206
>> >        [<ffffffff813f0acf>] lock_acquire+0x19f/0x3c0
>> > kernel/locking/lockdep.c:3585
>> >        [<     inline     >] __mutex_lock_common kernel/locking/mutex.c:518
>> >        [<ffffffff85c86ba6>] mutex_lock_nested+0xb6/0x940
>> > kernel/locking/mutex.c:618
>> >        [<ffffffff82bc77c0>] n_tracerouter_receivebuf+0x20/0x40
>> > drivers/tty/n_tracerouter.c:175
>> >        [<     inline     >] receive_buf drivers/tty/tty_buffer.c:454
>> >        [<ffffffff82bafc34>] flush_to_ldisc+0x584/0x7f0
>> > drivers/tty/tty_buffer.c:517
>> >        [<ffffffff8133833c>] process_one_work+0x76c/0x13e0
>> > kernel/workqueue.c:2030
>> >        [<ffffffff81339093>] worker_thread+0xe3/0xe90 kernel/workqueue.c:2162
>> >        [<ffffffff8134b63f>] kthread+0x23f/0x2d0 drivers/block/aoe/aoecmd.c:1303
>> >        [<ffffffff85c8eeef>] ret_from_fork+0x3f/0x70
>> > arch/x86/entry/entry_64.S:468
>> >
>
> The lock inversion the log shows seems to be a problem in n_tty not in
> the tracerouter code.
>
> However that seems to be
>
> tty_set_ldisc to N_TTY
>         takes tty lock
>         takes tty_ldisc lock
>         down termios_rwsem (R)
>         tty->ops->set_ldisc
>         down termios rwsem (W)
>
> verus
>
> receive_driver_bufs
>         buf->lock
>         termios rwsem (R)
>
>
> which also looks fine
>
> So I think it's just a false report caused by other changes in the tty
> layer, and nothing to do with the tracerouter code at all.


Thanks for looking into this.

The report says:

       CPU0                    CPU1
       ----                    ----
  lock(&buf->lock);
                               lock(&o_tty->termios_rwsem/1);
                               lock(&buf->lock);
  lock(routelock);

I don't see routelock in your stacks.
But I also I don't understand why this is a deadlock. I would
understand if one thread takes A then B; while another takes B then A.
But here are 3 non-circular lock acquisitions. Does it have something
to do with spinlocks vs mutex/rwsem?

[toc] | [prev] | [next] | [standalone]

#1313039 — Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc

On Fri, Jan 15, 2016 at 6:22 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Fri, Jan 15, 2016 at 5:33 PM, One Thousand Gnomes
> <gnomes@lxorguk.ukuu.org.uk> wrote:
>> On Fri, 15 Jan 2016 08:51:39 +0100
>> Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>>> On Wed, Dec 30, 2015 at 11:44 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>> > Hello,
>>> >
>>> > I've got the following lock order inversion report on
>>> > 8513342170278468bac126640a5d2d12ffbff106 (Dec 28). Unfortunately I
>>> > failed to find a reproducer for it.
>>>
>>> Ping. This is the most frequent failure during my testing now. J
>>> Freyensee, you seem to wrote drivers/tty/n_tracerouter.c. Please take
>>> a look. Thanks.
>>
>> This driver hasn't changed since forever, nor does it look like a bug in
>> n_tracerouter.
>>
>> This first two threads of execution is for the old line discipline so
>> don't occur at the same time as executing tracerouter.
>>
>> This is opening the port with the tracerouter discipline as you change
>> line discipline. The core code locks the tty and then takes the ldisc
>> lock. Closes the old ldisc, opens the new, opening the new takes the
>> router lock.
>>
>>         (tty_lock, tty_ldisc_lock, termios_sem, route_lock)
>>
>>
>>> > -> #1 (&port->buf.lock/1){+.+...}:
>>> >        [<ffffffff813f0acf>] lock_acquire+0x19f/0x3c0
>>> > kernel/locking/lockdep.c:3585
>>> >        [<     inline     >] __mutex_lock_common kernel/locking/mutex.c:518
>>> >        [<ffffffff85c86ba6>] mutex_lock_nested+0xb6/0x940
>>> > kernel/locking/mutex.c:618
>>> >        [<ffffffff82bc77ff>] n_tracerouter_open+0x1f/0x210
>>> > drivers/tty/n_tracerouter.c:75
>>> >        [<ffffffff82bacd78>] tty_ldisc_open.isra.2+0x78/0xd0
>>> > drivers/tty/tty_ldisc.c:447
>>> >        [<ffffffff82bad43a>] tty_set_ldisc+0x1ca/0xa30
>>> > drivers/tty/tty_ldisc.c:567
>>> >        [<     inline     >] tiocsetd drivers/tty/tty_io.c:2650
>>> >        [<ffffffff82b9483a>] tty_ioctl+0xb2a/0x2160 drivers/tty/tty_io.c:2883
>>> >        [<     inline     >] vfs_ioctl fs/ioctl.c:43
>>> >        [<ffffffff81750d41>] do_vfs_ioctl+0x681/0xe40 fs/ioctl.c:607
>>> >        [<     inline     >] SYSC_ioctl fs/ioctl.c:622
>>> >        [<ffffffff8175158f>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:613
>>> >        [<ffffffff85c8eb36>] entry_SYSCALL_64_fastpath+0x16/0x7a
>>> > arch/x86/entry/entry_64.S:185
>>
>> This is data being received by the new line discipline.
>>
>>         (buf->lock, routelock)
>>
>>> >
>>> > -> #0 (routelock){+.+...}:
>>> >        [<     inline     >] check_prev_add kernel/locking/lockdep.c:1853
>>> >        [<     inline     >] check_prevs_add kernel/locking/lockdep.c:1958
>>> >        [<     inline     >] validate_chain kernel/locking/lockdep.c:2144
>>> >        [<ffffffff813ee02e>] __lock_acquire+0x345e/0x3ca0
>>> > kernel/locking/lockdep.c:3206
>>> >        [<ffffffff813f0acf>] lock_acquire+0x19f/0x3c0
>>> > kernel/locking/lockdep.c:3585
>>> >        [<     inline     >] __mutex_lock_common kernel/locking/mutex.c:518
>>> >        [<ffffffff85c86ba6>] mutex_lock_nested+0xb6/0x940
>>> > kernel/locking/mutex.c:618
>>> >        [<ffffffff82bc77c0>] n_tracerouter_receivebuf+0x20/0x40
>>> > drivers/tty/n_tracerouter.c:175
>>> >        [<     inline     >] receive_buf drivers/tty/tty_buffer.c:454
>>> >        [<ffffffff82bafc34>] flush_to_ldisc+0x584/0x7f0
>>> > drivers/tty/tty_buffer.c:517
>>> >        [<ffffffff8133833c>] process_one_work+0x76c/0x13e0
>>> > kernel/workqueue.c:2030
>>> >        [<ffffffff81339093>] worker_thread+0xe3/0xe90 kernel/workqueue.c:2162
>>> >        [<ffffffff8134b63f>] kthread+0x23f/0x2d0 drivers/block/aoe/aoecmd.c:1303
>>> >        [<ffffffff85c8eeef>] ret_from_fork+0x3f/0x70
>>> > arch/x86/entry/entry_64.S:468
>>> >
>>
>> The lock inversion the log shows seems to be a problem in n_tty not in
>> the tracerouter code.
>>
>> However that seems to be
>>
>> tty_set_ldisc to N_TTY
>>         takes tty lock
>>         takes tty_ldisc lock
>>         down termios_rwsem (R)
>>         tty->ops->set_ldisc
>>         down termios rwsem (W)
>>
>> verus
>>
>> receive_driver_bufs
>>         buf->lock
>>         termios rwsem (R)
>>
>>
>> which also looks fine
>>
>> So I think it's just a false report caused by other changes in the tty
>> layer, and nothing to do with the tracerouter code at all.
>
>
> Thanks for looking into this.
>
> The report says:
>
>        CPU0                    CPU1
>        ----                    ----
>   lock(&buf->lock);
>                                lock(&o_tty->termios_rwsem/1);
>                                lock(&buf->lock);
>   lock(routelock);
>
> I don't see routelock in your stacks.
> But I also I don't understand why this is a deadlock. I would
> understand if one thread takes A then B; while another takes B then A.
> But here are 3 non-circular lock acquisitions. Does it have something
> to do with spinlocks vs mutex/rwsem?

+Peter, Ingo, can you please suggest a way to annotate this locking
pattern to avoid lockdep false positive?
It is cause by the fact that old and new ldisc locks are aliased?

[toc] | [prev] | [next] | [standalone]

#1313150 — Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc

On Wed, Jan 20, 2016 at 10:36:01AM +0100, Dmitry Vyukov wrote:

> +Peter, Ingo, can you please suggest a way to annotate this locking
> pattern to avoid lockdep false positive?

No, because I haven't a clue what the actual problem is.

The report is fairly clear on how:

 routelock
   &port->buf.lock/1
     &o_tty->termios_rwsem/1
       &buf->lock

gets established, and shows where we try:

  &buf->lock
    routelock

Which gives a circle, ergo deadlock.

Show which link is wrong and why, and I can suggest ways of annotating
that.

[toc] | [prev] | [next] | [standalone]

#1313160 — Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc

On Wed, Jan 20, 2016 at 12:44 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Jan 20, 2016 at 10:36:01AM +0100, Dmitry Vyukov wrote:
>
>> +Peter, Ingo, can you please suggest a way to annotate this locking
>> pattern to avoid lockdep false positive?
>
> No, because I haven't a clue what the actual problem is.
>
> The report is fairly clear on how:
>
>  routelock
>    &port->buf.lock/1
>      &o_tty->termios_rwsem/1
>        &buf->lock
>
> gets established, and shows where we try:
>
>   &buf->lock
>     routelock
>
> Which gives a circle, ergo deadlock.
>
> Show which link is wrong and why, and I can suggest ways of annotating
> that.


Alan provided an explanation calling this "false report":
https://groups.google.com/d/msg/syzkaller/YrV0bzdfa-g/cCVoUf1OFQAJ


And I don't understand how the following is a deadlock, since there is
no cycle...

 Possible unsafe locking scenario:
      CPU0                    CPU1
       ----                    ----
  lock(&buf->lock);
                               lock(&o_tty->termios_rwsem/1);
                               lock(&buf->lock);
  lock(routelock);


Shouldn't it look like:

 Possible unsafe locking scenario:
      CPU0                    CPU1
       ----                    ----
  lock(&buf->lock);
                               lock(routelock);
                               lock(&buf->lock);
  lock(routelock);

?

[toc] | [prev] | [next] | [standalone]

#1313171 — Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc

On Wed, Jan 20, 2016 at 12:54:55PM +0100, Dmitry Vyukov wrote:
> On Wed, Jan 20, 2016 at 12:44 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Wed, Jan 20, 2016 at 10:36:01AM +0100, Dmitry Vyukov wrote:
> >
> >> +Peter, Ingo, can you please suggest a way to annotate this locking
> >> pattern to avoid lockdep false positive?
> >
> > No, because I haven't a clue what the actual problem is.
> >
> > The report is fairly clear on how:
> >
> >  routelock
> >    &port->buf.lock/1
> >      &o_tty->termios_rwsem/1
> >        &buf->lock
> >
> > gets established, and shows where we try:
> >
> >   &buf->lock
> >     routelock
> >
> > Which gives a circle, ergo deadlock.
> >
> > Show which link is wrong and why, and I can suggest ways of annotating
> > that.
> 
> 
> Alan provided an explanation calling this "false report":
> https://groups.google.com/d/msg/syzkaller/YrV0bzdfa-g/cCVoUf1OFQAJ

I read that, I didn't understand it. Which link is wrong and why?

> And I don't understand how the following is a deadlock, since there is
> no cycle...
> 
>  Possible unsafe locking scenario:
>       CPU0                    CPU1
>        ----                    ----
>   lock(&buf->lock);
>                                lock(&o_tty->termios_rwsem/1);
>                                lock(&buf->lock);
>   lock(routelock);

Ignore the stupid picture, it only really works for simple cases.

[toc] | [prev] | [next] | [standalone]

#1313269

> I read that, I didn't understand it. Which link is wrong and why?
> 
> > And I don't understand how the following is a deadlock, since there is
> > no cycle...
> > 
> >  Possible unsafe locking scenario:
> >       CPU0                    CPU1
> >        ----                    ----
> >   lock(&buf->lock);
> >                                lock(&o_tty->termios_rwsem/1);
> >                                lock(&buf->lock);
> >   lock(routelock);
> 
> Ignore the stupid picture, it only really works for simple cases.

There are two line disciplines using two different locking orders

The two line disciplines never execute at once. A given tty is either
using one or the other and there is a clear and correctly locked
changeover.


semantically its something a bit like


 foo(x)
 {
          if (x == 1) {
		lock(A)
		lock(B)
	  } else {
		lock(B)
		lock(A)
	  }

	  Do stuff();

	  if (x == 1) {
		unlock(B)
		unlock(A)
	  } else {
		unlock(A)
		unlock(B)
	  }
}

with the guarantee made elsewhere that no instances of foo(1) and foo(0)
are ever executing at the same time.

That's not by dumb design - it's an interesting "nobody ever noticed
this" turned up by the lock detector between two totaly unrelated bits of
code.

Alan

[toc] | [prev] | [next] | [standalone]

#1313291 — Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc

On Wed, Jan 20, 2016 at 3:58 PM, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
>> I read that, I didn't understand it. Which link is wrong and why?
>>
>> > And I don't understand how the following is a deadlock, since there is
>> > no cycle...
>> >
>> >  Possible unsafe locking scenario:
>> >       CPU0                    CPU1
>> >        ----                    ----
>> >   lock(&buf->lock);
>> >                                lock(&o_tty->termios_rwsem/1);
>> >                                lock(&buf->lock);
>> >   lock(routelock);
>>
>> Ignore the stupid picture, it only really works for simple cases.
>
> There are two line disciplines using two different locking orders
>
> The two line disciplines never execute at once. A given tty is either
> using one or the other and there is a clear and correctly locked
> changeover.
>
>
> semantically its something a bit like
>
>
>  foo(x)
>  {
>           if (x == 1) {
>                 lock(A)
>                 lock(B)
>           } else {
>                 lock(B)
>                 lock(A)
>           }
>
>           Do stuff();
>
>           if (x == 1) {
>                 unlock(B)
>                 unlock(A)
>           } else {
>                 unlock(A)
>                 unlock(B)
>           }
> }
>
> with the guarantee made elsewhere that no instances of foo(1) and foo(0)
> are ever executing at the same time.
>
> That's not by dumb design - it's an interesting "nobody ever noticed
> this" turned up by the lock detector between two totaly unrelated bits of
> code.

In out user-space deadlock detector we have an annotation along the
lines of "forget all info this particular mutex" for such cases
(between foo(0) and foo(1)). Is there something similar in lockdep?

[toc] | [prev] | [next] | [standalone]

#1313345 — Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc

On Wed, Jan 20, 2016 at 04:16:52PM +0100, Dmitry Vyukov wrote:
> On Wed, Jan 20, 2016 at 3:58 PM, One Thousand Gnomes
> <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> I read that, I didn't understand it. Which link is wrong and why?
> >>
> >> > And I don't understand how the following is a deadlock, since there is
> >> > no cycle...
> >> >
> >> >  Possible unsafe locking scenario:
> >> >       CPU0                    CPU1
> >> >        ----                    ----
> >> >   lock(&buf->lock);
> >> >                                lock(&o_tty->termios_rwsem/1);
> >> >                                lock(&buf->lock);
> >> >   lock(routelock);
> >>
> >> Ignore the stupid picture, it only really works for simple cases.
> >
> > There are two line disciplines using two different locking orders
> >
> > The two line disciplines never execute at once. A given tty is either
> > using one or the other and there is a clear and correctly locked
> > changeover.
> >
> >
> > semantically its something a bit like
> >
> >
> >  foo(x)
> >  {
> >           if (x == 1) {
> >                 lock(A)
> >                 lock(B)
> >           } else {
> >                 lock(B)
> >                 lock(A)
> >           }
> >
> >           Do stuff();
> >
> >           if (x == 1) {
> >                 unlock(B)
> >                 unlock(A)
> >           } else {
> >                 unlock(A)
> >                 unlock(B)
> >           }
> > }
> >
> > with the guarantee made elsewhere that no instances of foo(1) and foo(0)
> > are ever executing at the same time.
> >
> > That's not by dumb design - it's an interesting "nobody ever noticed
> > this" turned up by the lock detector between two totaly unrelated bits of
> > code.

Its a fairly common thing; the typical solution uses lockdep_class_key
instances per type. See for example inodes, they have a per
file_system_type classes exactly because of this. See
inode_init_always()'s use of lockdep_set_class() and struct file_system_type.

The thing with lockdep_class_key's is that they must reside in static
storage, a quick grep shows that all tty_ldisc_ops structures are indeed
in static storage.

I'm just not sure when to apply the lock classes and to which locks though.
The one thing to keep in mind is that lockdep_set_class() cannot be used
on a held lock (I'm not sure we have debug msgs for that, /me makes a
note).

> In out user-space deadlock detector we have an annotation along the
> lines of "forget all info this particular mutex" for such cases
> (between foo(0) and foo(1)). Is there something similar in lockdep?

No, since we don't track this data per instance, and the lock chains
that were recorded are still valid, just not for this instance.

[toc] | [prev] | [next] | [standalone]

#1312812

On Fri, 2016-01-15 at 16:33 +0000, One Thousand Gnomes wrote:
> On Fri, 15 Jan 2016 08:51:39 +0100
> Dmitry Vyukov <dvyukov@google.com> wrote:
> 
> > On Wed, Dec 30, 2015 at 11:44 AM, Dmitry Vyukov <dvyukov@google.com
> > > wrote:
> > > Hello,
> > > 
> > > I've got the following lock order inversion report on
> > > 8513342170278468bac126640a5d2d12ffbff106 (Dec 28). Unfortunately
> > > I
> > > failed to find a reproducer for it.
> > 
> > Ping. This is the most frequent failure during my testing now. J
> > Freyensee, you seem to wrote drivers/tty/n_tracerouter.c. Please
> > take
> > a look. Thanks.

I did look at the report but didn't see how n_tracerouter could be
causing the issue.  I apologize for not responding, and I thank you so
very much for providing the thorough explanation, Alan.

Jay

> 
> Alan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web