From: Doug Anderson <dianders@chromium.org>
To: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Jason Wessel <jason.wessel@windriver.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jiri Slaby <jirislaby@kernel.org>,
kgdb-bugreport@lists.sourceforge.net,
linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
Liuye <liu.yeC@h3c.com>,
stable@vger.kernel.org
Subject: Re: [PATCH] serial: kgdboc: Fix NMI-safety problems from keyboard reset code
Date: Mon, 22 Apr 2024 14:54:08 -0700 [thread overview]
Message-ID: <CAD=FV=UHt7Pm-qEBs7vtK0B0DCbu9YbU465OdpSKCYZVpNuOaA@mail.gmail.com> (raw)
In-Reply-To: <20240419-kgdboc_fix_schedule_work-v1-1-ff19881677e5@linaro.org>
Hi,
On Fri, Apr 19, 2024 at 3:30 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> Currently, when kdb is compiled with keyboard support, then we will use
> schedule_work() to provoke reset of the keyboard status. Unfortunately
> schedule_work() gets called from the kgdboc post-debug-exception
> handler. That risks deadlock since schedule_work() is not NMI-safe and,
> even on platforms where the NMI is not directly used for debugging, the
> debug trap can have NMI-like behaviour depending on where breakpoints
> are placed.
>
> Fix this by using the irq work system, which is NMI-safe, to defer the
> call to schedule_work() to a point when it is safe to call.
>
> Reported-by: Liuye <liu.yeC@h3c.com>
> Closes: https://lore.kernel.org/all/20240228025602.3087748-1-liu.yeC@h3c.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
> drivers/tty/serial/kgdboc.c | 30 +++++++++++++++++++++++++++++-
> 1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 7ce7bb1640054..adcea70fd7507 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -19,6 +19,7 @@
> #include <linux/console.h>
> #include <linux/vt_kern.h>
> #include <linux/input.h>
> +#include <linux/irq_work.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/serial_core.h>
> @@ -48,6 +49,25 @@ static struct kgdb_io kgdboc_earlycon_io_ops;
> static int (*earlycon_orig_exit)(struct console *con);
> #endif /* IS_BUILTIN(CONFIG_KGDB_SERIAL_CONSOLE) */
>
> +/*
> + * When we leave the debug trap handler we need to reset the keyboard status
> + * (since the original keyboard state gets partially clobbered by kdb use of
> + * the keyboard).
> + *
> + * The path to deliver the reset is somewhat circuitous.
> + *
> + * To deliver the reset we register an input handler, reset the keyboard and
> + * then deregister the input handler. However, to get this done right, we do
> + * have to carefully manage the calling context because we can only register
> + * input handlers from task context.
> + *
> + * In particular we need to trigger the action from the debug trap handler with
> + * all its NMI and/or NMI-like oddities. To solve this the kgdboc trap exit code
> + * (the "post_exception" callback) uses irq_work_queue(), which is NMI-safe, to
> + * schedule a callback from a hardirq context. From there we have to defer the
> + * work again, this time using schedule_Work(), to get a callback using the
nit: schedule_work() (no capital "W").
> + * system workqueue, which runs in task context.
Thank you for the comment. It makes the double-jump through IRQ work
and then normal work clearer.
Other than the nit in the comment, this looks good to me.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
prev parent reply other threads:[~2024-04-22 21:54 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-19 10:30 [PATCH] serial: kgdboc: Fix NMI-safety problems from keyboard reset code Daniel Thompson
2024-04-22 21:54 ` Doug Anderson [this message]
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='CAD=FV=UHt7Pm-qEBs7vtK0B0DCbu9YbU465OdpSKCYZVpNuOaA@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=daniel.thompson@linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=jason.wessel@windriver.com \
--cc=jirislaby@kernel.org \
--cc=kgdb-bugreport@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=liu.yeC@h3c.com \
--cc=stable@vger.kernel.org \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).