Re: [PATCH v2 11/12] iommu/vt-d: Use device_domain_lock accurately
On 2022/6/14 15:16, Tian, Kevin wrote: From: Lu Baolu Sent: Tuesday, June 14, 2022 10:52 AM The device_domain_lock is used to protect the device tracking list of a domain. Remove unnecessary spin_lock/unlock()'s and move the necessary ones around the list access. Signed-off-by: Lu Baolu --- drivers/iommu/intel/iommu.c | 68 +++-- 1 file changed, 27 insertions(+), 41 deletions(-) [...] +iommu_support_dev_iotlb(struct dmar_domain *domain, struct intel_iommu *iommu, + u8 bus, u8 devfn) { - struct device_domain_info *info; - - assert_spin_locked(&device_domain_lock); + struct device_domain_info *info = NULL, *tmp; + unsigned long flags; if (!iommu->qi) return NULL; - list_for_each_entry(info, &domain->devices, link) - if (info->iommu == iommu && info->bus == bus && - info->devfn == devfn) { - if (info->ats_supported && info->dev) - return info; + spin_lock_irqsave(&device_domain_lock, flags); + list_for_each_entry(tmp, &domain->devices, link) { + if (tmp->iommu == iommu && tmp->bus == bus && + tmp->devfn == devfn) { + if (tmp->ats_supported) + info = tmp; Directly returning with unlock here is clearer than adding another tmp variable... Sure. @@ -2460,15 +2450,14 @@ static int domain_add_dev_info(struct dmar_domain *domain, struct device *dev) if (!iommu) return -ENODEV; - spin_lock_irqsave(&device_domain_lock, flags); - info->domain = domain; ret = domain_attach_iommu(domain, iommu); - if (ret) { - spin_unlock_irqrestore(&device_domain_lock, flags); + if (ret) return ret; - } + + spin_lock_irqsave(&device_domain_lock, flags); list_add(&info->link, &domain->devices); spin_unlock_irqrestore(&device_domain_lock, flags); + info->domain = domain; This is incorrect. You need fully initialize the object before adding it to the list. Otherwise a search right after above unlock and before assigning info->domain will get a wrong data Fair enough. Will fix it in the next version. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v2 11/12] iommu/vt-d: Use device_domain_lock accurately
> From: Lu Baolu > Sent: Tuesday, June 14, 2022 10:52 AM > > The device_domain_lock is used to protect the device tracking list of > a domain. Remove unnecessary spin_lock/unlock()'s and move the necessary > ones around the list access. > > Signed-off-by: Lu Baolu > --- > drivers/iommu/intel/iommu.c | 68 +++-- > 1 file changed, 27 insertions(+), 41 deletions(-) > [...] > +iommu_support_dev_iotlb(struct dmar_domain *domain, struct > intel_iommu *iommu, > + u8 bus, u8 devfn) > { > - struct device_domain_info *info; > - > - assert_spin_locked(&device_domain_lock); > + struct device_domain_info *info = NULL, *tmp; > + unsigned long flags; > > if (!iommu->qi) > return NULL; > > - list_for_each_entry(info, &domain->devices, link) > - if (info->iommu == iommu && info->bus == bus && > - info->devfn == devfn) { > - if (info->ats_supported && info->dev) > - return info; > + spin_lock_irqsave(&device_domain_lock, flags); > + list_for_each_entry(tmp, &domain->devices, link) { > + if (tmp->iommu == iommu && tmp->bus == bus && > + tmp->devfn == devfn) { > + if (tmp->ats_supported) > + info = tmp; Directly returning with unlock here is clearer than adding another tmp variable... > @@ -2460,15 +2450,14 @@ static int domain_add_dev_info(struct > dmar_domain *domain, struct device *dev) > if (!iommu) > return -ENODEV; > > - spin_lock_irqsave(&device_domain_lock, flags); > - info->domain = domain; > ret = domain_attach_iommu(domain, iommu); > - if (ret) { > - spin_unlock_irqrestore(&device_domain_lock, flags); > + if (ret) > return ret; > - } > + > + spin_lock_irqsave(&device_domain_lock, flags); > list_add(&info->link, &domain->devices); > spin_unlock_irqrestore(&device_domain_lock, flags); > + info->domain = domain; > This is incorrect. You need fully initialize the object before adding it to the list. Otherwise a search right after above unlock and before assigning info->domain will get a wrong data ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu