From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Peter Hurley <peter@hurleysoftware.com>,
        Jiri Slaby <jirislaby@kernel.org>,
        Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc:     Max Filippov <jcmvbkbc@gmail.com>, David Sterba <dsterba@suse.com>,
        Bhaskar Chowdhury <unixbhaskar@gmail.com>,
        Igor Matheus Andrade Torrente <igormtorrente@gmail.com>,
        nick black <dankamongmen@gmail.com>,
        linux-kernel@vger.kernel.org,
        syzbot+5f47a8cea6a12b77a876@syzkaller.appspotmail.com,
        Marco Elver <elver@google.com>
Subject: Re: [PATCH] vt: Fix sleeping functions called from atomic context
Date:   Thu, 18 Nov 2021 18:01:24 +0100
Message-ID: <1701119.OD4kndUEs1@localhost.localdomain>
In-Reply-To: <44d83e9c-d8c9-38bf-501c-019b8c2f7b5e@i-love.sakura.ne.jp>
References: <20211116144937.19035-1-fmdefrancesco@gmail.com> <1825634.Qa45DEmgBM@localhost.localdomain> <44d83e9c-d8c9-38bf-501c-019b8c2f7b5e@i-love.sakura.ne.jp>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thursday, November 18, 2021 1:14:59 PM CET Tetsuo Handa wrote:
> On 2021/11/18 18:38, Fabio M. De Francesco wrote:

> If ->flow.lock were held from only schedulable context, replacing this spinlock with
> a mutex would be possible. But stop_tty() says "may be called from any context" which
> means that we can't use a mutex...
> 
> Making do_con_write() no-op when called with IRQs disabled would be the minimal change
> that can silence the syzbot. But this does not fix the regression for drivers/tty/n_hdlc.c
> introduced by f9e053dcfc02b0ad.
> 
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -2902,7 +2902,7 @@ static int do_con_write(struct tty_struct *tty, const unsigned char *buf, int co
>  	struct vt_notifier_param param;
>  	bool rescan;
>  
> -	if (in_interrupt())
> +	if (in_interrupt() || irqs_disabled())
>  		return count;
>  
>  	console_lock();
> @@ -3358,7 +3358,7 @@ static void con_flush_chars(struct tty_struct *tty)
>  {
>  	struct vc_data *vc;
>  
> -	if (in_interrupt())	/* from flush_to_ldisc */
> +	if (in_interrupt() || irqs_disabled())	/* from flush_to_ldisc */
>  		return;
>  
>  	/* if we race with con_close(), vt may be null */
>

For what my opinion is worth, I like the solution by Tetsuo that is reported above. 
The bug is real and I suppose that it must be addressed. This seems the most straightforward
and effective way to fix it.

Regards,

Fabio M. De Francesco
 
> According to scripts/get_maintainer.pl , Greg and Jiri are maintainers for the n_hdlc driver.
> Greg and Jiri, what do you think? Is sacrificing ability to write to consoles when
> n_hdlc_send_frames() is called from __start_tty() path considered tolerable? (Maybe
> OK for now and stable kernels, for nobody was reporting this problem suggests that
> nobody depends on this ability.)
> 
> But if we must fix the regression for drivers/tty/n_hdlc.c , we need to use something
> like https://lkml.kernel.org/r/7d851c88-f657-dfd8-34ab-4891ac6388dc@i-love.sakura.ne.jp
> in order to achieve what f9e053dcfc02b0ad meant. That is, extend tty->stopped in order
> to be able to represent "started state (currently indicated by tty->stopped == false)",
> "stopped state (currently indicated by tty->stopped == true)" and "changing state
> (currently impossible due to bool)", but this approach might need to touch many locations,
> and I worry that touching many locations introduces some oversight bugs.
> 
>