Re: Deadlock during PCIe hot remove and SPDK exit

Re: Deadlock during PCIe hot remove and SPDK exit

[fengnan chang]

> 2024年11月27日 14:56，fengnan chang <fengnanchang@gmail.com> 写道：
> 
> Dear PCI maintainers:
>   I'm having a deadlock issue, somewhat similar to a previous one https://lore.kernel.org/linux-pci/CS1PR8401MB0728FC6FDAB8A35C22BD90EC95F10@CS1PR8401MB0728.NAMPRD84.PROD.OUTLOOK.COM/#t， but my kernel (6.6.40) already included the fix f5eff55. 
>   Here is my test process, I’m running kernel with 6.6.40 and SPDK v22.05:
>   1. SPDK use vfio driver to takeover two nvme disks, running some io in nvme.
>   2. pull out two nvme disks
>   3. Try to kill -9 SPDK process.
>   Then deadlock issue happened. For now I can 100% reproduce this problem. I’m not an export in PCI, but I did a brief analysis:
>   irq 149 thread take pci_rescan_remove_lock mutex lock, and wait for SPDK to release vfio.
>   irq 148 thread take reset_lock of ctrl A, and wait for psi_rescan_remove_lock
>   SPDK process try to release vfio driver, but wait for reset_lock of ctrl A.
> 
> 
> irq/149-pciehp stack, cat /proc/514/stack, 
> [<0>] pciehp_unconfigure_device+0x48/0x160 // wait for pci_rescan_remove_lock
> [<0>] pciehp_disable_slot+0x6b/0x130       // hold reset_lock of ctrl A
> [<0>] pciehp_handle_presence_or_link_change+0x7d/0x4d0
> [<0>] pciehp_ist+0x236/0x260
> [<0>] irq_thread_fn+0x1b/0x60
> [<0>] irq_thread+0xed/0x190
> [<0>] kthread+0xe4/0x110
> [<0>] ret_from_fork+0x2d/0x50
> [<0>] ret_from_fork_asm+0x11/0x20
> 
> 
> irq/148-pciehp stack, cat /proc/513/stack
> [<0>] vfio_unregister_group_dev+0x97/0xe0 [vfio]     //wait for

My mistake, this is wait for SPDK to release vfio device. This problem can reproduce in 6.12.
Besides, My college give me an idea, we can make vfio_device_fops_release be async, so when we close fd, it
won’t block, and when we close another fd, it will release  vfio device, this stack will not block too, then the deadlock disappears.

Here is my test patch, cc some vfio guys:
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index 50128da18bca..4ebe154a4ae5 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -19,6 +19,7 @@ struct vfio_container;
 struct vfio_device_file {
        struct vfio_device *device;
        struct vfio_group *group;
+       struct work_struct      release_work;

        u8 access_granted;
        u32 devid; /* only valid when iommufd is valid */
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index a5a62d9d963f..47e3e3f73d70 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -487,6 +487,22 @@ static bool vfio_assert_device_open(struct vfio_device *device)
        return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
 }

