From: Petr Mladek <pmladek@suse.com>
To: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jiri Slaby <jirislaby@kernel.org>,
Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
Steven Rostedt <rostedt@goodmis.org>,
John Ogness <john.ogness@linutronix.de>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] tty: use printk_safe context at tty_msg()
Date: Tue, 6 Apr 2021 17:10:44 +0200 [thread overview]
Message-ID: <YGx59PEq2Y015YdK@alley> (raw)
In-Reply-To: <20210403041444.4081-1-penguin-kernel@I-love.SAKURA.ne.jp>
On Sat 2021-04-03 13:14:44, Tetsuo Handa wrote:
> syzbot is reporting circular locking dependency due to calling printk()
> with port lock held [1]. When this problem was reported, we worried
Could you please include the lockdep report into the commit message?
External links are not guaranteed to stay.
> whether printk_safe context will remain available in future kernels [2],
> and then this problem was forgotten. But in order to utilize syzbot's
> resource for finding other bugs/reproducers by closing this one of
> top
Is it possible to quantify how many resources are wasted by this
bug, please? Is this visible in one run from 10 or from 1000
or from 100000 runs?
I ask because this is just a workaround. It takes our time instead
of working on the proper solution. It will also add extra work when
the right solution is in please. See below for more details.
> crashers, let's apply a patch which counts on availability of printk_safe
> context.
>
> syzbot is also reporting same dependency due to memory allocation fault
> injection at tty_buffer_alloc(). Although __GFP_NOWARN cannot prevent
> memory allocation fault injection from calling printk(), let's use
> __GFP_NOWARN at tty_buffer_alloc() in addition to using printk_safe
> context, for generating many lines of messages due to warn_alloc() is
> annoying. If we want to report it, we can use pr_warn() instead.
>
> [1] https://syzkaller.appspot.com/bug?id=39ea6caa479af471183997376dc7e90bc7d64a6a
> [2] https://lkml.kernel.org/r/20190218054649.GA26686@jagdpanzerIV
>
> Reported-by: syzbot <syzbot+43e93968b964e369db0b@syzkaller.appspotmail.com>
> Reported-by: syzbot <syzbot+3ed715090790806d8b18@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Fixes: b6da31b2c07c46f2 ("tty: Fix data race in tty_insert_flip_string_fixed_flag")
> Cc: <stable@vger.kernel.org> # 4.18+
> ---
> drivers/tty/tty_buffer.c | 5 ++++-
> include/linux/tty.h | 9 ++++++++-
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 6d4995a5f318..d59f7873bc49 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -156,6 +156,7 @@ static struct tty_buffer *tty_buffer_alloc(struct tty_port *port, size_t size)
> {
> struct llist_node *free;
> struct tty_buffer *p;
> + unsigned long flags;
>
> /* Round the buffer size out */
> size = __ALIGN_MASK(size, TTYB_ALIGN_MASK);
> @@ -172,7 +173,9 @@ static struct tty_buffer *tty_buffer_alloc(struct tty_port *port, size_t size)
> have queued and recycle that ? */
> if (atomic_read(&port->buf.mem_used) > port->buf.mem_limit)
> return NULL;
> - p = kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC);
> + printk_safe_enter_irqsave(flags);
> + p = kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC | __GFP_NOWARN);
> + printk_safe_exit_irqrestore(flags);
I do not see tty_buffer_alloc() anywhere at
https://syzkaller.appspot.com/bug?id=39ea6caa479af471183997376dc7e90bc7d64a6a
Could you please provide more details why this is needed?
> if (p == NULL)
> return NULL;
>
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 95fc2f100f12..7ae8eb46fec3 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -14,6 +14,7 @@
> #include <uapi/linux/tty.h>
> #include <linux/rwsem.h>
> #include <linux/llist.h>
> +#include <../../kernel/printk/internal.h>
Do not do this, please! We would need to make the printk_safe() API
public if we want to use it outside kernel/printk/
> /*
> @@ -773,7 +774,13 @@ static inline void proc_tty_unregister_driver(struct tty_driver *d) {}
> #endif
>
> #define tty_msg(fn, tty, f, ...) \
> - fn("%s %s: " f, tty_driver_name(tty), tty_name(tty), ##__VA_ARGS__)
> + do { \
> + unsigned long flags; \
> + \
> + printk_safe_enter_irqsave(flags); \
> + fn("%s %s: " f, tty_driver_name(tty), tty_name(tty), ##__VA_ARGS__); \
> + printk_safe_exit_irqrestore(flags); \
> + } while (0)
There is no need to call single printk() in printk_safe() context.
printk_deferred() is enough.
Anyway, I am not sure if it is really a deadlock. One part of the
cycle is:
PU: 1 PID: 9840 Comm: syz-executor339 Not tainted 5.12.0-rc6-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:79 [inline]
dump_stack+0x141/0x1d7 lib/dump_stack.c:120
check_noncircular+0x25f/0x2e0 kernel/locking/lockdep.c:2127
check_prev_add kernel/locking/lockdep.c:2936 [inline]
check_prevs_add kernel/locking/lockdep.c:3059 [inline]
validate_chain kernel/locking/lockdep.c:3674 [inline]
__lock_acquire+0x2b14/0x54c0 kernel/locking/lockdep.c:4900
lock_acquire kernel/locking/lockdep.c:5510 [inline]
lock_acquire+0x1ab/0x740 kernel/locking/lockdep.c:5475
console_lock_spinning_enable kernel/printk/printk.c:1714 [inline]
console_unlock+0x371/0xc80 kernel/printk/printk.c:2573
vprintk_emit+0x1ca/0x560 kernel/printk/printk.c:2098
vprintk_func+0x8d/0x1e0 kernel/printk/printk_safe.c:401
printk+0xba/0xed kernel/printk/printk.c:2146
tty_port_close_start.part.0+0x503/0x550 drivers/tty/tty_port.c:569
tty_port_close_start drivers/tty/tty_port.c:641 [inline]
tty_port_close+0x46/0x170 drivers/tty/tty_port.c:634
tty_release+0x45e/0x1210 drivers/tty/tty_io.c:1779
__fput+0x288/0x920 fs/file_table.c:280
task_work_run+0xdd/0x1a0 kernel/task_work.c:140
tracehook_notify_resume include/linux/tracehook.h:189 [inline]
exit_to_user_mode_loop kernel/entry/common.c:174 [inline]
exit_to_user_mode_prepare+0x249/0x250 kernel/entry/common.c:208
__syscall_exit_to_user_mode_work kernel/entry/common.c:290 [inline]
syscall_exit_to_user_mode+0x19/0x60 kernel/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x44/0xae
It guess that the tty using this port_lock is not longer
registered console when tty_port_close() is called. If this
is true than the problematic port_lock will not longer
be taken from console_unlock().
Sure, this report breaks the test and might prevent finding
real problems.
Just note that printk_deferred()/printk_safe() is only temporary
solution. It will get obsoleted by offloading the console handling
to kthreads. It goes slowly because it is not trivial. We want
to do it a clean way and carefully. Any historic or new
workarounds/hacks would only slow it down.
My estimation is that we could have the offloaded consoles
within the next 3-4 development cycles.
Please, consider whether it is really worth adding these workarounds.
Best Regards,
Petr
next prev parent reply other threads:[~2021-04-06 15:10 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-03 4:14 [PATCH] tty: use printk_safe context at tty_msg() Tetsuo Handa
2021-04-03 6:52 ` kernel test robot
2021-04-03 10:11 ` [PATCH] printk: Make multiple inclusion of kernel/printk/internal.h safe Tetsuo Handa
2021-04-06 4:51 ` [PATCH] tty: use printk_safe context at tty_msg() Jiri Slaby
2021-04-06 5:31 ` Tetsuo Handa
2021-04-06 7:10 ` Greg Kroah-Hartman
2021-04-06 11:16 ` Tetsuo Handa
2021-04-06 13:42 ` Greg Kroah-Hartman
2021-04-06 15:10 ` Petr Mladek [this message]
2021-04-06 16:22 ` Tetsuo Handa
2021-04-06 19:10 ` Greg Kroah-Hartman
2021-04-07 9:20 ` Petr Mladek
2021-04-07 13:26 ` [PATCH v2] tty: use printk_deferred() " Tetsuo Handa
2021-04-07 13:48 ` Greg Kroah-Hartman
2021-04-07 14:24 ` Tetsuo Handa
2021-04-12 10:39 ` How to handle concurrent access to /dev/ttyprintk ? Tetsuo Handa
2021-04-12 10:44 ` Greg Kroah-Hartman
2021-04-12 11:25 ` Tetsuo Handa
2021-04-12 12:04 ` Greg Kroah-Hartman
2021-04-14 0:45 ` Tetsuo Handa
2021-04-14 11:11 ` Tetsuo Handa
2021-04-14 16:15 ` Samo Pogačnik
2021-04-15 0:22 ` [PATCH] ttyprintk: Add TTY hangup callback Tetsuo Handa
2021-04-18 11:16 ` Samo Pogačnik
2021-04-22 10:02 ` Greg Kroah-Hartman
2021-04-23 4:22 ` Jiri Slaby
2021-04-23 9:55 ` Samo Pogačnik
2021-04-23 10:12 ` Tetsuo Handa
2021-04-23 19:47 ` Samo Pogačnik
2021-04-24 1:16 ` Tetsuo Handa
2021-04-24 9:57 ` Samo Pogačnik
2021-04-26 10:00 ` Petr Mladek
2021-04-26 16:42 ` Samo Pogačnik
2021-04-27 10:08 ` Petr Mladek
2021-04-27 11:31 ` Samo Pogačnik
2021-04-23 10:28 ` Jiri Slaby
2021-04-23 12:23 ` [PATCH] ttyprintk: Add TTY port shutdown callback Samo Pogačnik
2021-04-12 12:41 ` How to handle concurrent access to /dev/ttyprintk ? Samo Pogačnik
2021-04-13 9:41 ` Petr Mladek
2021-04-13 11:10 ` Samo Pogačnik
2021-04-13 14:32 ` Petr Mladek
2021-04-13 15:22 ` Samo Pogačnik
2021-04-14 17:36 ` Petr Mladek
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=YGx59PEq2Y015YdK@alley \
--to=pmladek@suse.com \
--cc=gregkh@linuxfoundation.org \
--cc=jirislaby@kernel.org \
--cc=john.ogness@linutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=rostedt@goodmis.org \
--cc=sergey.senozhatsky@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).