iommu/vt-d: unlock on error in intel_iommu_load_translation_tables()
Subsystems (2)
PATCH 1/1 iommu/vt-d: unlock on error in intel_iommu_load_translation_tables()
Smatch found some error paths that don't unlock. Also we can return -ENOMEM instead of -1 if we don't have an old root entry.
---
Releasing the lock is good, but we might need to do other error handling as well. Presumably this function always succeeds anyway? It seems like it might be essential for booting.
drivers/iommu/intel-iommu.c +10 -4
@@ -5151,22 +5151,24 @@ static int intel_iommu_load_translation_tables(struct intel_iommu *iommu) iommu->root_entry (struct root_entry *)alloc_pgtable_page(iommu->node);if (!iommu->root_entry) { -spin_unlock_irqrestore (&iommu->lock, flags); -return -ENOMEM ; + ret = -ENOMEM ; + goto err_unlock; } } iommu->root_entry_old_phys = q &VTD_PAGE_MASK ;if (!iommu->root_entry_old_phys) {pr_err ("Could not read old root entry address." ); -return -1 ; + ret = -ENOMEM ; + goto err_unlock; } iommu->root_entry_old_virt =ioremap_cache (iommu->root_entry_old_phys,VTD_PAGE_SIZE );if (!iommu->root_entry_old_virt) {pr_err ("Could not map the old root entry." ); -return -ENOMEM ; + ret = -ENOMEM ; + goto err_unlock; }__iommu_load_old_root_entry (iommu);
@@ -5179,6 +5181,10 @@ static int intel_iommu_load_translation_tables(struct intel_iommu *iommu)__iommu_free_mapped_mem ();return ret; + +err_unlock: +spin_unlock_irqrestore (&iommu->lock, flags); +return ret; } static void unmap_device_dma(struct dmar_domain *domain,
Quoted 5 lines · Dan Carpenter
Smatch found some error paths that don't unlock. Also we can return
-ENOMEMinstead of -1 if we don't have an old root entry.Fixes 5908f10af4b9 ('iommu/vt-d: datatypes and functions used for kdump')Signed-off-by Dan Carpenter <[email protected]>
Applied, thanks.
Quoted 3 lines
Releasing the lock is good, but we might need to do other error handling as well. Presumably this function always succeeds anyway? It seems like it might be essential for booting.
What do you mean by 'other error handling'? In error case a negative value is returned and the caller checks that.
Joerg
Quoted 7 lines · Joerg Roedel
Quoted 5 lines · Dan Carpenter
Smatch found some error paths that don't unlock. Also we can return
-ENOMEMinstead of -1 if we don't have an old root entry.Fixes 5908f10af4b9 ('iommu/vt-d: datatypes and functions used for kdump')Signed-off-by Dan Carpenter <[email protected]>
Quoted 8 lines
Applied, thanks.
Quoted 3 lines
Releasing the lock is good, but we might need to do other error handling as well. Presumably this function always succeeds anyway? It seems like it might be essential for booting.
What do you mean by 'other error handling'? In error case a negative value is returned and the caller checks that.
I was just worried we should call __iommu_free_mapped_mem() or something. I don't know this code very well is what I'm saying...
regards,
dan carpenter
Dan,
Do not need to call __iommu_free_mapped_mem() for err_unlock. for no new elements are added into __iommu_remapped_mem when drops to err_unlock.
Thanks
Zhenhua
Quoted 23 lines · Dan Carpenter
Quoted 7 lines · Joerg Roedel
Quoted 5 lines · Dan Carpenter
Smatch found some error paths that don't unlock. Also we can return
-ENOMEMinstead of -1 if we don't have an old root entry.Fixes 5908f10af4b9 ('iommu/vt-d: datatypes and functions used for kdump')Signed-off-by Dan Carpenter <[email protected]>Quoted 8 lines
Applied, thanks.
Quoted 3 lines
Releasing the lock is good, but we might need to do other error handling as well. Presumably this function always succeeds anyway? It seems like it might be essential for booting.
What do you mean by 'other error handling'? In error case a negative value is returned and the caller checks that.
I was just worried we should call
__iommu_free_mapped_mem()or something. I don't know this code very well is what I'm saying...regards,
dan carpenter