handle_receive_interrupt_napi_sp() running inside interrupt handler
could introduce inverse lock ordering between &dd->irq_src_lock
and &dd->uctxt_lock, if read_mod_write() is preempted by the isr.
[CPU0] | [CPU1]
hfi1_ipoib_dev_open() |
--> hfi1_netdev_enable_queues() |
--> enable_queues(rx) |
--> hfi1_rcvctrl() |
--> set_intr_bits() |
--> read_mod_write() |
--> spin_lock(&dd->irq_src_lock) |
| hfi1_poll()
| --> poll_next()
| --> spin_lock_irq(&dd->uctxt_lock)
|
| --> hfi1_rcvctrl()
| --> set_intr_bits()
| --> read_mod_write()
| --> spin_lock(&dd->irq_src_lock)
<interrupt> |
--> handle_receive_interrupt_napi_sp() |
--> set_all_fastpath() |
--> hfi1_rcd_get_by_index() |
--> spin_lock_irqsave(&dd->uctxt_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 use spin_lock_irqsave()
on &dd->irq_src_lock inside read_mod_write() to prevent the possible
deadlock scenario.
Signed-off-by: Chengfeng Ye <[email protected]>
---
drivers/infiniband/hw/hfi1/chip.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/hw/hfi1/chip.c b/drivers/infiniband/hw/hfi1/chip.c
index 0814291a0412..9b542f7c6c11 100644
--- a/drivers/infiniband/hw/hfi1/chip.c
+++ b/drivers/infiniband/hw/hfi1/chip.c
@@ -13185,15 +13185,16 @@ static void read_mod_write(struct hfi1_devdata *dd, u16 src, u64 bits,
{
u64 reg;
u16 idx = src / BITS_PER_REGISTER;
+ unsigned long flags;
- spin_lock(&dd->irq_src_lock);
+ spin_lock_irqsave(&dd->irq_src_lock, flags);
reg = read_csr(dd, CCE_INT_MASK + (8 * idx));
if (set)
reg |= bits;
else
reg &= ~bits;
write_csr(dd, CCE_INT_MASK + (8 * idx), reg);
- spin_unlock(&dd->irq_src_lock);
+ spin_unlock_irqrestore(&dd->irq_src_lock, flags);
}
/**
--
2.17.1
On Tue, Sep 26, 2023 at 10:11:16AM +0000, Chengfeng Ye wrote:
> handle_receive_interrupt_napi_sp() running inside interrupt handler
> could introduce inverse lock ordering between &dd->irq_src_lock
> and &dd->uctxt_lock, if read_mod_write() is preempted by the isr.
>
> [CPU0] | [CPU1]
> hfi1_ipoib_dev_open() |
> --> hfi1_netdev_enable_queues() |
> --> enable_queues(rx) |
> --> hfi1_rcvctrl() |
> --> set_intr_bits() |
> --> read_mod_write() |
> --> spin_lock(&dd->irq_src_lock) |
> | hfi1_poll()
> | --> poll_next()
> | --> spin_lock_irq(&dd->uctxt_lock)
> |
> | --> hfi1_rcvctrl()
> | --> set_intr_bits()
> | --> read_mod_write()
> | --> spin_lock(&dd->irq_src_lock)
> <interrupt> |
> --> handle_receive_interrupt_napi_sp() |
> --> set_all_fastpath() |
> --> hfi1_rcd_get_by_index() |
> --> spin_lock_irqsave(&dd->uctxt_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 use spin_lock_irqsave()
> on &dd->irq_src_lock inside read_mod_write() to prevent the possible
> deadlock scenario.
>
> Signed-off-by: Chengfeng Ye <[email protected]>
> ---
> drivers/infiniband/hw/hfi1/chip.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Dennis? This needs your ack/nack
Thanks,
Jason
On 10/23/23 12:01 PM, Jason Gunthorpe wrote:
> On Tue, Sep 26, 2023 at 10:11:16AM +0000, Chengfeng Ye wrote:
>> handle_receive_interrupt_napi_sp() running inside interrupt handler
>> could introduce inverse lock ordering between &dd->irq_src_lock
>> and &dd->uctxt_lock, if read_mod_write() is preempted by the isr.
>>
>> [CPU0] | [CPU1]
>> hfi1_ipoib_dev_open() |
>> --> hfi1_netdev_enable_queues() |
>> --> enable_queues(rx) |
>> --> hfi1_rcvctrl() |
>> --> set_intr_bits() |
>> --> read_mod_write() |
>> --> spin_lock(&dd->irq_src_lock) |
>> | hfi1_poll()
>> | --> poll_next()
>> | --> spin_lock_irq(&dd->uctxt_lock)
>> |
>> | --> hfi1_rcvctrl()
>> | --> set_intr_bits()
>> | --> read_mod_write()
>> | --> spin_lock(&dd->irq_src_lock)
>> <interrupt> |
>> --> handle_receive_interrupt_napi_sp() |
>> --> set_all_fastpath() |
>> --> hfi1_rcd_get_by_index() |
>> --> spin_lock_irqsave(&dd->uctxt_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 use spin_lock_irqsave()
>> on &dd->irq_src_lock inside read_mod_write() to prevent the possible
>> deadlock scenario.
>>
>> Signed-off-by: Chengfeng Ye <[email protected]>
>> ---
>> drivers/infiniband/hw/hfi1/chip.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> Dennis? This needs your ack/nack
Looks like we need to disable the interrupt. Sorry for the delay.
Acked-by: Dennis Dalessandro <[email protected]>
On Tue, 26 Sep 2023 10:11:16 +0000, Chengfeng Ye wrote:
> handle_receive_interrupt_napi_sp() running inside interrupt handler
> could introduce inverse lock ordering between &dd->irq_src_lock
> and &dd->uctxt_lock, if read_mod_write() is preempted by the isr.
>
> [CPU0] | [CPU1]
> hfi1_ipoib_dev_open() |
> --> hfi1_netdev_enable_queues() |
> --> enable_queues(rx) |
> --> hfi1_rcvctrl() |
> --> set_intr_bits() |
> --> read_mod_write() |
> --> spin_lock(&dd->irq_src_lock) |
> | hfi1_poll()
> | --> poll_next()
> | --> spin_lock_irq(&dd->uctxt_lock)
> |
> | --> hfi1_rcvctrl()
> | --> set_intr_bits()
> | --> read_mod_write()
> | --> spin_lock(&dd->irq_src_lock)
> <interrupt> |
> --> handle_receive_interrupt_napi_sp() |
> --> set_all_fastpath() |
> --> hfi1_rcd_get_by_index() |
> --> spin_lock_irqsave(&dd->uctxt_lock) |
>
> [...]
Applied, thanks!
[1/1] IB/hfi1: Fix potential deadlock on &irq_src_lock and &dd->uctxt_lock
https://git.kernel.org/rdma/rdma/c/2f19c4b8395ccb
Best regards,
--
Leon Romanovsky <[email protected]>