Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <31a4dec3d36ed131402244693cae180816ebd4d7.camel@t-2.net>
Subject: Re: How to handle concurrent access to /dev/ttyprintk ?
From:   Samo =?UTF-8?Q?Poga=C4=8Dnik?= <samo_pogacnik@t-2.net>
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>
Cc:     Petr Mladek <pmladek@suse.com>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        John Ogness <john.ogness@linutronix.de>,
        linux-kernel@vger.kernel.org,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>
Date:   Wed, 14 Apr 2021 18:15:23 +0200
In-Reply-To: <095d5393-b212-c4d8-5d6d-666bd505cc3d@i-love.sakura.ne.jp>
References: <20210403041444.4081-1-penguin-kernel@I-love.SAKURA.ne.jp>
         <YGx59PEq2Y015YdK@alley>
         <3c15d32f-c568-7f6f-fa7e-af4deb9b49f9@i-love.sakura.ne.jp>
         <d78ae8da-16e9-38d9-e274-048c54e24360@i-love.sakura.ne.jp>
         <YG24F9Kx+tjxhh8G@kroah.com>
         <051b550c-1cdd-6503-d2b7-0877bf0578fc@i-love.sakura.ne.jp>
         <cd213843-45fe-2eac-4943-0906ab8d272b@i-love.sakura.ne.jp>
         <YHQkeZVs3pmyie9e@kroah.com>
         <32e75be6-6e9f-b33f-d585-13db220519da@i-love.sakura.ne.jp>
         <YHQ3Zy9gRdZsu77w@kroah.com>
         <ffcc8099-614c-f4b1-10c1-f1d4c7f72e65@i-love.sakura.ne.jp>
         <095d5393-b212-c4d8-5d6d-666bd505cc3d@i-love.sakura.ne.jp>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Dne 14.04.2021 (sre) ob 20:11 +0900 je Tetsuo Handa napisal(a):
> On 2021/04/14 9:45, Tetsuo Handa wrote:
> > On 2021/04/12 21:04, Greg Kroah-Hartman wrote:
> > > > Since syzkaller is a fuzzer, syzkaller happily opens /dev/ttyprintk from
> > > > multiple threads. Should we update syzkaller to use CONFIG_TTY_PRINTK=n
> > > > ?
> > > 
> > > Why?  Can you not hit the same tty code paths from any other tty driver
> > > being open multiple times?  Why is ttyprintk somehow "special" here?
> > 
> > I found a simplified reproducer. If we call ioctl(TIOCVHANGUP) on
> > /dev/ttyprintk ,
> > "struct ttyprintk_port tpk_port".port.count cannot be decremented by
> > tty_port_close() from tpk_close() due to tty_hung_up_p() == true when
> > close() is called. As a result, tty->count and port count gets out of sync.
> > 
> > Then, when /dev/ttyprintk is opened again and then closed without calling
> > ioctl(TIOCVHANGUP), this message is printed due to tty_hung_up_p() == false.
> > 
> > If I replace /dev/ttyprintk with /dev/ttyS0 in the reproducer shown above,
> > this message is not printed.
> > 
> 
> The difference between /dev/ttyS0 and /dev/ttyprintk is that
> the former provides uart_hangup() as "struct tty_operations"->hangup
> while the latter does not provide "struct tty_operations"->hangup .
> 
> It seems to me that below patch avoids this message, but I'm not familiar
> with tty code. Is this fix correct? Don't we need to enforce all drivers
> to provide "struct tty_operations"->hangup in order to reset port count ?
> 
> diff --git a/drivers/char/ttyprintk.c b/drivers/char/ttyprintk.c
> index 6a0059e508e3..ff3b9a41b91b 100644
> --- a/drivers/char/ttyprintk.c
> +++ b/drivers/char/ttyprintk.c
> @@ -158,12 +158,26 @@ static int tpk_ioctl(struct tty_struct *tty,
>  	return 0;
>  }
>  
> +/*
> + * TTY operations hangup function.
> + */
> +static void tpk_hangup(struct tty_struct *tty)
> +{
> +	struct ttyprintk_port *tpkp = tty->driver_data;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tpkp->port.lock, flags);
> +	tpkp->port.count = 0;
> +	spin_unlock_irqrestore(&tpkp->port.lock, flags);
> +}
> +
>  static const struct tty_operations ttyprintk_ops = {
>  	.open = tpk_open,
>  	.close = tpk_close,
>  	.write = tpk_write,
>  	.write_room = tpk_write_room,
>  	.ioctl = tpk_ioctl,
> +	.hangup = tpk_hangup,
>  };
>  
>  static const struct tty_port_operations null_ops = { };

Wish i could be of more help here, especially around locking.

If this addition is needed, i'd probably do something similar.
However, when comparing the code around other drivers it seems that
'tty_port_hangup()' should be used instead explicit 'port.count'
reset.

best regards, Samo