As &card->cli_queue_lock is acquired under softirq context along the
following call chain from solos_bh(), other acquisition of the same
lock inside process context should disable at least bh to avoid double
lock.
<deadlock #1>
console_show()
--> spin_lock(&card->cli_queue_lock)
<interrupt>
--> solos_bh()
--> spin_lock(&card->cli_queue_lock)
This flaw was found by an experimental static analysis tool I am
developing for irq-related deadlock.
To prevent the potential deadlock, the patch uses spin_lock_irqsave()
on the card->cli_queue_lock under process context code consistently
to prevent the possible deadlock scenario.
Fixes: 9c54004ea717 ("atm: Driver for Solos PCI ADSL2+ card.")
Signed-off-by: Chengfeng Ye <[email protected]>
---
V2: add fix tag, and slipt into two patches
drivers/atm/solos-pci.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/atm/solos-pci.c b/drivers/atm/solos-pci.c
index 94fbc3abe60e..48cf9b36b61a 100644
--- a/drivers/atm/solos-pci.c
+++ b/drivers/atm/solos-pci.c
@@ -447,11 +447,12 @@ static ssize_t console_show(struct device *dev, struct device_attribute *attr,
struct atm_dev *atmdev = container_of(dev, struct atm_dev, class_dev);
struct solos_card *card = atmdev->dev_data;
struct sk_buff *skb;
+ unsigned long flags;
unsigned int len;
- spin_lock(&card->cli_queue_lock);
+ spin_lock_irqsave(&card->cli_queue_lock, flags);
skb = skb_dequeue(&card->cli_queue[SOLOS_CHAN(atmdev)]);
- spin_unlock(&card->cli_queue_lock);
+ spin_unlock_irqrestore(&card->cli_queue_lock, flags);
if(skb == NULL)
return sprintf(buf, "No data.\n");
--
2.17.1
On Thu, 5 Oct 2023 07:48:58 +0000 Chengfeng Ye wrote:
> As &card->cli_queue_lock is acquired under softirq context along the
you say softirq here
> following call chain from solos_bh(), other acquisition of the same
> lock inside process context should disable at least bh to avoid double
> lock.
>
> <deadlock #1>
> console_show()
> --> spin_lock(&card->cli_queue_lock)
> <interrupt>
> --> solos_bh()
> --> spin_lock(&card->cli_queue_lock)
>
> This flaw was found by an experimental static analysis tool I am
> developing for irq-related deadlock.
>
> To prevent the potential deadlock, the patch uses spin_lock_irqsave()
> on the card->cli_queue_lock under process context code consistently
> to prevent the possible deadlock scenario.
and irqsave here. I think you're right that it's just softirq (== bh)
that may deadlock, so no need to take the irqsave() version in process
context.
--
pw-bot: cr
Hi Jakub,
> and irqsave here. I think you're right that it's just softirq (== bh)
> that may deadlock, so no need to take the irqsave() version in process
> context.
Yes, spin_lock_bh() is enough.
I just found spin_lock_irqsave() is more frequently used in this file, so I
also used spin_lock_irqsave() here for uniformity consideration at that time.
Should I send a new patch series to change this to spin_lock_bh()? That's
better for performance consideration.
Thanks,
Chengfeng
On Sat, 7 Oct 2023 23:58:36 +0800 Chengfeng Ye wrote:
> > and irqsave here. I think you're right that it's just softirq (== bh)
> > that may deadlock, so no need to take the irqsave() version in process
> > context.
>
> Yes, spin_lock_bh() is enough.
>
> I just found spin_lock_irqsave() is more frequently used in this file, so I
> also used spin_lock_irqsave() here for uniformity consideration at that time.
>
> Should I send a new patch series to change this to spin_lock_bh()? That's
> better for performance consideration.
Yes, performance is one reason and another is that the code will
be easier to understand if the locking matches the requirements.
Dear Maintainers,
Sorry for the late reply after such a long time. I have just sent the v3 patch
to change spin_lock_irqsave() to spin_lock_bh().
Thanks much for your effort in reviewing!
Chengfeng