Messages in this thread Patch in this message | | | Date | Fri, 30 Jun 2023 10:31:02 +0930 | From | "Andrew Jeffery" <> | Subject | Re: [PATCH] ipmi: fix potential deadlock on &kcs_bmc->lock |
| |
Hi Corey, Chengfeng,
On Wed, 28 Jun 2023, at 21:17, Corey Minyard wrote: > Indeed, this looks like an issue. > > Andrew, any opinions on this? The attached patch will work, the other > option would be to disable interrupts when calling > kcs_bmc_handle_event() in the timer handler. But then you have to worry > about RT.
Right, I think we'd do best to not over-complicate things. spin_lock_irq{save,restore}() are the intuitive choice to me.
I'll follow up with R-b/T-b tags once I've booted the patch and done some testing.
Andrew
> > -corey > > On Tue, Jun 27, 2023 at 03:24:49PM +0000, Chengfeng Ye wrote: >> As kcs_bmc_handle_event() is executed inside both a timer and a hardirq, >> it should disable irq before lock acquisition otherwise deadlock could >> happen if the timmer is preemtped by the irq. >> >> Possible deadlock scenario: >> aspeed_kcs_check_obe() (timer) >> -> kcs_bmc_handle_event() >> -> spin_lock(&kcs_bmc->lock) >> <irq interruption> >> -> aspeed_kcs_irq() >> -> kcs_bmc_handle_event() >> -> spin_lock(&kcs_bmc->lock) (deadlock here) >> >> This flaw was found using an experimental static analysis tool we are >> developing for irq-related deadlock. >> >> The tentative patch fix the potential deadlock by spin_lock_irqsave() >> >> Signed-off-by: Chengfeng Ye <dg573847474@gmail.com> >> --- >> drivers/char/ipmi/kcs_bmc.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c >> index 03d02a848f3a..8b1161d5194a 100644 >> --- a/drivers/char/ipmi/kcs_bmc.c >> +++ b/drivers/char/ipmi/kcs_bmc.c >> @@ -56,12 +56,13 @@ irqreturn_t kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc) >> { >> struct kcs_bmc_client *client; >> irqreturn_t rc = IRQ_NONE; >> + unsigned long flags; >> >> - spin_lock(&kcs_bmc->lock); >> + spin_lock_irqsave(&kcs_bmc->lock, flags); >> client = kcs_bmc->client; >> if (client) >> rc = client->ops->event(client); >> - spin_unlock(&kcs_bmc->lock); >> + spin_unlock_irqrestore(&kcs_bmc->lock, flags); >> >> return rc; >> } >> -- >> 2.17.1 >>
| |