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

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

[Chengfeng Ye]

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 <dg573847474@gmail.com>
---
 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

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

[Jakub Kicinski]

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 <dg573847474@gmail.com>

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

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

[Chengfeng Ye]

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

Best Regards,
Chengfeng