| Message ID | 20240409020326.2125332-1-liu.yec@h3c.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V9] kdb: Fix the deadlock issue in KDB debugging. | expand |
On Wed, Apr 10, 2024 at 5:07 AM <liu.yec@h3c.com> wrote: > > From: LiuYe <liu.yeC@h3c.com> > > Currently, if CONFIG_KDB_KEYBOARD is enabled, then kgdboc will > attempt to use schedule_work() to provoke a keyboard reset when > transitioning out of the debugger and back to normal operation. > This can cause deadlock because schedule_work() is not NMI-safe. > > The stack trace below shows an example of the problem. In this > case the master cpu is not running from NMI but it has parked > the slave CPUs using an NMI and the parked CPUs is holding > spinlocks needed by schedule_work(). > > Example: > BUG: spinlock lockup suspected on CPU#0. owner_cpu: 1 > CPU1: Call Trace: > __schedule > schedule > schedule_hrtimeout_range_clock > mutex_unlock > ep_scan_ready_list > schedule_hrtimeout_range > ep_poll > wake_up_q > SyS_epoll_wait > entry_SYSCALL_64_fastpath > > CPU0: Call Trace: > dump_stack > spin_dump > do_raw_spin_lock > _raw_spin_lock > try_to_wake_up > wake_up_process > insert_work > __queue_work > queue_work_on > kgdboc_post_exp_handler > kgdb_cpu_enter > kgdb_handle_exception > __kgdb_notify > kgdb_notify > notifier_call_chain > notify_die > do_int3 > int3 > > We fix the problem by using irq_work to call schedule_work() > instead of calling it directly. This is because we cannot > resynchronize the keyboard state from the hardirq context > provided by irq_work. This must be done from the task context > in order to call the input subsystem. > > Therefore, we have to defer the work twice. First, safely > switch from the debug trap context (similar to NMI) to the > hardirq, and then switch from the hardirq to the system work queue. ... > Signed-off-by: Greg KH <gregkh@linuxfoundation.org> > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > V9 -> V10 : Add Signed-off-by of Greg KH and Andy Shevchenko, Acked-by of Daniel Thompson Huh?!
>> Signed-off-by: Greg KH <gregkh@linuxfoundation.org> > >I have NOT signed off on this commit. You just said that I made a legal statement about this commit without that actually being true??? > >Sorry, but that is flat out not acceptable at all. Please go work with your company lawyers to figure out what you did and come back with an explaination of exactly what this is and how it will not happen again. > >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> V9 -> V10 : Add Signed-off-by of Greg KH and Andy Shevchenko, Acked-by >> of Daniel Thompson > >Huh?! @greg k-h ： @Andy Shevchenko ： Sorry, it was my mistake. I misunderstood the meaning of "signed-off-by", which led to usage issues. I want to express my gratitude for the suggestions on the patch from the two of you. What do I need to do now? Release PATCH V11 and delete these two signatures in it ?
On Wed, Apr 10, 2024 at 05:54:08AM +0000, Liuye wrote: > >> Signed-off-by: Greg KH <gregkh@linuxfoundation.org> > > > >I have NOT signed off on this commit. You just said that I made a legal statement about this commit without that actually being true??? > > > >Sorry, but that is flat out not acceptable at all. Please go work with your company lawyers to figure out what you did and come back with an explaination of exactly what this is and how it will not happen again. > > > > >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > >> V9 -> V10 : Add Signed-off-by of Greg KH and Andy Shevchenko, Acked-by > >> of Daniel Thompson > > > >Huh?! > > @greg k-h ： > @Andy Shevchenko ： > > Sorry, it was my mistake. I misunderstood the meaning of "signed-off-by", which led to usage issues. > > I want to express my gratitude for the suggestions on the patch from the two of you. > > What do I need to do now? Release PATCH V11 and delete these two signatures in it ? As I said, go work with your corporate lawyers on this to understand what just happened and have them let us know how it will not happen again. thanks, greg k-h
>On Wed, Apr 10, 2024 at 05:54:08AM +0000, Liuye wrote: >> >> Signed-off-by: Greg KH <gregkh@linuxfoundation.org> >> > >> >I have NOT signed off on this commit. You just said that I made a legal statement about this commit without that actually being true??? >> > >> >Sorry, but that is flat out not acceptable at all. Please go work with your company lawyers to figure out what you did and come back with an explaination of exactly what this is and how it will not happen again. >> > >> >> >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> > >> >> V9 -> V10 : Add Signed-off-by of Greg KH and Andy Shevchenko, >> >> Acked-by of Daniel Thompson >> > >> >Huh?! >> >> @greg k-h ： >> @Andy Shevchenko ： >> >> Sorry, it was my mistake. I misunderstood the meaning of "signed-off-by", which led to usage issues. >> >> I want to express my gratitude for the suggestions on the patch from the two of you. >> >> What do I need to do now? Release PATCH V11 and delete these two signatures in it ? > >As I said, go work with your corporate lawyers on this to understand what just happened and have them let us know how it will not happen again. I'm very sorry, this is my first time submitting a patch and I made a significant mistake in using "Signed-off-by". I now understand the meaning of this field and will not make the same mistake again in the future. Thanks， Liu Ye
On Wed, Apr 10, 2024 at 06:10:10AM +0000, Liuye wrote: > >On Wed, Apr 10, 2024 at 05:54:08AM +0000, Liuye wrote: > >> >> Signed-off-by: Greg KH <gregkh@linuxfoundation.org> > >> > > >> >I have NOT signed off on this commit. You just said that I made a legal statement about this commit without that actually being true??? > >> > > >> >Sorry, but that is flat out not acceptable at all. Please go work with your company lawyers to figure out what you did and come back with an explaination of exactly what this is and how it will not happen again. > >> > > >> > >> >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> > > >> >> V9 -> V10 : Add Signed-off-by of Greg KH and Andy Shevchenko, > >> >> Acked-by of Daniel Thompson > >> > > >> >Huh?! > >> > >> @greg k-h ： > >> @Andy Shevchenko ： > >> > >> Sorry, it was my mistake. I misunderstood the meaning of "signed-off-by", which led to usage issues. > >> > >> I want to express my gratitude for the suggestions on the patch from the two of you. > >> > >> What do I need to do now? Release PATCH V11 and delete these two signatures in it ? > > > >As I said, go work with your corporate lawyers on this to understand what just happened and have them let us know how it will not happen again. > > I'm very sorry, this is my first time submitting a patch and I made a significant mistake in using "Signed-off-by". I now understand the meaning of this field and will not make the same mistake again in the future. Understood, but you still need to go work with your corporate legal group so that you all ensure this does not happen again for any other developer in your company, as I am sure they will want to know about this. thanks, greg k-h
>On Wed, Apr 10, 2024 at 06:10:10AM +0000, Liuye wrote: >> >On Wed, Apr 10, 2024 at 05:54:08AM +0000, Liuye wrote: >> >> >> Signed-off-by: Greg KH <gregkh@linuxfoundation.org> >> >> > >> >> >I have NOT signed off on this commit. You just said that I made a legal statement about this commit without that actually being true??? >> >> > >> >> >Sorry, but that is flat out not acceptable at all. Please go work with your company lawyers to figure out what you did and come back with an explaination of exactly what this is and how it will not happen again. >> >> > >> >> >> >> >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> >> > >> >> >> V9 -> V10 : Add Signed-off-by of Greg KH and Andy Shevchenko, >> >> >> Acked-by of Daniel Thompson >> >> > >> >> >Huh?! >> >> >> >> @greg k-h ： >> >> @Andy Shevchenko ： >> >> >> >> Sorry, it was my mistake. I misunderstood the meaning of "signed-off-by", which led to usage issues. >> >> >> >> I want to express my gratitude for the suggestions on the patch from the two of you. >> >> >> >> What do I need to do now? Release PATCH V11 and delete these two signatures in it ? >> > >> >As I said, go work with your corporate lawyers on this to understand what just happened and have them let us know how it will not happen again. >> >> I'm very sorry, this is my first time submitting a patch and I made a significant mistake in using "Signed-off-by". I now understand the meaning of this field and will not make the same mistake again in the future. > >Understood, but you still need to go work with your corporate legal group so that you all ensure this does not happen again for any other developer in your company, as I am sure they will want to know about this. Ok, I will report this to the company. At the same time, I will add an audit mechanism to the patch submission process. Thanks again for your reminder. I will remove this part in PATCH V11. Thanks， Liu Ye
On Wed, Apr 10, 2024 at 06:30:59AM +0000, Liuye wrote: > >On Wed, Apr 10, 2024 at 06:10:10AM +0000, Liuye wrote: > >> >On Wed, Apr 10, 2024 at 05:54:08AM +0000, Liuye wrote: > >> >> >> Signed-off-by: Greg KH <gregkh@linuxfoundation.org> > >> >> > > >> >> >I have NOT signed off on this commit. You just said that I made a legal statement about this commit without that actually being true??? > >> >> > > >> >> >Sorry, but that is flat out not acceptable at all. Please go work with your company lawyers to figure out what you did and come back with an explaination of exactly what this is and how it will not happen again. > >> >> > > >> >> > >> >> >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > >> >> > > >> >> >> V9 -> V10 : Add Signed-off-by of Greg KH and Andy Shevchenko, > >> >> >> Acked-by of Daniel Thompson > >> >> > > >> >> >Huh?! > >> >> > >> >> @greg k-h ： > >> >> @Andy Shevchenko ： > >> >> > >> >> Sorry, it was my mistake. I misunderstood the meaning of "signed-off-by", which led to usage issues. > >> >> > >> >> I want to express my gratitude for the suggestions on the patch from the two of you. > >> >> > >> >> What do I need to do now? Release PATCH V11 and delete these two signatures in it ? > >> > > >> >As I said, go work with your corporate lawyers on this to understand what just happened and have them let us know how it will not happen again. > >> > >> I'm very sorry, this is my first time submitting a patch and I made a significant mistake in using "Signed-off-by". I now understand the meaning of this field and will not make the same mistake again in the future. > > > >Understood, but you still need to go work with your corporate legal group so that you all ensure this does not happen again for any other developer in your company, as I am sure they will want to know about this. > > Ok, I will report this to the company. At the same time, I will add an audit mechanism to the patch submission process. Thanks again for your reminder. > > I will remove this part in PATCH V11. No, you will need to do this before we can accept your change. And some sort of verification that this is now in place properly for obvious reasons. thanks, greg k-h
>On Wed, Apr 10, 2024 at 06:30:59AM +0000, Liuye wrote: >> >On Wed, Apr 10, 2024 at 06:10:10AM +0000, Liuye wrote: >> >> >On Wed, Apr 10, 2024 at 05:54:08AM +0000, Liuye wrote: >> >> >> >> Signed-off-by: Greg KH <gregkh@linuxfoundation.org> >> >> >> > >> >> >> >I have NOT signed off on this commit. You just said that I made a legal statement about this commit without that actually being true??? >> >> >> > >> >> >> >Sorry, but that is flat out not acceptable at all. Please go work with your company lawyers to figure out what you did and come back with an explaination of exactly what this is and how it will not happen again. >> >> >> > >> >> >> >> >> >> >> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> >> >> >> > >> >> >> >> V9 -> V10 : Add Signed-off-by of Greg KH and Andy Shevchenko, >> >> >> >> Acked-by of Daniel Thompson >> >> >> > >> >> >> >Huh?! >> >> >> >> >> >> @greg k-h ： >> >> >> @Andy Shevchenko ： >> >> >> >> >> >> Sorry, it was my mistake. I misunderstood the meaning of "signed-off-by", which led to usage issues. >> >> >> >> >> >> I want to express my gratitude for the suggestions on the patch from the two of you. >> >> >> >> >> >> What do I need to do now? Release PATCH V11 and delete these two signatures in it ? >> >> > >> >> >As I said, go work with your corporate lawyers on this to understand what just happened and have them let us know how it will not happen again. >> >> >> >> I'm very sorry, this is my first time submitting a patch and I made a significant mistake in using "Signed-off-by". I now understand the meaning of this field and will not make the same mistake again in the future. >> > >> >Understood, but you still need to go work with your corporate legal group so that you all ensure this does not happen again for any other developer in your company, as I am sure they will want to know about this. >> >> Ok, I will report this to the company. At the same time, I will add an audit mechanism to the patch submission process. Thanks again for your reminder. >> >> I will remove this part in PATCH V11. > >No, you will need to do this before we can accept your change. And some sort of verification that this is now in place properly for obvious reasons. What does "No" mean? Are you talking about giving feedback to the company to prevent this incident from happening? Or submitting PATCH V11? If it's the former, how should I give you feedback?"
diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c index 7ce7bb164..32410fec7 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> @@ -82,6 +83,19 @@ static struct input_handler kgdboc_reset_handler = { static DEFINE_MUTEX(kgdboc_reset_mutex); +/* + * This code ensures that the keyboard state, which is changed during kdb + * execution, is resynchronized when we leave the debug trap. The resync + * logic calls into the input subsystem to force a reset. The calls into + * the input subsystem must be executed from normal task context. + * + * We need to trigger the resync from the debug trap, which executes in an + * NMI (or similar) context. To make it safe to call into the input + * subsystem we end up having use two deferred execution techniques. + * Firstly, we use irq_work, which is NMI-safe, to provoke a callback from + * hardirq context. Then, from the hardirq callback we use the system + * workqueue to provoke the callback that actually performs the resync. + */ static void kgdboc_restore_input_helper(struct work_struct *dummy) { /* @@ -99,10 +113,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 +154,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 */