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 <[email protected]>
Closes: https://lore.kernel.org/all/[email protected]/
Cc: [email protected]
Signed-off-by: Daniel Thompson <[email protected]>
---
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
+ * system workqueue, which runs in task context.
+ */
#ifdef CONFIG_KDB_KEYBOARD
static int kgdboc_reset_connect(struct input_handler *handler,
struct input_dev *dev,
@@ -99,10 +119,17 @@ static void kgdboc_restore_input_helper(struct work_struct *dummy)
static DECLARE_WORK(kgdboc_restore_input_work, kgdboc_restore_input_helper);
+static void kgdboc_queue_restore_input_helper(struct irq_work *unused)
+{
+ schedule_work(&kgdboc_restore_input_work);
+}
+
+static DEFINE_IRQ_WORK(kgdboc_restore_input_irq_work, kgdboc_queue_restore_input_helper);
+
static void kgdboc_restore_input(void)
{
if (likely(system_state == SYSTEM_RUNNING))
- schedule_work(&kgdboc_restore_input_work);
+ irq_work_queue(&kgdboc_restore_input_irq_work);
}
static int kgdboc_register_kbd(char **cptr)
@@ -133,6 +160,7 @@ static void kgdboc_unregister_kbd(void)
i--;
}
}
+ irq_work_sync(&kgdboc_restore_input_irq_work);
flush_work(&kgdboc_restore_input_work);
}
#else /* ! CONFIG_KDB_KEYBOARD */
---
base-commit: 0bbac3facb5d6cc0171c45c9873a2dc96bea9680
change-id: 20240419-kgdboc_fix_schedule_work-f0cb44b8a354
Best regards,
--
Daniel Thompson <[email protected]>
Hi,
On Fri, Apr 19, 2024 at 3:30 AM Daniel Thompson
<[email protected]> 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 <[email protected]>
> Closes: https://lore.kernel.org/all/[email protected]/
> Cc: [email protected]
> Signed-off-by: Daniel Thompson <[email protected]>
> ---
> 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 <[email protected]>