linux-kernel - Re: tty: deadlock between n_tracerouter_receivebuf and flush_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 20 Jan 2016 17:32:54 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Dmitry Vyukov <dvyukov@...gle.com>
Cc:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Ingo Molnar <mingo@...hat.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.com>,
	LKML <linux-kernel@...r.kernel.org>,
	J Freyensee <james_p_freyensee@...ux.intel.com>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	Eric Dumazet <edumazet@...gle.com>
Subject: 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@...rguk.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.