| Message ID | 20230926101116.2797-1-dg573847474@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | IB/hfi1: Fix potential deadlock on &irq_src_lock and &dd->uctxt_lock | expand |
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 <dg573847474@gmail.com> > --- > 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 <dg573847474@gmail.com> >> --- >> 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 <dennis.dalessandro@cornelisnetworks.com>
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,
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); } /**
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 <dg573847474@gmail.com> --- drivers/infiniband/hw/hfi1/chip.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)