Re: [PATCH v1 1/3] iommu/vt-d: Fix prq reporting issues

Yi Sun
2020-10-28T06:10:39Z

Thanks! Will do the modifications according to your comments in next
version.

On 20-10-28 12:50:37, Lu Baolu wrote:
> Hi Yi,
>
> On 10/28/20 9:36 AM, Yi Sun wrote:
> >From: Liu Yi L <yi.l.liu@intel.com>
> >
> >Should get correct sid and set it into sdev. Because we execute
> >'sdev->sid != req->rid' in the loop of prq_event_thread().
>
> How about making the title more accurate, how about something like:
>
> iommu/vt-d: Fix use before set issue in intel_svm_bind_gpasid()
>
> >
> >Signed-off-by: Liu Yi L <yi.l.liu@intel.com>
> >Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> >---
> > drivers/iommu/intel/svm.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> >diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> >index f1861fa..75d9dc9 100644
> >--- a/drivers/iommu/intel/svm.c
> >+++ b/drivers/iommu/intel/svm.c
> >@@ -277,6 +277,7 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
> > struct iommu_gpasid_bind_data *data)
> > {
> > struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
> >+ struct device_domain_info *info;
> > struct intel_svm_dev *sdev = NULL;
> > struct dmar_domain *dmar_domain;
> > struct intel_svm *svm = NULL;
> >@@ -357,6 +358,8 @@ int intel_svm_bind_gpasid(struct iommu_domain *domain, struct device *dev,
> > goto out;
> > }
> > sdev->dev = dev;
> >+ info = get_domain_info(dev);
> >+ sdev->sid = PCI_DEVID(info->bus, info->devfn);
> > /* Only count users if device has aux domains */
> > if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX))
> >
>
> How about moving get_domain_info() up to the sanity check part?
>
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index f1861fa3d0e4..75846692f2f2 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -279,6 +279,7 @@ int intel_svm_bind_gpasid(struct iommu_domain
> *domain, struct device *dev,
> struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
> struct intel_svm_dev *sdev = NULL;
> struct dmar_domain *dmar_domain;
> + struct device_domain_info *info;
> struct intel_svm *svm = NULL;
> int ret = 0;
>
> @@ -310,6 +311,10 @@ int intel_svm_bind_gpasid(struct iommu_domain
> *domain, struct device *dev,
> if (data->hpasid <= 0 || data->hpasid >= PASID_MAX)
> return -EINVAL;
>
> + info = get_domain_info(dev);
> + if (!info)
> + return -EINVAL;
> +
> dmar_domain = to_dmar_domain(domain);
>
> mutex_lock(&pasid_mutex);
> @@ -357,6 +362,7 @@ int intel_svm_bind_gpasid(struct iommu_domain
> *domain, struct device *dev,
> goto out;
> }
> sdev->dev = dev;
> + sdev->sid = PCI_DEVID(info->bus, info->devfn);
>
> /* Only count users if device has aux domains */
> if (iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_AUX))
>
> Best regards,
> baolu