From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Peter Hurley <peter@hurleysoftware.com>,
Jiri Slaby <jirislaby@kernel.org>,
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 09:31:06 +0100 [thread overview]
Message-ID: <77983591.hkYTjcaLry@localhost.localdomain> (raw)
In-Reply-To: <df266c83-88df-4d1a-5c7e-ea0214f3de3b@i-love.sakura.ne.jp>
On Wednesday, November 17, 2021 11:51:13 AM CET Tetsuo Handa wrote:
> On 2021/11/17 17:54, Greg Kroah-Hartman wrote:
> > Great, you have a reproducer, so you should be able to duplicate this
> > locally to figure out what is really happening here.
>
> Until commit ac751efa6a0d70f2 ("console: rename acquire/release_console_sem() to
> console_lock/unlock()"), do_con_write() was surely designed to be able to sleep.
>
> > $ git blame ac751efa6a0d7~1 drivers/tty/vt/vt.c
>
> [...]
>
> Until that commit, n_hdlc_send_frames() was prepared for being interrupted by signal
> while sleeping.
>
> $ git blame ac751efa6a0d7~1 drivers/tty/n_hdlc.c
>
> [...]
>
> But as of commit c545b66c6922b002 ("tty: Serialize tcflow() with other tty flow
> control changes"), start_tty() was already holding spinlock.
Hi Tetsuo,
Actually, we don't care of start_tty(). It's not in the path that triggers sleeping in atomic bug.
According to Syzbot report and to my ftrace analysis it's __start_tty() that is called by
n_tty_ioctl_helper(), and it is this function that acquires a spinlock and disables interrupts.
I must admit that I've never used git-blame and I'm not sure to understand what you did here :(
Have you had a chance to read my analysis?
> $ git blame c545b66c6922b002~1 drivers/tty/tty_io.c
>
> [...]
>
> Actually, it is commit f9e053dcfc02b0ad ("tty: Serialize tty flow control changes
> with flow_lock") that started calling tty->ops->start(tty) from atomic context.
>
> $ git blame f9e053dcfc02b~1 drivers/tty/tty_io.c
>
> [...]
>
> Therefore, I think that bisection will reach f9e053dcfc02b0ad, and I guess that
> this bug was not noticed simply because little people tested n_hdlc driver.
>
> Well, how to fix? Introduce a new flag for indicating "starting" state (like drivers/block/loop.c uses Lo_* state) ?
I think this is not the correct fix, but I might very well be wrong...
Can you please reply to my last email (the one with the ftrace analysis)?
In the last lines I proposed two alternative solutions, what about them?
Thanks,
Fabio
next prev parent reply other threads:[~2021-11-18 8:31 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-16 14:49 [PATCH] vt: Fix sleeping functions called from atomic context Fabio M. De Francesco
2021-11-16 14:58 ` Greg Kroah-Hartman
2021-11-16 15:35 ` Fabio M. De Francesco
2021-11-16 16:59 ` Greg Kroah-Hartman
2021-11-16 17:28 ` Fabio M. De Francesco
2021-11-17 8:23 ` Fabio M. De Francesco
2021-11-17 8:54 ` Greg Kroah-Hartman
2021-11-17 10:51 ` Tetsuo Handa
2021-11-18 8:31 ` Fabio M. De Francesco [this message]
2021-11-18 9:38 ` Fabio M. De Francesco
2021-11-18 12:14 ` Tetsuo Handa
2021-11-18 17:01 ` Fabio M. De Francesco
2021-11-19 14:55 ` [PATCH] tty: vt: make do_con_write() no-op if IRQ is disabled Tetsuo Handa
2021-12-01 13:40 ` Tetsuo Handa
2021-12-01 14:20 ` Greg Kroah-Hartman
2021-12-01 19:05 ` Linus Torvalds
2021-12-02 15:40 ` Tetsuo Handa
2021-12-02 18:35 ` Linus Torvalds
2021-12-03 5:03 ` Jiri Slaby
2021-12-03 11:00 ` Fabio M. De Francesco
2021-12-03 12:32 ` Tetsuo Handa
2021-12-03 14:51 ` Fabio M. De Francesco
2021-11-17 12:38 ` [PATCH] vt: Fix sleeping functions called from atomic context Fabio M. De Francesco
2021-11-17 1:55 ` Tetsuo Handa
2021-11-17 7:02 ` Fabio M. De Francesco
2021-12-06 11:44 ` [PATCH] tty: n_hdlc: make n_hdlc_tty_wakeup() asynchronous Tetsuo Handa
2021-12-06 18:07 ` Linus Torvalds
2021-12-09 13:18 ` Tetsuo Handa
2021-12-15 11:52 ` [PATCH (resend)] " Tetsuo Handa
2021-12-06 19:06 ` [PATCH] " Fabio M. De Francesco
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=77983591.hkYTjcaLry@localhost.localdomain \
--to=fmdefrancesco@gmail.com \
--cc=dankamongmen@gmail.com \
--cc=dsterba@suse.com \
--cc=elver@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=igormtorrente@gmail.com \
--cc=jcmvbkbc@gmail.com \
--cc=jirislaby@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=peter@hurleysoftware.com \
--cc=syzbot+5f47a8cea6a12b77a876@syzkaller.appspotmail.com \
--cc=unixbhaskar@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).