linux-kernel - Re: [PATCH 10/10] iommu/vt-d: Register PASID notifier for status change

Open Source and information security mailing list archives

Message-ID: <20200327164701.5cfee1c3@jacob-builder>
Date:   Fri, 27 Mar 2020 16:47:01 -0700
From:   Jacob Pan <jacob.jun.pan@...ux.intel.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>
Cc:     Joerg Roedel <joro@...tes.org>,
        Alex Williamson <alex.williamson@...hat.com>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        David Woodhouse <dwmw2@...radead.org>,
        Jean-Philippe Brucker <jean-philippe@...aro.com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        Jonathan Cameron <jic23@...nel.org>,
        jacob.jun.pan@...ux.intel.com
Subject: Re: [PATCH 10/10] iommu/vt-d: Register PASID notifier for status
 change

On Fri, 27 Mar 2020 10:22:57 +0000
"Tian, Kevin" <kevin.tian@...el.com> wrote:

> > From: Jacob Pan
> > Sent: Thursday, March 26, 2020 1:56 AM
> > 
> > In bare metal SVA, IOMMU driver ensures that IOASID free call
> > always comes after IOASID unbind operation.
> > 
> > However, for guest SVA the unbind and free call come from user space
> > via VFIO, which could be out of order. This patch registers a
> > notifier block in case IOASID free() comes before unbind such that
> > VT-d driver can take action to clean up PASID context and data.  
> 
> clearly the patch includes more than above usage. It also handles the
> bind usage to notify KVM for setting PASID translation table.
> 
> > 
> > Signed-off-by: Liu Yi L <yi.l.liu@...el.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@...ux.intel.com>
> > ---
> >  drivers/iommu/intel-svm.c   | 68
> > ++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/intel-iommu.h |  1 +
> >  2 files changed, 68 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> > index f511855d187b..779dd2c6f9e1 100644
> > --- a/drivers/iommu/intel-svm.c
> > +++ b/drivers/iommu/intel-svm.c
> > @@ -23,6 +23,7 @@
> >  #include "intel-pasid.h"
> > 
> >  static irqreturn_t prq_event_thread(int irq, void *d);
> > +static DEFINE_MUTEX(pasid_mutex);
> > 
> >  #define PRQ_ORDER 0
> > 
> > @@ -92,6 +93,65 @@ static inline bool intel_svm_capable(struct
> > intel_iommu *iommu)
> >  	return iommu->flags & VTD_FLAG_SVM_CAPABLE;
> >  }
> > 
> > +#define pasid_lock_held() lock_is_held(&pasid_mutex.dep_map)
> > +
> > +static int pasid_status_change(struct notifier_block *nb,
> > +				unsigned long code, void *data)
> > +{
> > +	struct ioasid_nb_args *args = (struct ioasid_nb_args
> > *)data;
> > +	struct intel_svm_dev *sdev;
> > +	struct intel_svm *svm;
> > +	int ret = NOTIFY_DONE;
> > +
> > +	if (code == IOASID_FREE) {
> > +		/*
> > +		 * Unbind all devices associated with this PASID
> > which is
> > +		 * being freed by other users such as VFIO.
> > +		 */
> > +		mutex_lock(&pasid_mutex);
> > +		svm = ioasid_find(INVALID_IOASID_SET, args->id,
> > NULL);
> > +		if (!svm || !svm->iommu)
> > +			goto done_unlock;  
> 
> should we treat !svm->iommu as an error condition? if not, do you have
> an example when it may occur in normal situation?
> 
Right, should be an error. Initially, unbind could retrieve iommu from
struct device, but with notifier we have to set svm->iommu all the time.

> > +
> > +		if (IS_ERR(svm)) {
> > +			ret = NOTIFY_BAD;
> > +			goto done_unlock;
> > +		}  
> 
> svm->iommu should be referenced after IS_ERR check
> 
Good point, will move up.
> > +
> > +		list_for_each_entry_rcu(sdev, &svm->devs, list,
> > pasid_lock_held()) {
> > +			/* Does not poison forward pointer */
> > +			list_del_rcu(&sdev->list);
> > +			intel_pasid_tear_down_entry(svm->iommu,
> > sdev-  
> > >dev,  
> > +						    svm->pasid);
> > +			kfree_rcu(sdev, rcu);
> > +
> > +			/*
> > +			 * Free before unbind only happens with
> > guest usaged
> > +			 * host PASIDs. IOASID free will detach
> > private data
> > +			 * and free the IOASID entry.  
> 
> "guest usaged host PASIDs"?
> 
I mean VM used PASID, will change to
/*
 * Free before unbind only happens with PASIDs used
 * by VM. IOASID free will detach private data
 * and free the IOASID entry.
 */

> > +			 */
> > +			if (list_empty(&svm->devs))
> > +				kfree(svm);
> > +		}
> > +		mutex_unlock(&pasid_mutex);
> > +
> > +		return NOTIFY_OK;
> > +	}
> > +
> > +done_unlock:
> > +	mutex_unlock(&pasid_mutex);
> > +	return ret;
> > +}
> > +
> > +static struct notifier_block pasid_nb = {
> > +		.notifier_call = pasid_status_change,
> > +};
> > +
> > +void intel_svm_add_pasid_notifier(void)
> > +{
> > +	ioasid_add_notifier(&pasid_nb);
> > +}
> > +
> >  void intel_svm_check(struct intel_iommu *iommu)
> >  {
> >  	if (!pasid_supported(iommu))
> > @@ -219,7 +279,6 @@ static const struct mmu_notifier_ops
> > intel_mmuops = {
> >  	.invalidate_range = intel_invalidate_range,
> >  };
> > 
> > -static DEFINE_MUTEX(pasid_mutex);
> >  static LIST_HEAD(global_svm_list);
> > 
> >  #define for_each_svm_dev(sdev, svm, d)			\
> > @@ -319,6 +378,7 @@ int intel_svm_bind_gpasid(struct iommu_domain
> > *domain,
> >  			svm->gpasid = data->gpasid;
> >  			svm->flags |= SVM_FLAG_GUEST_PASID;
> >  		}
> > +		svm->iommu = iommu;  
> 
> ah, it's interesting to see we have a field defined before but never
> used. 😊
> 
The unbind call can retrieve iommu from struct device.

> > 
> >  		ioasid_attach_data(data->hpasid, svm);
> >  		INIT_LIST_HEAD_RCU(&svm->devs);
> > @@ -383,6 +443,11 @@ int intel_svm_bind_gpasid(struct iommu_domain
> > *domain,
> >  	}
> >  	svm->flags |= SVM_FLAG_GUEST_MODE;
> > 
> > +	/*
> > +	 * Notify KVM new host-guest PASID bind is ready. KVM will
> > set up
> > +	 * PASID translation table to support guest ENQCMD.
> > +	 */  
> 
> should take it as an example instead of the only possible usage.
> 
True, we don;t know who is getting notified.

> > +	ioasid_notify(data->hpasid, IOASID_BIND);
> >  	init_rcu_head(&sdev->rcu);
> >  	list_add_rcu(&sdev->list, &svm->devs);
> >   out:
> > @@ -440,6 +505,7 @@ int intel_svm_unbind_gpasid(struct device *dev,
> > int pasid)
> >  				 * used by another.
> >  				 */
> >  				ioasid_attach_data(pasid, NULL);
> > +				ioasid_notify(pasid,
> > IOASID_UNBIND); kfree(svm);
> >  			}
> >  		}
> > diff --git a/include/linux/intel-iommu.h
> > b/include/linux/intel-iommu.h index f8504a980981..64799067ea58
> > 100644 --- a/include/linux/intel-iommu.h
> > +++ b/include/linux/intel-iommu.h
> > @@ -708,6 +708,7 @@ extern struct iommu_sva *
> >  intel_svm_bind(struct device *dev, struct mm_struct *mm, void
> > *drvdata); extern void intel_svm_unbind(struct iommu_sva *handle);
> >  extern int intel_svm_get_pasid(struct iommu_sva *handle);
> > +extern void intel_svm_add_pasid_notifier(void);
> > 
> >  struct svm_dev_ops;
> > 
> > --
> > 2.7.4
> > 
> > _______________________________________________
> > iommu mailing list
> > iommu@...ts.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu  

[Jacob Pan]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives