linux-kernel - Re: [bisected] Re: tty lockdep trace

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]

Message-ID: <6b025884-8fbc-1ae2-3119-63e601453ad6@oracle.com>
Date:   Sun, 4 Jun 2017 12:00:17 +0200
From:   Vegard Nossum <vegard.nossum@...cle.com>
To:     Mike Galbraith <efault@....de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Dave Jones <davej@...emonkey.org.uk>,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        Jiri Slaby <jslaby@...e.com>
Subject: Re: [bisected] Re: tty lockdep trace

On 06/04/17 11:02, Mike Galbraith wrote:
> On Sun, 2017-06-04 at 10:32 +0200, Greg Kroah-Hartman wrote:
>> On Sat, Jun 03, 2017 at 08:33:52AM +0200, Mike Galbraith wrote:
>>> On Wed, 2017-05-31 at 13:21 -0400, Dave Jones wrote:
>>>> Just hit this during a trinity run.
>>>
>>> 925bb1ce47f429f69aad35876df7ecd8c53deb7e is the first bad commit
>>> commit 925bb1ce47f429f69aad35876df7ecd8c53deb7e
>>> Author: Vegard Nossum <vegard.nossum@...cle.com>
>>> Date:   Thu May 11 12:18:52 2017 +0200
>>>
>>>      tty: fix port buffer locking
>>
>> Now reverting this.  Oops, sorry, forgot to add Dave and your names to
>> the patch revert.  The list of people who reported this was really long,
>> many thanks for this.
> 
> If flush_to_ldisc() is the problem, and taking atomic_write_lock in
> that path an acceptable solution, how about do that a bit differently
> instead.  Lockdep stopped grumbling, vbox seems happy.
> 
> 925bb1ce47f4 (tty: fix port buffer locking) upset lockdep by holding buf->lock
> while acquiring tty->atomic_write_lock.  Move acquisition to flush_to_ldisc(),
> taking it prior to taking buf->lock.  Costs a reference, but appeases lockdep.
> 
> Not-so-signed-off-by: /me
> ---
>   drivers/tty/tty_buffer.c |   10 ++++++++++
>   drivers/tty/tty_port.c   |    2 --
>   2 files changed, 10 insertions(+), 2 deletions(-)
> 
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -465,7 +465,13 @@ static void flush_to_ldisc(struct work_s
>   {
>   	struct tty_port *port = container_of(work, struct tty_port, buf.work);
>   	struct tty_bufhead *buf = &port->buf;
> +	struct tty_struct *tty = READ_ONCE(port->itty);
> +	struct tty_ldisc *disc = NULL;
>   
> +	if (tty)
> +		disc = tty_ldisc_ref(tty);
> +	if (disc)
> +		mutex_lock(&tty->atomic_write_lock);
>   	mutex_lock(&buf->lock);
>   
>   	while (1) {
> @@ -501,6 +507,10 @@ static void flush_to_ldisc(struct work_s
>   	}
>   
>   	mutex_unlock(&buf->lock);
> +	if (disc) {
> +		mutex_unlock(&tty->atomic_write_lock);
> +		tty_ldisc_deref(disc);
> +	}
>   
>   }
>   
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -34,9 +34,7 @@ static int tty_port_default_receive_buf(
>   	if (!disc)
>   		return 0;
>   
> -	mutex_lock(&tty->atomic_write_lock);
>   	ret = tty_ldisc_receive_buf(disc, p, (char *)f, count);
> -	mutex_unlock(&tty->atomic_write_lock);
>   
>   	tty_ldisc_deref(disc);
>   
>
I don't know how you did it, but this passes my testing (reproducers for
both the original issue and the lockdep splat/hang). Although given the
track record I'm not sure how much that's worth :-/


Vegard