Re: [PATCH] iommu: Introduce reset_mutex to avoid circular locking dependency

[David Matlack]

On Thu, Aug 27, 2026 at 10:35 AM David Matlack <dmatlack@xxxxxxxxxx> wrote:
>
> Introduce a dedicated reset_mutex inside struct iommu_group to serialize
> device reset operations and domain resetting state without acquiring
> group->mutex.
>
> Commit f5b16b802174 ("PCI: Suspend iommu function prior to resetting a
> device") introduced pci_dev_reset_iommu_prepare() and
> pci_dev_reset_iommu_done(), which acquire group->mutex during PCI device
> resets. In drivers such as VFIO, device reset handlers (e.g.
> pci_try_reset_function()) are executed while holding driver locks such
> as vdev->memory_lock.
>
> This introduces a circular locking dependency that can lead to tasks
> being permanently stuck in a deadlock:
>
> [vdev->memory_lock]  ──────(1. VFIO Reset)──────>  [iommu_group->mutex]
>          ▲
>          │                                                    │
>  (3. VFIO BAR Page Fault)                                    ... [*]
>          │                                                    │
>          │                                                    │
>          └───────────  [mm->mmap_lock]  <─────────────────────┘
>
> Fix this by using group->reset_mutex instead of group->mutex in
> pci_dev_reset_iommu_prepare() and pci_dev_reset_iommu_done(). To ensure
> other iommu_group operations stay properly synchronized with resets,
> also take group->reset_mutex when checking group->recovery_cnt.
>
> [*] The iommu_group->mutex --> mm->mmap_lock dependency was reported as
> a transitive chain: iommu_group->mutex --> cpu_hotplug_lock -->
> i_mutex_dir_key --> mm->mmap_lock.
>
> Reported-by: Vipin Sharma <vipinsh@xxxxxxxxxx>
> Closes: https://lore.kernel.org/kvm/20260821193502.92431-1-vipinsh@xxxxxxxxxx/
> Fixes: f5b16b802174 ("PCI: Suspend iommu function prior to resetting a device")
> Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>
> ---
> Cc: Alex Williamson <alex@xxxxxxxxxxx>
>
>  drivers/iommu/iommu.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e8f13dcebbde..29e63697211f 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -56,6 +56,7 @@ struct iommu_group {
>         struct list_head devices;
>         struct xarray pasid_array;
>         struct mutex mutex;
> +       struct mutex reset_mutex;
>         void *iommu_data;
>         void (*iommu_data_release)(void *iommu_data);
>         char *name;
> @@ -1080,6 +1081,7 @@ struct iommu_group *iommu_group_alloc(void)
>
>         group->kobj.kset = iommu_group_kset;
>         mutex_init(&group->mutex);
> +       mutex_init(&group->reset_mutex);
>         INIT_LIST_HEAD(&group->devices);
>         INIT_LIST_HEAD(&group->entry);
>         xa_init(&group->pasid_array);
> @@ -2476,6 +2478,7 @@ static int __iommu_group_set_domain_internal(struct iommu_group *group,
>          * pci_dev_reset_iommu_done() attaches the device to group->domain, if
>          * IOMMU_SET_DOMAIN_MUST_SUCCEED is not set.
>          */
> +       guard(mutex)(&group->reset_mutex);
>         if (group->recovery_cnt && !(flags & IOMMU_SET_DOMAIN_MUST_SUCCEED))
>                 return -EBUSY;
>
> @@ -3652,6 +3655,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>          * This is a concurrent attach during device recovery. Reject it until
>          * pci_dev_reset_iommu_done() attaches the device to group->domain.
>          */
> +       guard(mutex)(&group->reset_mutex);
>         if (group->recovery_cnt) {
>                 ret = -EBUSY;
>                 goto out_unlock;
> @@ -3745,6 +3749,7 @@ int iommu_replace_device_pasid(struct iommu_domain *domain,
>          * This is a concurrent attach during device recovery. Reject it until
>          * pci_dev_reset_iommu_done() attaches the device to group->domain.
>          */
> +       guard(mutex)(&group->reset_mutex);
>         if (group->recovery_cnt) {
>                 ret = -EBUSY;
>                 goto out_unlock;
> @@ -4042,7 +4047,7 @@ int pci_dev_reset_iommu_prepare(struct pci_dev *pdev)
>         if (!pci_ats_supported(pdev) || !dev_has_iommu(&pdev->dev))
>                 return 0;
>
> -       guard(mutex)(&group->mutex);
> +       guard(mutex)(&group->reset_mutex);
>
>         gdev = __dev_to_gdev(&pdev->dev);
>         if (WARN_ON(!gdev))
> @@ -4153,7 +4158,7 @@ void pci_dev_reset_iommu_done(struct pci_dev *pdev)
>         if (!pci_ats_supported(pdev) || !dev_has_iommu(&pdev->dev))
>                 return;
>
> -       guard(mutex)(&group->mutex);
> +       guard(mutex)(&group->reset_mutex);
>
>         gdev = __dev_to_gdev(&pdev->dev);

Ah, this does not work. __dev_to_gdev() has a lockdep assert that
group->mutex must be held. I'm not sure why my test did not catch
that...

>         if (WARN_ON(!gdev))
>
> base-commit: 37ffa24c9d07edcd414d34283e02af3f3866cf12
> --
> 2.55.0.897.gb25b4bd76c-goog
>