| Message ID | 20240419-kgdboc_fix_schedule_work-v1-1-ff19881677e5@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: kgdboc: Fix NMI-safety problems from keyboard reset code | expand |
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>
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 */
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(-) --- base-commit: 0bbac3facb5d6cc0171c45c9873a2dc96bea9680 change-id: 20240419-kgdboc_fix_schedule_work-f0cb44b8a354 Best regards,