Re: tty: deadlock between n_tracerouter_receivebuf and flush_to_ldisc

[Peter Zijlstra]

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@xxxxxxxxxxxxxxxxxxx> 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.