+static void vfio_fops_release_work(struct work_struct *work)
+{
+       struct vfio_device_file *df =
+               container_of(work, struct vfio_device_file, release_work);
+       struct vfio_device *device = df->device;
+
+       if (df->group)
+               vfio_df_group_close(df);
+       else
+               vfio_df_unbind_iommufd(df);
+
+       vfio_device_put_registration(device);
+
+       kfree(df);
+}
+
 struct vfio_device_file *
 vfio_allocate_device_file(struct vfio_device *device)
 {
@@ -497,6 +513,7 @@ vfio_allocate_device_file(struct vfio_device *device)
                return ERR_PTR(-ENOMEM);

        df->device = device;
+       INIT_WORK(&df->release_work, vfio_fops_release_work);
        spin_lock_init(&df->kvm_ref_lock);

        return df;
@@ -628,16 +645,8 @@ static inline void vfio_device_pm_runtime_put(struct vfio_device *device)
 static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 {
        struct vfio_device_file *df = filep->private_data;
-       struct vfio_device *device = df->device;

-       if (df->group)
-               vfio_df_group_close(df);
-       else
-               vfio_df_unbind_iommufd(df);
-
-       vfio_device_put_registration(device);
-
-       kfree(df);
+       schedule_work(&df->release_work);

        return 0;
 }


> [<0>] vfio_pci_core_unregister_device+0x19/0x80 [vfio_pci_core]
> [<0>] vfio_pci_remove+0x15/0x20 [vfio_pci]
> [<0>] pci_device_remove+0x39/0xb0
> [<0>] device_release_driver_internal+0xad/0x120
> [<0>] pci_stop_bus_device+0x5d/0x80
> [<0>] pci_stop_and_remove_bus_device+0xe/0x20
> [<0>] pciehp_unconfigure_device+0x91/0x160   //hold pci_rescan_remove_lock, release reset_lock of ctrl B 
> [<0>] pciehp_disable_slot+0x6b/0x130
> [<0>] pciehp_handle_presence_or_link_change+0x7d/0x4d0
> [<0>] pciehp_ist+0x236/0x260             //hold reset_lock of ctrl B 
> [<0>] irq_thread_fn+0x1b/0x60
> [<0>] irq_thread+0xed/0x190
> [<0>] kthread+0xe4/0x110
> [<0>] ret_from_fork+0x2d/0x50
> [<0>] ret_from_fork_asm+0x11/0x20
> 
> 
> SPDK stack, cat /proc/166634/task/167181/stack
> [<0>] down_write_nested+0x1b7/0x1c0            //wait for reset_lock of ctrl A.
> [<0>] pciehp_reset_slot+0x58/0x160
> [<0>] pci_reset_hotplug_slot+0x3b/0x60
> [<0>] pci_reset_bus_function+0x3b/0xb0
> [<0>] __pci_reset_function_locked+0x3e/0x60
> [<0>] vfio_pci_core_disable+0x3ce/0x400 [vfio_pci_core]
> [<0>] vfio_pci_core_close_device+0x67/0xc0 [vfio_pci_core]
> [<0>] vfio_df_close+0x79/0xd0 [vfio]
> [<0>] vfio_df_group_close+0x36/0x70 [vfio]
> [<0>] vfio_device_fops_release+0x20/0x40 [vfio]
> [<0>] __fput+0xec/0x290
> [<0>] task_work_run+0x61/0x90
> [<0>] do_exit+0x39c/0xc40
> [<0>] do_group_exit+0x33/0xa0
> [<0>] get_signal+0xd84/0xd90
> [<0>] arch_do_signal_or_restart+0x2a/0x260
> [<0>] exit_to_user_mode_prepare+0x1c7/0x240
> [<0>] syscall_exit_to_user_mode+0x2a/0x60
> [<0>] do_syscall_64+0x3e/0x90
> 

Re: Deadlock during PCIe hot remove and SPDK exit

[Yi Liu]

On 2024/11/28 11:50, fengnan chang wrote:
> 
> 
>> 2024年11月27日 14:56，fengnan chang <fengnanchang@gmail.com> 写道：
>>
>> Dear PCI maintainers:
>>    I'm having a deadlock issue, somewhat similar to a previous one https://lore.kernel.org/linux-pci/CS1PR8401MB0728FC6FDAB8A35C22BD90EC95F10@CS1PR8401MB0728.NAMPRD84.PROD.OUTLOOK.COM/#t， but my kernel (6.6.40) already included the fix f5eff55.

The previous bug was solved by the below commit.

commit f5eff5591b8f9c5effd25c92c758a127765f74c1
Author: Lukas Wunner <lukas@wunner.de>
Date:   Tue Apr 11 08:21:02 2023 +0200

     PCI: pciehp: Fix AB-BA deadlock between reset_lock and device_lock

     In 2013, commits

       2e35afaefe64 ("PCI: pciehp: Add reset_slot() method")
       608c388122c7 ("PCI: Add slot reset option to pci_dev_reset()")

     amended PCIe hotplug to mask Presence Detect Changed events during a
     Secondary Bus Reset.  The reset thus no longer causes gratuitous slot
     bringdown and bringup.

     However the commits neglected to serialize reset with code paths reading
     slot registers.  For instance, a slot bringup due to an earlier hotplug
     event may see the Presence Detect State bit cleared during a concurrent
     Secondary Bus Reset.

     In 2018, commit

       5b3f7b7d062b ("PCI: pciehp: Avoid slot access during reset")

     retrofitted the missing locking.  It introduced a reset_lock which
     serializes a Secondary Bus Reset with other parts of pciehp.

     Unfortunately the locking turns out to be overzealous:  reset_lock is
     held for the entire enumeration and de-enumeration of hotplugged devices,
     including driver binding and unbinding.

     Driver binding and unbinding acquires device_lock while the reset_lock
     of the ancestral hotplug port is held.  A concurrent Secondary Bus Reset
     acquires the ancestral reset_lock while already holding the device_lock.
     The asymmetric locking order in the two code paths can lead to AB-BA
     deadlocks.

     Michael Haeuptle reports such deadlocks on simultaneous hot-removal and
     vfio release (the latter implies a Secondary Bus Reset):

       pciehp_ist()                                    # down_read(reset_lock)
         pciehp_handle_presence_or_link_change()
           pciehp_disable_slot()
             __pciehp_disable_slot()
               remove_board()
                 pciehp_unconfigure_device()
                   pci_stop_and_remove_bus_device()
                     pci_stop_bus_device()
                       pci_stop_dev()
                         device_release_driver()
                           device_release_driver_internal()
                             __device_driver_lock()    # device_lock()

       SYS_munmap()
         vfio_device_fops_release()
           vfio_device_group_close()
             vfio_device_close()
               vfio_device_last_close()


>>    Here is my test process, I’m running kernel with 6.6.40 and SPDK v22.05:
>>    1. SPDK use vfio driver to takeover two nvme disks, running some io in nvme.
>>    2. pull out two nvme disks
>>    3. Try to kill -9 SPDK process.
>>    Then deadlock issue happened. For now I can 100% reproduce this problem. I’m not an export in PCI, but I did a brief analysis:
>>    irq 149 thread take pci_rescan_remove_lock mutex lock, and wait for SPDK to release vfio.
>>    irq 148 thread take reset_lock of ctrl A, and wait for psi_rescan_remove_lock
>>    SPDK process try to release vfio driver, but wait for reset_lock of ctrl A.
>>
>>
>> irq/149-pciehp stack, cat /proc/514/stack,
>> [<0>] pciehp_unconfigure_device+0x48/0x160 // wait for pci_rescan_remove_lock
>> [<0>] pciehp_disable_slot+0x6b/0x130       // hold reset_lock of ctrl A
>> [<0>] pciehp_handle_presence_or_link_change+0x7d/0x4d0
>> [<0>] pciehp_ist+0x236/0x260
>> [<0>] irq_thread_fn+0x1b/0x60
>> [<0>] irq_thread+0xed/0x190
>> [<0>] kthread+0xe4/0x110
>> [<0>] ret_from_fork+0x2d/0x50
>> [<0>] ret_from_fork_asm+0x11/0x20
>>
>>
>> irq/148-pciehp stack, cat /proc/513/stack
>> [<0>] vfio_unregister_group_dev+0x97/0xe0 [vfio]     //wait for
> 
> My mistake, this is wait for SPDK to release vfio device. This problem can reproduce in 6.12.
> Besides, My college give me an idea, we can make vfio_device_fops_release be async, so when we close fd, it
> won’t block, and when we close another fd, it will release  vfio device, this stack will not block too, then the deadlock disappears.
> 

In the hotplug path, vfio needs to notify userspace to stop the usage of
this device and release reference on the vfio_device. When the last
refcount is released, the wait in the vfio_unregister_group_dev() will be
unblocked. It is the vfio_device_fops_release() either userspace exits or
userspace explicitly close the vfio device fd. Your below test patch moves
the majority of the vfio_device_fops_release() out of the existing path.
I don't see a reason why it can work so far.

As the locking issue has been solved in the above commit, seems there is
no deadlock with the reset_lock and device_lock. Can you confirm if the
scenario can be reproduced with one device? Also, even with two devices,
does killing the process matters or not?

> Here is my test patch, cc some vfio guys:
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index 50128da18bca..4ebe154a4ae5 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -19,6 +19,7 @@ struct vfio_container;
>   struct vfio_device_file {
>          struct vfio_device *device;
>          struct vfio_group *group;
> +       struct work_struct      release_work;
> 
>          u8 access_granted;
>          u32 devid; /* only valid when iommufd is valid */
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index a5a62d9d963f..47e3e3f73d70 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -487,6 +487,22 @@ static bool vfio_assert_device_open(struct vfio_device *device)
>          return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
>   }
> 
> +static void vfio_fops_release_work(struct work_struct *work)
> +{
> +       struct vfio_device_file *df =
> +               container_of(work, struct vfio_device_file, release_work);
> +       struct vfio_device *device = df->device;
> +
> +       if (df->group)
> +               vfio_df_group_close(df);
> +       else
> +               vfio_df_unbind_iommufd(df);
> +
> +       vfio_device_put_registration(device);
> +
> +       kfree(df);
> +}
> +
>   struct vfio_device_file *
>   vfio_allocate_device_file(struct vfio_device *device)
>   {
> @@ -497,6 +513,7 @@ vfio_allocate_device_file(struct vfio_device *device)
>                  return ERR_PTR(-ENOMEM);
> 
>          df->device = device;
> +       INIT_WORK(&df->release_work, vfio_fops_release_work);
>          spin_lock_init(&df->kvm_ref_lock);
> 
>          return df;
> @@ -628,16 +645,8 @@ static inline void vfio_device_pm_runtime_put(struct vfio_device *device)
>   static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>   {
>          struct vfio_device_file *df = filep->private_data;
> -       struct vfio_device *device = df->device;
> 
> -       if (df->group)
> -               vfio_df_group_close(df);
> -       else
> -               vfio_df_unbind_iommufd(df);
> -
> -       vfio_device_put_registration(device);
> -
> -       kfree(df);
> +       schedule_work(&df->release_work);
> 
>          return 0;
>   }
> 
> 
>> [<0>] vfio_pci_core_unregister_device+0x19/0x80 [vfio_pci_core]
>> [<0>] vfio_pci_remove+0x15/0x20 [vfio_pci]
>> [<0>] pci_device_remove+0x39/0xb0
>> [<0>] device_release_driver_internal+0xad/0x120
>> [<0>] pci_stop_bus_device+0x5d/0x80
>> [<0>] pci_stop_and_remove_bus_device+0xe/0x20
>> [<0>] pciehp_unconfigure_device+0x91/0x160   //hold pci_rescan_remove_lock, release reset_lock of ctrl B
>> [<0>] pciehp_disable_slot+0x6b/0x130
>> [<0>] pciehp_handle_presence_or_link_change+0x7d/0x4d0
>> [<0>] pciehp_ist+0x236/0x260             //hold reset_lock of ctrl B
>> [<0>] irq_thread_fn+0x1b/0x60
>> [<0>] irq_thread+0xed/0x190
>> [<0>] kthread+0xe4/0x110
>> [<0>] ret_from_fork+0x2d/0x50
>> [<0>] ret_from_fork_asm+0x11/0x20
>>
>>
>> SPDK stack, cat /proc/166634/task/167181/stack
>> [<0>] down_write_nested+0x1b7/0x1c0            //wait for reset_lock of ctrl A.
>> [<0>] pciehp_reset_slot+0x58/0x160
>> [<0>] pci_reset_hotplug_slot+0x3b/0x60
>> [<0>] pci_reset_bus_function+0x3b/0xb0
>> [<0>] __pci_reset_function_locked+0x3e/0x60
>> [<0>] vfio_pci_core_disable+0x3ce/0x400 [vfio_pci_core]
>> [<0>] vfio_pci_core_close_device+0x67/0xc0 [vfio_pci_core]
>> [<0>] vfio_df_close+0x79/0xd0 [vfio]
>> [<0>] vfio_df_group_close+0x36/0x70 [vfio]
>> [<0>] vfio_device_fops_release+0x20/0x40 [vfio]
>> [<0>] __fput+0xec/0x290
>> [<0>] task_work_run+0x61/0x90
>> [<0>] do_exit+0x39c/0xc40
>> [<0>] do_group_exit+0x33/0xa0
>> [<0>] get_signal+0xd84/0xd90
>> [<0>] arch_do_signal_or_restart+0x2a/0x260
>> [<0>] exit_to_user_mode_prepare+0x1c7/0x240
>> [<0>] syscall_exit_to_user_mode+0x2a/0x60
>> [<0>] do_syscall_64+0x3e/0x90
>>
> 
> 

-- 
Regards,
Yi Liu

Re: Deadlock during PCIe hot remove and SPDK exit

[fengnan chang]

On Fri, Nov 29, 2024 at 4:00 PM Yi Liu <yi.l.liu@intel.com> wrote:
>
> On 2024/11/28 11:50, fengnan chang wrote:
> >
> >
> >> 2024年11月27日 14:56，fengnan chang <fengnanchang@gmail.com> 写道：
> >>
> >> Dear PCI maintainers:
> >>    I'm having a deadlock issue, somewhat similar to a previous one https://lore.kernel.org/linux-pci/CS1PR8401MB0728FC6FDAB8A35C22BD90EC95F10@CS1PR8401MB0728.NAMPRD84.PROD.OUTLOOK.COM/#t， but my kernel (6.6.40) already included the fix f5eff55.
>
> The previous bug was solved by the below commit.
>
> commit f5eff5591b8f9c5effd25c92c758a127765f74c1
> Author: Lukas Wunner <lukas@wunner.de>
> Date:   Tue Apr 11 08:21:02 2023 +0200
>
>      PCI: pciehp: Fix AB-BA deadlock between reset_lock and device_lock
>

Yes, my kernel version included this fix.

>      In 2013, commits
>
>        2e35afaefe64 ("PCI: pciehp: Add reset_slot() method")
>        608c388122c7 ("PCI: Add slot reset option to pci_dev_reset()")
>
>      amended PCIe hotplug to mask Presence Detect Changed events during a
>      Secondary Bus Reset.  The reset thus no longer causes gratuitous slot
>      bringdown and bringup.
>
>      However the commits neglected to serialize reset with code paths reading
>      slot registers.  For instance, a slot bringup due to an earlier hotplug
>      event may see the Presence Detect State bit cleared during a concurrent
>      Secondary Bus Reset.
>
>      In 2018, commit
>
>        5b3f7b7d062b ("PCI: pciehp: Avoid slot access during reset")
>
>      retrofitted the missing locking.  It introduced a reset_lock which
>      serializes a Secondary Bus Reset with other parts of pciehp.
>
>      Unfortunately the locking turns out to be overzealous:  reset_lock is
>      held for the entire enumeration and de-enumeration of hotplugged devices,
>      including driver binding and unbinding.
>
>      Driver binding and unbinding acquires device_lock while the reset_lock
>      of the ancestral hotplug port is held.  A concurrent Secondary Bus Reset
>      acquires the ancestral reset_lock while already holding the device_lock.
>      The asymmetric locking order in the two code paths can lead to AB-BA
>      deadlocks.
>
>      Michael Haeuptle reports such deadlocks on simultaneous hot-removal and
>      vfio release (the latter implies a Secondary Bus Reset):
>
>        pciehp_ist()                                    # down_read(reset_lock)
>          pciehp_handle_presence_or_link_change()
>            pciehp_disable_slot()
>              __pciehp_disable_slot()
>                remove_board()
>                  pciehp_unconfigure_device()
>                    pci_stop_and_remove_bus_device()
>                      pci_stop_bus_device()
>                        pci_stop_dev()
>                          device_release_driver()
>                            device_release_driver_internal()
>                              __device_driver_lock()    # device_lock()
>
>        SYS_munmap()
>          vfio_device_fops_release()
>            vfio_device_group_close()
>              vfio_device_close()
>                vfio_device_last_close()
>
>
> >>    Here is my test process, I’m running kernel with 6.6.40 and SPDK v22.05:
> >>    1. SPDK use vfio driver to takeover two nvme disks, running some io in nvme.
> >>    2. pull out two nvme disks
> >>    3. Try to kill -9 SPDK process.
> >>    Then deadlock issue happened. For now I can 100% reproduce this problem. I’m not an export in PCI, but I did a brief analysis:
> >>    irq 149 thread take pci_rescan_remove_lock mutex lock, and wait for SPDK to release vfio.
> >>    irq 148 thread take reset_lock of ctrl A, and wait for psi_rescan_remove_lock
> >>    SPDK process try to release vfio driver, but wait for reset_lock of ctrl A.
> >>
> >>
> >> irq/149-pciehp stack, cat /proc/514/stack,
> >> [<0>] pciehp_unconfigure_device+0x48/0x160 // wait for pci_rescan_remove_lock
> >> [<0>] pciehp_disable_slot+0x6b/0x130       // hold reset_lock of ctrl A
> >> [<0>] pciehp_handle_presence_or_link_change+0x7d/0x4d0
> >> [<0>] pciehp_ist+0x236/0x260
> >> [<0>] irq_thread_fn+0x1b/0x60
> >> [<0>] irq_thread+0xed/0x190
> >> [<0>] kthread+0xe4/0x110
> >> [<0>] ret_from_fork+0x2d/0x50
> >> [<0>] ret_from_fork_asm+0x11/0x20
> >>
> >>
> >> irq/148-pciehp stack, cat /proc/513/stack
> >> [<0>] vfio_unregister_group_dev+0x97/0xe0 [vfio]     //wait for
> >
> > My mistake, this is wait for SPDK to release vfio device. This problem can reproduce in 6.12.
> > Besides, My college give me an idea, we can make vfio_device_fops_release be async, so when we close fd, it
> > won’t block, and when we close another fd, it will release  vfio device, this stack will not block too, then the deadlock disappears.
> >
>
> In the hotplug path, vfio needs to notify userspace to stop the usage of
> this device and release reference on the vfio_device. When the last
> refcount is released, the wait in the vfio_unregister_group_dev() will be
> unblocked. It is the vfio_device_fops_release() either userspace exits or
> userspace explicitly close the vfio device fd. Your below test patch moves
> the majority of the vfio_device_fops_release() out of the existing path.
> I don't see a reason why it can work so far.

Forgive my poor English. Let me explain in more detail.
irq 149 thread take pci_rescan_remove_lock mutex lock, take reset_lock
of ctrl B and release reset_lock of ctrl B. Then wait for SPDK to
release vfio refcount connected to ctrl B.
irq 148 thread take reset_lock of ctrl A, and wait for psi_rescan_remove_lock.
SPDK process exit, and try to release vfio refcount connected to ctrl
A, but need to wait for reset_lock of ctrl A.  Since SPDK is blocked
here, it won’t release vfio refcount connected to ctrl B. So the
deadlock is happening.
After my patch. When SPDK exit it’s async to release vfio refcount
connected to ctrl A, it still will block, but SPDK can still can
release vfio refcount connected to ctrl B. Since irq 149 release
reset_lock of ctrl B, it won’t Block. After release vfio refcount
connected to ctrl B, irq 149 can finish it’s work, and will release
pci_rescan_remove_lock mutex lock, and then irq 148 can take
pci_rescan_remove_lock, then all is same as ctrl A.


>
> As the locking issue has been solved in the above commit, seems there is
> no deadlock with the reset_lock and device_lock. Can you confirm if the
> scenario can be reproduced with one device? Also, even with two devices,
> does killing the process matters or not?

This problem is not  a deadlock with the reset_lock and device_lock.
The scenario can’t be reproduced with one
device, only two devices can reproduce this problem. Kill the process
may not be important, but
the interrupt thread is blocked too, and all lspci commands will
block, this affects some monitoring
programs.


>
> > Here is my test patch, cc some vfio guys:
> > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> > index 50128da18bca..4ebe154a4ae5 100644
> > --- a/drivers/vfio/vfio.h
> > +++ b/drivers/vfio/vfio.h
> > @@ -19,6 +19,7 @@ struct vfio_container;
> >   struct vfio_device_file {
> >          struct vfio_device *device;
> >          struct vfio_group *group;
> > +       struct work_struct      release_work;
> >
> >          u8 access_granted;
> >          u32 devid; /* only valid when iommufd is valid */
> > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > index a5a62d9d963f..47e3e3f73d70 100644
> > --- a/drivers/vfio/vfio_main.c
> > +++ b/drivers/vfio/vfio_main.c
> > @@ -487,6 +487,22 @@ static bool vfio_assert_device_open(struct vfio_device *device)
> >          return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
> >   }
> >
> > +static void vfio_fops_release_work(struct work_struct *work)
> > +{
> > +       struct vfio_device_file *df =
> > +               container_of(work, struct vfio_device_file, release_work);
> > +       struct vfio_device *device = df->device;
> > +
> > +       if (df->group)
> > +               vfio_df_group_close(df);
> > +       else
> > +               vfio_df_unbind_iommufd(df);
> > +
> > +       vfio_device_put_registration(device);
> > +
> > +       kfree(df);
> > +}
> > +
> >   struct vfio_device_file *
> >   vfio_allocate_device_file(struct vfio_device *device)
> >   {
> > @@ -497,6 +513,7 @@ vfio_allocate_device_file(struct vfio_device *device)
> >                  return ERR_PTR(-ENOMEM);
> >
> >          df->device = device;
> > +       INIT_WORK(&df->release_work, vfio_fops_release_work);
> >          spin_lock_init(&df->kvm_ref_lock);
> >
> >          return df;
> > @@ -628,16 +645,8 @@ static inline void vfio_device_pm_runtime_put(struct vfio_device *device)
> >   static int vfio_device_fops_release(struct inode *inode, struct file *filep)
> >   {
> >          struct vfio_device_file *df = filep->private_data;
> > -       struct vfio_device *device = df->device;
> >
> > -       if (df->group)
> > -               vfio_df_group_close(df);
> > -       else
> > -               vfio_df_unbind_iommufd(df);
> > -
> > -       vfio_device_put_registration(device);
> > -
> > -       kfree(df);
> > +       schedule_work(&df->release_work);
> >
> >          return 0;
> >   }
> >
> >
> >> [<0>] vfio_pci_core_unregister_device+0x19/0x80 [vfio_pci_core]
> >> [<0>] vfio_pci_remove+0x15/0x20 [vfio_pci]
> >> [<0>] pci_device_remove+0x39/0xb0
> >> [<0>] device_release_driver_internal+0xad/0x120
> >> [<0>] pci_stop_bus_device+0x5d/0x80
> >> [<0>] pci_stop_and_remove_bus_device+0xe/0x20
> >> [<0>] pciehp_unconfigure_device+0x91/0x160   //hold pci_rescan_remove_lock, release reset_lock of ctrl B
> >> [<0>] pciehp_disable_slot+0x6b/0x130
> >> [<0>] pciehp_handle_presence_or_link_change+0x7d/0x4d0
> >> [<0>] pciehp_ist+0x236/0x260             //hold reset_lock of ctrl B
> >> [<0>] irq_thread_fn+0x1b/0x60
> >> [<0>] irq_thread+0xed/0x190
> >> [<0>] kthread+0xe4/0x110
> >> [<0>] ret_from_fork+0x2d/0x50
> >> [<0>] ret_from_fork_asm+0x11/0x20
> >>
> >>
> >> SPDK stack, cat /proc/166634/task/167181/stack
> >> [<0>] down_write_nested+0x1b7/0x1c0            //wait for reset_lock of ctrl A.
> >> [<0>] pciehp_reset_slot+0x58/0x160
> >> [<0>] pci_reset_hotplug_slot+0x3b/0x60
> >> [<0>] pci_reset_bus_function+0x3b/0xb0
> >> [<0>] __pci_reset_function_locked+0x3e/0x60
> >> [<0>] vfio_pci_core_disable+0x3ce/0x400 [vfio_pci_core]
> >> [<0>] vfio_pci_core_close_device+0x67/0xc0 [vfio_pci_core]
> >> [<0>] vfio_df_close+0x79/0xd0 [vfio]
> >> [<0>] vfio_df_group_close+0x36/0x70 [vfio]
> >> [<0>] vfio_device_fops_release+0x20/0x40 [vfio]
> >> [<0>] __fput+0xec/0x290
> >> [<0>] task_work_run+0x61/0x90
> >> [<0>] do_exit+0x39c/0xc40
> >> [<0>] do_group_exit+0x33/0xa0
> >> [<0>] get_signal+0xd84/0xd90
> >> [<0>] arch_do_signal_or_restart+0x2a/0x260
> >> [<0>] exit_to_user_mode_prepare+0x1c7/0x240
> >> [<0>] syscall_exit_to_user_mode+0x2a/0x60
> >> [<0>] do_syscall_64+0x3e/0x90
> >>
> >
> >
>
> --
> Regards,
> Yi Liu

Re: Deadlock during PCIe hot remove and SPDK exit

[Yi Liu]

On 2024/12/2 11:25, fengnan chang wrote:
> On Fri, Nov 29, 2024 at 4:00 PM Yi Liu <yi.l.liu@intel.com> wrote:
>>
>> On 2024/11/28 11:50, fengnan chang wrote:
>>>
>>>
>>>> 2024年11月27日 14:56，fengnan chang <fengnanchang@gmail.com> 写道：
>>>>
>>>> Dear PCI maintainers:
>>>>     I'm having a deadlock issue, somewhat similar to a previous one https://lore.kernel.org/linux-pci/CS1PR8401MB0728FC6FDAB8A35C22BD90EC95F10@CS1PR8401MB0728.NAMPRD84.PROD.OUTLOOK.COM/#t， but my kernel (6.6.40) already included the fix f5eff55.
>>
>> The previous bug was solved by the below commit.
>>
>> commit f5eff5591b8f9c5effd25c92c758a127765f74c1
>> Author: Lukas Wunner <lukas@wunner.de>
>> Date:   Tue Apr 11 08:21:02 2023 +0200
>>
>>       PCI: pciehp: Fix AB-BA deadlock between reset_lock and device_lock
>>
> 
> Yes, my kernel version included this fix.
> 
>>       In 2013, commits
>>
>>         2e35afaefe64 ("PCI: pciehp: Add reset_slot() method")
>>         608c388122c7 ("PCI: Add slot reset option to pci_dev_reset()")
>>
>>       amended PCIe hotplug to mask Presence Detect Changed events during a
>>       Secondary Bus Reset.  The reset thus no longer causes gratuitous slot
>>       bringdown and bringup.
>>
>>       However the commits neglected to serialize reset with code paths reading
>>       slot registers.  For instance, a slot bringup due to an earlier hotplug
>>       event may see the Presence Detect State bit cleared during a concurrent
>>       Secondary Bus Reset.
>>
>>       In 2018, commit
>>
>>         5b3f7b7d062b ("PCI: pciehp: Avoid slot access during reset")
>>
>>       retrofitted the missing locking.  It introduced a reset_lock which
>>       serializes a Secondary Bus Reset with other parts of pciehp.
>>
>>       Unfortunately the locking turns out to be overzealous:  reset_lock is
>>       held for the entire enumeration and de-enumeration of hotplugged devices,
>>       including driver binding and unbinding.
>>
>>       Driver binding and unbinding acquires device_lock while the reset_lock
>>       of the ancestral hotplug port is held.  A concurrent Secondary Bus Reset
>>       acquires the ancestral reset_lock while already holding the device_lock.
>>       The asymmetric locking order in the two code paths can lead to AB-BA
>>       deadlocks.
>>
>>       Michael Haeuptle reports such deadlocks on simultaneous hot-removal and
>>       vfio release (the latter implies a Secondary Bus Reset):
>>
>>         pciehp_ist()                                    # down_read(reset_lock)
>>           pciehp_handle_presence_or_link_change()
>>             pciehp_disable_slot()
>>               __pciehp_disable_slot()
>>                 remove_board()
>>                   pciehp_unconfigure_device()
>>                     pci_stop_and_remove_bus_device()
>>                       pci_stop_bus_device()
>>                         pci_stop_dev()
>>                           device_release_driver()
>>                             device_release_driver_internal()
>>                               __device_driver_lock()    # device_lock()
>>
>>         SYS_munmap()
>>           vfio_device_fops_release()
>>             vfio_device_group_close()
>>               vfio_device_close()
>>                 vfio_device_last_close()
>>
>>
>>>>     Here is my test process, I’m running kernel with 6.6.40 and SPDK v22.05:
>>>>     1. SPDK use vfio driver to takeover two nvme disks, running some io in nvme.
>>>>     2. pull out two nvme disks
>>>>     3. Try to kill -9 SPDK process.
>>>>     Then deadlock issue happened. For now I can 100% reproduce this problem. I’m not an export in PCI, but I did a brief analysis:
>>>>     irq 149 thread take pci_rescan_remove_lock mutex lock, and wait for SPDK to release vfio.
>>>>     irq 148 thread take reset_lock of ctrl A, and wait for psi_rescan_remove_lock
>>>>     SPDK process try to release vfio driver, but wait for reset_lock of ctrl A.
>>>>
>>>>
>>>> irq/149-pciehp stack, cat /proc/514/stack,
>>>> [<0>] pciehp_unconfigure_device+0x48/0x160 // wait for pci_rescan_remove_lock
>>>> [<0>] pciehp_disable_slot+0x6b/0x130       // hold reset_lock of ctrl A
>>>> [<0>] pciehp_handle_presence_or_link_change+0x7d/0x4d0
>>>> [<0>] pciehp_ist+0x236/0x260
>>>> [<0>] irq_thread_fn+0x1b/0x60
>>>> [<0>] irq_thread+0xed/0x190
>>>> [<0>] kthread+0xe4/0x110
>>>> [<0>] ret_from_fork+0x2d/0x50
>>>> [<0>] ret_from_fork_asm+0x11/0x20
>>>>
>>>>
>>>> irq/148-pciehp stack, cat /proc/513/stack
>>>> [<0>] vfio_unregister_group_dev+0x97/0xe0 [vfio]     //wait for
>>>
>>> My mistake, this is wait for SPDK to release vfio device. This problem can reproduce in 6.12.
>>> Besides, My college give me an idea, we can make vfio_device_fops_release be async, so when we close fd, it
>>> won’t block, and when we close another fd, it will release  vfio device, this stack will not block too, then the deadlock disappears.
>>>
>>
>> In the hotplug path, vfio needs to notify userspace to stop the usage of
>> this device and release reference on the vfio_device. When the last
>> refcount is released, the wait in the vfio_unregister_group_dev() will be
>> unblocked. It is the vfio_device_fops_release() either userspace exits or
>> userspace explicitly close the vfio device fd. Your below test patch moves
>> the majority of the vfio_device_fops_release() out of the existing path.
>> I don't see a reason why it can work so far.
> 
> Forgive my poor English. Let me explain in more detail.
> irq 149 thread take pci_rescan_remove_lock mutex lock, take reset_lock
> of ctrl B and release reset_lock of ctrl B. Then wait for SPDK to
> release vfio refcount connected to ctrl B.
> irq 148 thread take reset_lock of ctrl A, and wait for psi_rescan_remove_lock.

it's 148 which holds pci_rescan_remove_lock and wait for the last refcount
of vfio_device.

> SPDK process exit, and try to release vfio refcount connected to ctrl
> A, but need to wait for reset_lock of ctrl A.  Since SPDK is blocked
> here, it won’t release vfio refcount connected to ctrl B. So the
> deadlock is happening.

but I got you now. So the SPDK process exit thread closes the vfio_device
A and B sequentially. The vfio_device A file release is blocked by the 149
while 149 is blocked by the pci_rescan_remove_lock which is held by 148.
148 is waiting for the vfio_device B's refcount which is done in
vfio_device B's file release.  The vfio_device B file release is not done
because of the exit thread is dealing with vfio_device A. So this should be
related to timing. Did you see any difference w.r.t. the order of plugging
out the devices? Maybe you can try something different to double confirm
it. e.g. plugging out the devices as the order of how they are listed in
the QEMU cmdline.

> After my patch. When SPDK exit it’s async to release vfio refcount
> connected to ctrl A, it still will block, but SPDK can still can
> release vfio refcount connected to ctrl B. Since irq 149 release
> reset_lock of ctrl B, it won’t Block. After release vfio refcount
> connected to ctrl B, irq 149 can finish it’s work, and will release
> pci_rescan_remove_lock mutex lock, and then irq 148 can take
> pci_rescan_remove_lock, then all is same as ctrl A.
> 
> 
>>
>> As the locking issue has been solved in the above commit, seems there is
>> no deadlock with the reset_lock and device_lock. Can you confirm if the
>> scenario can be reproduced with one device? Also, even with two devices,
>> does killing the process matters or not?
> 
> This problem is not  a deadlock with the reset_lock and device_lock.
> The scenario can’t be reproduced with one
> device, only two devices can reproduce this problem. Kill the process
> may not be important, but
> the interrupt thread is blocked too, and all lspci commands will
> block, this affects some monitoring
> programs.

yes, this issue is not related to no response from userspace.


-- 
Regards,
Yi Liu

Re: Deadlock during PCIe hot remove and SPDK exit

[fengnan chang]

On Tue, Dec 3, 2024 at 1:31 PM Yi Liu <yi.l.liu@intel.com> wrote:
>
> On 2024/12/2 11:25, fengnan chang wrote:
> > On Fri, Nov 29, 2024 at 4:00 PM Yi Liu <yi.l.liu@intel.com> wrote:
> >>
> >> On 2024/11/28 11:50, fengnan chang wrote:
> >>>
> >>>
> >>>> 2024年11月27日 14:56，fengnan chang <fengnanchang@gmail.com> 写道：
> >>>>
> >>>> Dear PCI maintainers:
> >>>>     I'm having a deadlock issue, somewhat similar to a previous one https://lore.kernel.org/linux-pci/CS1PR8401MB0728FC6FDAB8A35C22BD90EC95F10@CS1PR8401MB0728.NAMPRD84.PROD.OUTLOOK.COM/#t， but my kernel (6.6.40) already included the fix f5eff55.
> >>
> >> The previous bug was solved by the below commit.
> >>
> >> commit f5eff5591b8f9c5effd25c92c758a127765f74c1
> >> Author: Lukas Wunner <lukas@wunner.de>
> >> Date:   Tue Apr 11 08:21:02 2023 +0200
> >>
> >>       PCI: pciehp: Fix AB-BA deadlock between reset_lock and device_lock
> >>
> >
> > Yes, my kernel version included this fix.
> >
> >>       In 2013, commits
> >>
> >>         2e35afaefe64 ("PCI: pciehp: Add reset_slot() method")
> >>         608c388122c7 ("PCI: Add slot reset option to pci_dev_reset()")
> >>
> >>       amended PCIe hotplug to mask Presence Detect Changed events during a
> >>       Secondary Bus Reset.  The reset thus no longer causes gratuitous slot
> >>       bringdown and bringup.
> >>
> >>       However the commits neglected to serialize reset with code paths reading
> >>       slot registers.  For instance, a slot bringup due to an earlier hotplug
> >>       event may see the Presence Detect State bit cleared during a concurrent
> >>       Secondary Bus Reset.
> >>
> >>       In 2018, commit
> >>
> >>         5b3f7b7d062b ("PCI: pciehp: Avoid slot access during reset")
> >>
> >>       retrofitted the missing locking.  It introduced a reset_lock which
> >>       serializes a Secondary Bus Reset with other parts of pciehp.
> >>
> >>       Unfortunately the locking turns out to be overzealous:  reset_lock is
> >>       held for the entire enumeration and de-enumeration of hotplugged devices,
> >>       including driver binding and unbinding.
> >>
> >>       Driver binding and unbinding acquires device_lock while the reset_lock
> >>       of the ancestral hotplug port is held.  A concurrent Secondary Bus Reset
> >>       acquires the ancestral reset_lock while already holding the device_lock.
> >>       The asymmetric locking order in the two code paths can lead to AB-BA
> >>       deadlocks.
> >>
> >>       Michael Haeuptle reports such deadlocks on simultaneous hot-removal and
> >>       vfio release (the latter implies a Secondary Bus Reset):
> >>
> >>         pciehp_ist()                                    # down_read(reset_lock)
> >>           pciehp_handle_presence_or_link_change()
> >>             pciehp_disable_slot()
> >>               __pciehp_disable_slot()
> >>                 remove_board()
> >>                   pciehp_unconfigure_device()
> >>                     pci_stop_and_remove_bus_device()
> >>                       pci_stop_bus_device()
> >>                         pci_stop_dev()
> >>                           device_release_driver()
> >>                             device_release_driver_internal()
> >>                               __device_driver_lock()    # device_lock()
> >>
> >>         SYS_munmap()
> >>           vfio_device_fops_release()
> >>             vfio_device_group_close()
> >>               vfio_device_close()
> >>                 vfio_device_last_close()
> >>
> >>
> >>>>     Here is my test process, I’m running kernel with 6.6.40 and SPDK v22.05:
> >>>>     1. SPDK use vfio driver to takeover two nvme disks, running some io in nvme.
> >>>>     2. pull out two nvme disks
> >>>>     3. Try to kill -9 SPDK process.
> >>>>     Then deadlock issue happened. For now I can 100% reproduce this problem. I’m not an export in PCI, but I did a brief analysis:
> >>>>     irq 149 thread take pci_rescan_remove_lock mutex lock, and wait for SPDK to release vfio.
> >>>>     irq 148 thread take reset_lock of ctrl A, and wait for psi_rescan_remove_lock
> >>>>     SPDK process try to release vfio driver, but wait for reset_lock of ctrl A.
> >>>>
> >>>>
> >>>> irq/149-pciehp stack, cat /proc/514/stack,
> >>>> [<0>] pciehp_unconfigure_device+0x48/0x160 // wait for pci_rescan_remove_lock
> >>>> [<0>] pciehp_disable_slot+0x6b/0x130       // hold reset_lock of ctrl A
> >>>> [<0>] pciehp_handle_presence_or_link_change+0x7d/0x4d0
> >>>> [<0>] pciehp_ist+0x236/0x260
> >>>> [<0>] irq_thread_fn+0x1b/0x60
> >>>> [<0>] irq_thread+0xed/0x190
> >>>> [<0>] kthread+0xe4/0x110
> >>>> [<0>] ret_from_fork+0x2d/0x50
> >>>> [<0>] ret_from_fork_asm+0x11/0x20
> >>>>
> >>>>
> >>>> irq/148-pciehp stack, cat /proc/513/stack
> >>>> [<0>] vfio_unregister_group_dev+0x97/0xe0 [vfio]     //wait for
> >>>
> >>> My mistake, this is wait for SPDK to release vfio device. This problem can reproduce in 6.12.
> >>> Besides, My college give me an idea, we can make vfio_device_fops_release be async, so when we close fd, it
> >>> won’t block, and when we close another fd, it will release  vfio device, this stack will not block too, then the deadlock disappears.
> >>>
> >>
> >> In the hotplug path, vfio needs to notify userspace to stop the usage of
> >> this device and release reference on the vfio_device. When the last
> >> refcount is released, the wait in the vfio_unregister_group_dev() will be
> >> unblocked. It is the vfio_device_fops_release() either userspace exits or
> >> userspace explicitly close the vfio device fd. Your below test patch moves
> >> the majority of the vfio_device_fops_release() out of the existing path.
> >> I don't see a reason why it can work so far.
> >
> > Forgive my poor English. Let me explain in more detail.
> > irq 149 thread take pci_rescan_remove_lock mutex lock, take reset_lock
> > of ctrl B and release reset_lock of ctrl B. Then wait for SPDK to
> > release vfio refcount connected to ctrl B.
> > irq 148 thread take reset_lock of ctrl A, and wait for psi_rescan_remove_lock.
>
> it's 148 which holds pci_rescan_remove_lock and wait for the last refcount
> of vfio_device.
>
> > SPDK process exit, and try to release vfio refcount connected to ctrl
> > A, but need to wait for reset_lock of ctrl A.  Since SPDK is blocked
> > here, it won’t release vfio refcount connected to ctrl B. So the
> > deadlock is happening.
>
> but I got you now. So the SPDK process exit thread closes the vfio_device
> A and B sequentially. The vfio_device A file release is blocked by the 149
> while 149 is blocked by the pci_rescan_remove_lock which is held by 148.
> 148 is waiting for the vfio_device B's refcount which is done in
> vfio_device B's file release.  The vfio_device B file release is not done
> because of the exit thread is dealing with vfio_device A. So this should be
> related to timing. Did you see any difference w.r.t. the order of plugging
> out the devices? Maybe you can try something different to double confirm
> it. e.g. plugging out the devices as the order of how they are listed in
> the QEMU cmdline.

Yes, if the order of plugging out the device changed, this problem
would also disappear.
But in the real world, we encounter problems because the ssd disk is
broken, so I couldn't
change the order.

>
> > After my patch. When SPDK exit it’s async to release vfio refcount
> > connected to ctrl A, it still will block, but SPDK can still can
> > release vfio refcount connected to ctrl B. Since irq 149 release
> > reset_lock of ctrl B, it won’t Block. After release vfio refcount
> > connected to ctrl B, irq 149 can finish it’s work, and will release
> > pci_rescan_remove_lock mutex lock, and then irq 148 can take
> > pci_rescan_remove_lock, then all is same as ctrl A.
> >
> >
> >>
> >> As the locking issue has been solved in the above commit, seems there is
> >> no deadlock with the reset_lock and device_lock. Can you confirm if the
> >> scenario can be reproduced with one device? Also, even with two devices,
> >> does killing the process matters or not?
> >
> > This problem is not  a deadlock with the reset_lock and device_lock.
> > The scenario can’t be reproduced with one
> > device, only two devices can reproduce this problem. Kill the process
> > may not be important, but
> > the interrupt thread is blocked too, and all lspci commands will
> > block, this affects some monitoring
> > programs.
>
> yes, this issue is not related to no response from userspace.
>
>
> --
> Regards,
> Yi Liu