Re: [PATCH] vt: Fix sleeping functions called from atomic context

From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>,
	Max Filippov <jcmvbkbc@gmail.com>,
	David Sterba <dsterba@suse.com>,
	Bhaskar Chowdhury <unixbhaskar@gmail.com>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	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: Wed, 17 Nov 2021 09:23:45 +0100	[thread overview]
Message-ID: <1851530.LmWsTuDv9j@localhost.localdomain> (raw)
In-Reply-To: <YZPjbI/uCNtugFJZ@kroah.com>

On Tuesday, November 16, 2021 5:59:24 PM CET Greg Kroah-Hartman wrote:
> On Tue, Nov 16, 2021 at 04:35:07PM +0100, Fabio M. De Francesco wrote:
> > On Tuesday, November 16, 2021 3:58:44 PM CET Greg Kroah-Hartman wrote:
> > > On Tue, Nov 16, 2021 at 03:49:37PM +0100, Fabio M. De Francesco wrote:
> > > > Fix two sleeping functions called from atomic context by doing 
immediate
> > > > return to the caller if !preemptible() evaluates 'true'. Remove two
> > > > in_interrupt() tests because they are not suited for being used here.
> > > > 
> > > > Since functions do_con_write() and con_flush_chars() might sleep in
> > > > console_lock(), it must be assured that they are never executed in
> > > > atomic contexts.
> > > > 
> > > > This issue is reported by Syzbot which notices that they are executed
> > > > while holding spinlocks and with interrupts disabled. Actually Syzbot
> > > > emits a first report and then, after fixing do_con_write(), a second
> > > > report for the same problem in con_flush_chars() because these 
functions
> > > > are called one after the other by con_write().
> > > > 
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Reported-by: syzbot+5f47a8cea6a12b77a876@syzkaller.appspotmail.com
> > > > Suggested-by: Marco Elver <elver@google.com>
> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > > ---
> > > >  drivers/tty/vt/vt.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > > index 7359c3e80d63..508f8a56d361 100644
> > > > --- 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 (!preemptible())
> > > >  		return count;
> > > 
> > > Very odd, what code is calling these functions to trigger this check?
> > 
> > This is the call trace reported by Syzbot (https://syzkaller.appspot.com/
bug?
> > id=fe5a4d5a2482bd73064db5de5d28e024f1e2a387):
> > 
> > Call Trace:
> >  <TASK>
> >  __dump_stack lib/dump_stack.c:88 [inline]
> >  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> >  __might_resched.cold+0x222/0x26b kernel/sched/core.c:9539
> >  console_lock+0x17/0x80 kernel/printk/printk.c:2522
> >  do_con_write+0x10f/0x1e40 drivers/tty/vt/vt.c:2908
> >  con_write+0x21/0x40 drivers/tty/vt/vt.c:3295
> >  n_hdlc_send_frames+0x24b/0x490 drivers/tty/n_hdlc.c:290
> >  tty_wakeup+0xe1/0x120 drivers/tty/tty_io.c:534
> >  __start_tty drivers/tty/tty_io.c:806 [inline]
> >  __start_tty+0xfb/0x130 drivers/tty/tty_io.c:799
> >  n_tty_ioctl_helper+0x299/0x2d0 drivers/tty/tty_ioctl.c:880
> > 
> > 	^^^^^^^^^^
> > n_tty_ioctl_helper() disabled interrupts via spin_lock_irq(&tty-
>flow.lock).
> > 
> >  n_hdlc_tty_ioctl+0xd2/0x340 drivers/tty/n_hdlc.c:633
> >  tty_ioctl+0xc69/0x1670 drivers/tty/tty_io.c:2814
> >  vfs_ioctl fs/ioctl.c:51 [inline]
> >  __do_sys_ioctl fs/ioctl.c:874 [inline]
> >  __se_sys_ioctl fs/ioctl.c:860 [inline]
> >  __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:860
> >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> >  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> >  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > 
> > > Shouldn't the caller be fixed instead?
> > 
> > Maybe that the caller has no need to disable IRQs, but I cannot yet 
answer to 
> > this particular question.
> > 
> > > What changed to suddenly cause this to show up?
> > 
> > Commit c545b66c6922 ("tty: Serialize tcflow() with other tty flow control 
> > changes") introduced a call to spin_lock_irq() for command "TCOON", just 
> > before calling __start_tty().
> 
> That commit happened in 2014.  Why is this suddenly an issue now that no
> one ever saw before?

Hi Greg,

I have just been informed by Pavel Skripkin (who at this moment cannot 
directly write to LKML) that this bug has been reported at least other two 
times. 

We cannot say why nobody has ever addressed this issue before.

Please read the following reports.

https://syzkaller.appspot.com/bug?id=9ed44c22919e3cd2c71907b963fb048ac1115073
https://syzkaller.appspot.com/bug?id=e806305a3a65a0bb2f1be7e672819a079d558016

Thanks,

Fabio

> I am worried you are not actually fixing the real issue here by just
> making syzbot be quiet.  Can you work to figure out what the real issue
> is please?
> 
> thanks,
> 
> greg k-h
> 