Messages in this thread | | | Subject | Re: [PATCH v7 09/11] iommu/vt-d: Add bind guest PASID support | From | Lu Baolu <> | Date | Tue, 29 Oct 2019 10:54:48 +0800 |
| |
Hi,
On 10/29/19 6:29 AM, Jacob Pan wrote: > Hi Baolu, > > Appreciate the thorough review, comments inline.
You are welcome.
> > On Sat, 26 Oct 2019 10:01:19 +0800 > Lu Baolu <baolu.lu@linux.intel.com> wrote: > >> Hi, >>
[...]
>>> + * allow multiple bind calls with the same >>> PASID and pdev. >>> + */ >>> + sdev->users++; >>> + goto out; >>> + } >> >> I remember I ever pointed this out before. But I forgot how we >> addressed it. So forgive me if this has been addressed. >> >> What if we have a valid bound svm but @dev doesn't belong to it >> (a.k.a. @dev not in svm->devs list)? >> > If we are binding a new device to an existing/active PASID, the code > will allocate a new sdev and add that to the svm->devs list.
But allocating a new sdev and adding device is in below else branch, so it will never reach there, right?
>>> + } else { >>> + /* We come here when PASID has never been bond to >>> a device. */ >>> + svm = kzalloc(sizeof(*svm), GFP_KERNEL); >>> + if (!svm) { >>> + ret = -ENOMEM; >>> + goto out; >>> + } >>> + /* REVISIT: upper layer/VFIO can track host >>> process that bind the PASID. >>> + * ioasid_set = mm might be sufficient for vfio to >>> check pasid VMM >>> + * ownership. >>> + */ >>> + svm->mm = get_task_mm(current); >>> + svm->pasid = data->hpasid; >>> + if (data->flags & IOMMU_SVA_GPASID_VAL) { >>> + svm->gpasid = data->gpasid; >>> + svm->flags |= SVM_FLAG_GUEST_PASID; >>> + } >>> + ioasid_set_data(data->hpasid, svm); >>> + INIT_LIST_HEAD_RCU(&svm->devs); >>> + INIT_LIST_HEAD(&svm->list); >>> + >>> + mmput(svm->mm); >>> + } >> >> A blank line, please. > looks good. >> >>> + sdev = kzalloc(sizeof(*sdev), GFP_KERNEL); >>> + if (!sdev) { >>> + if (list_empty(&svm->devs)) >>> + kfree(svm); >> >> This is dangerous. This might leave a wild pointer bound with gpasid. >> > why is that? can you please explain? > if the list is empty that means we just allocated the new svm, no > users. why can't we free it here?
svm has been associated with the pasid private data. It needs to be unbound from pasid before getting freed. Otherwise, a wild pointer will be left.
ioasid_set_data(pasid, NULL); kfree(svm);
> >>> + ret = -ENOMEM; >>> + goto out; >>> + } >>> + sdev->dev = dev; >>> + sdev->users = 1; >>> + >>> + /* Set up device context entry for PASID if not enabled >>> already */ >>> + ret = intel_iommu_enable_pasid(iommu, sdev->dev); >>> + if (ret) { >>> + dev_err(dev, "Failed to enable PASID >>> capability\n"); >>> + kfree(sdev); >>> + goto out; >>> + } >>> + >>> + /* >>> + * For guest bind, we need to set up PASID table entry as >>> follows: >>> + * - FLPM matches guest paging mode >>> + * - turn on nested mode >>> + * - SL guest address width matching >>> + */ >>> + ret = intel_pasid_setup_nested(iommu, >>> + dev, >>> + (pgd_t *)data->gpgd, >>> + data->hpasid, >>> + &data->vtd, >>> + ddomain, >>> + data->addr_width); >>> + if (ret) { >>> + dev_err(dev, "Failed to set up PASID %llu in >>> nested mode, Err %d\n", >>> + data->hpasid, ret); >> >> This error handling is insufficient. You should at least: >> >> 1. free sdev > already done below > >> 2. if list_empty(&svm->devs) >> unbound the svm from gpasid >> free svm >> > yes, agreed. > >> The same for above error handling. Add a branch for error recovery at >> the end of function might help here. >> > not sure which code is the same as above? could you point it out?
Above last comment. :-)
>>> + kfree(sdev); >>> + goto out; >>> + } >>> + svm->flags |= SVM_FLAG_GUEST_MODE; >>> + >>> + init_rcu_head(&sdev->rcu); >>> + list_add_rcu(&sdev->list, &svm->devs); >>> + out: >>> + mutex_unlock(&pasid_mutex); >>> + return ret; >>> +} >>> + >>> +int intel_svm_unbind_gpasid(struct device *dev, int pasid) >>> +{ >>> + struct intel_svm_dev *sdev; >>> + struct intel_iommu *iommu; >>> + struct intel_svm *svm; >>> + int ret = -EINVAL; >>> + >>> + mutex_lock(&pasid_mutex); >>> + iommu = intel_svm_device_to_iommu(dev); >>> + if (!iommu) >>> + goto out; >> >> Make it symmetrical with bind function. >> >> if (WARN_ON(!iommu)) >> goto out; >> > sounds good. >>> + >>> + svm = ioasid_find(NULL, pasid, NULL); >>> + if (IS_ERR_OR_NULL(svm)) { >>> + ret = PTR_ERR(svm); >> >> If svm == NULL, this function will return success. This is not >> expected, right? >> > good catch, will fix. >>> + goto out; >>> + } >>> + >>> + for_each_svm_dev(svm, dev) { >>> + ret = 0; >>> + sdev->users--; >>> + if (!sdev->users) { >>> + list_del_rcu(&sdev->list); >>> + intel_pasid_tear_down_entry(iommu, dev, >>> svm->pasid); >>> + /* TODO: Drain in flight PRQ for the PASID >>> since it >>> + * may get reused soon, we don't want to >>> + * confuse with its previous life. >>> + * intel_svm_drain_prq(dev, pasid); >>> + */ >>> + kfree_rcu(sdev, rcu); >>> + >>> + if (list_empty(&svm->devs)) { >>> + list_del(&svm->list); >>> + kfree(svm); >>> + /* >>> + * We do not free PASID here until >>> explicit call >>> + * from VFIO to free. The PASID >>> life cycle >>> + * management is largely tied to >>> VFIO management >>> + * of assigned device life cycles. >>> In case of >>> + * guest exit without a explicit >>> free PASID call, >>> + * the responsibility lies in VFIO >>> layer to free >>> + * the PASIDs allocated for the >>> guest. >>> + * For security reasons, VFIO has >>> to track the >>> + * PASID ownership per guest >>> anyway to ensure >>> + * that PASID allocated by one >>> guest cannot be >>> + * used by another. >>> + */ >>> + ioasid_set_data(pasid, NULL); >> >> Exchange order. First unbind svm from gpasid and then free svm. >> > I am not following, aren't we already doing free svm after unbind? > please explain.
I meant
ioasid_set_data(pasid, NULL); kfree(svm);
in reverse order, it leaves a short window when svm is freed, but pasid private data is still kept svm (wild pointer).
>>> + } >>> + } >>> + break; >>> + } >>> + out: >>> + mutex_unlock(&pasid_mutex); >>> + >>> + return ret; >>> +} >>> + >>> int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, >>> struct svm_dev_ops *ops) { >>> struct intel_iommu *iommu = >>> intel_svm_device_to_iommu(dev); diff --git >>> a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h index >>> 3dba6ad3e9ad..6c74c71b1ebf 100644 --- a/include/linux/intel-iommu.h >>> +++ b/include/linux/intel-iommu.h >>> @@ -673,7 +673,9 @@ int intel_iommu_enable_pasid(struct intel_iommu >>> *iommu, struct device *dev); int intel_svm_init(struct intel_iommu >>> *iommu); extern int intel_svm_enable_prq(struct intel_iommu *iommu); >>> extern int intel_svm_finish_prq(struct intel_iommu *iommu); >>> - >>> +extern int intel_svm_bind_gpasid(struct iommu_domain *domain, >>> + struct device *dev, struct iommu_gpasid_bind_data >>> *data); +extern int intel_svm_unbind_gpasid(struct device *dev, int >>> pasid); struct svm_dev_ops; >>> >>> struct intel_svm_dev { >>> @@ -690,9 +692,13 @@ struct intel_svm_dev { >>> struct intel_svm { >>> struct mmu_notifier notifier; >>> struct mm_struct *mm; >>> + >>> struct intel_iommu *iommu; >>> int flags; >>> int pasid; >>> + int gpasid; /* Guest PASID in case of vSVA bind with >>> non-identity host >>> + * to guest PASID mapping. >>> + */ >>> struct list_head devs; >>> struct list_head list; >>> }; >>> diff --git a/include/linux/intel-svm.h b/include/linux/intel-svm.h >>> index 94f047a8a845..a2c189ad0b01 100644 >>> --- a/include/linux/intel-svm.h >>> +++ b/include/linux/intel-svm.h >>> @@ -44,6 +44,23 @@ struct svm_dev_ops { >>> * do such IOTLB flushes automatically. >>> */ >>> #define SVM_FLAG_SUPERVISOR_MODE (1<<1) >>> +/* >>> + * The SVM_FLAG_GUEST_MODE flag is used when a guest process bind >>> to a device. >>> + * In this case the mm_struct is in the guest kernel or userspace, >>> its life >>> + * cycle is managed by VMM and VFIO layer. For IOMMU driver, this >>> API provides >>> + * means to bind/unbind guest CR3 with PASIDs allocated for a >>> device. >>> + */ >>> +#define SVM_FLAG_GUEST_MODE (1<<2) >> >> How about keeping this aligned with top by adding a tab? >> > sounds good. >> BIT macro is preferred. Hence, make it BIT(1), BIT(2), BIT(3) is >> preferred. >> > I know, but the existing mainline code is not using BIT, so I wanted > to keep coding style consistent. Perhaps a separate cleanup patch will > do later.
It makes sense to me.
>>> +/* >>> + * The SVM_FLAG_GUEST_PASID flag is used when a guest has its own >>> PASID space, >>> + * which requires guest and host PASID translation at both >>> directions. We keep >>> + * track of guest PASID in order to provide lookup service to >>> device drivers. >>> + * One such example is a physical function (PF) driver that >>> supports mediated >>> + * device (mdev) assignment. Guest programming of mdev >>> configuration space can >>> + * only be done with guest PASID, therefore PF driver needs to >>> find the matching >>> + * host PASID to program the real hardware. >>> + */ >>> +#define SVM_FLAG_GUEST_PASID (1<<3) >> >> Ditto. >> >> Best regards, >> baolu > > [Jacob Pan] >
Best regards, baolu
| |