2023-07-25 17:50:30

by Chengfeng Ye

[permalink] [raw]
Subject: [PATCH] mISDN: hfcpci: Fix potential deadlock on &hc->lock

As &hc->lock is acquired by both timer _hfcpci_softirq() and hardirq
hfcpci_int(), the timer should disable irq before lock acquisition
otherwise deadlock could happen if the timmer is preemtped by the hadr irq.

Possible deadlock scenario:
hfcpci_softirq() (timer)
-> _hfcpci_softirq()
-> spin_lock(&hc->lock);
<irq interruption>
-> hfcpci_int()
-> spin_lock(&hc->lock); (deadlock here)

This flaw was found by an experimental static analysis tool I am developing
for irq-related deadlock.

The tentative patch fixes the potential deadlock by spin_lock_irq()
in timer.

But the patch could be not enough since between the lock critical section
inside the timer, tx_birq() is called in which a lot of stuff is executed
such as dev_kfree_skb(). I am not sure what's the best way to solve this
potential deadlock problem.

Signed-off-by: Chengfeng Ye <[email protected]>
---
drivers/isdn/hardware/mISDN/hfcpci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/isdn/hardware/mISDN/hfcpci.c b/drivers/isdn/hardware/mISDN/hfcpci.c
index c0331b268010..ca77f259fd93 100644
--- a/drivers/isdn/hardware/mISDN/hfcpci.c
+++ b/drivers/isdn/hardware/mISDN/hfcpci.c
@@ -2277,7 +2277,7 @@ _hfcpci_softirq(struct device *dev, void *unused)
return 0;

if (hc->hw.int_m2 & HFCPCI_IRQ_ENABLE) {
- spin_lock(&hc->lock);
+ spin_lock_irq(&hc->lock);
bch = Sel_BCS(hc, hc->hw.bswapped ? 2 : 1);
if (bch && bch->state == ISDN_P_B_RAW) { /* B1 rx&tx */
main_rec_hfcpci(bch);
@@ -2288,7 +2288,7 @@ _hfcpci_softirq(struct device *dev, void *unused)
main_rec_hfcpci(bch);
tx_birq(bch);
}
- spin_unlock(&hc->lock);
+ spin_unlock_irq(&hc->lock);
}
return 0;
}
--
2.17.1



2023-07-27 05:18:29

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH] mISDN: hfcpci: Fix potential deadlock on &hc->lock

On Tue, 25 Jul 2023 17:37:28 +0000 Chengfeng Ye wrote:
> As &hc->lock is acquired by both timer _hfcpci_softirq() and hardirq
> hfcpci_int(), the timer should disable irq before lock acquisition
> otherwise deadlock could happen if the timmer is preemtped by the hadr irq.
>
> Possible deadlock scenario:
> hfcpci_softirq() (timer)
> -> _hfcpci_softirq()
> -> spin_lock(&hc->lock);
> <irq interruption>
> -> hfcpci_int()
> -> spin_lock(&hc->lock); (deadlock here)
>
> This flaw was found by an experimental static analysis tool I am developing
> for irq-related deadlock.
>
> The tentative patch fixes the potential deadlock by spin_lock_irq()
> in timer.
>
> But the patch could be not enough since between the lock critical section
> inside the timer, tx_birq() is called in which a lot of stuff is executed
> such as dev_kfree_skb(). I am not sure what's the best way to solve this
> potential deadlock problem.

Yeah, the dev_kfree_skb() should be dev_kfree_skb_any() or _irq()

> Signed-off-by: Chengfeng Ye <[email protected]>

LGTM, but please repost with a Fixes tag added.
--
pw-bot: cr

2023-07-27 09:26:03

by Chengfeng Ye

[permalink] [raw]
Subject: Re: [PATCH] mISDN: hfcpci: Fix potential deadlock on &hc->lock

Thanks. v2 patch is just sent to address the problem.

Best Regards,
Chengfeng