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.
-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 <dg573847...@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
>
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
