RE: [PATCH v5 07/15] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
Hi Eric,
> From: Auger Eric
> Sent: Sunday, July 19, 2020 11:39 PM
>
> Yi,
>
> On 7/12/20 1:21 PM, Liu Yi L wrote:
> > This patch allows user space to request PASID allocation/free, e.g.
> > when serving the request from the guest.
> >
> > PASIDs that are not freed by userspace are automatically freed when
> > the IOASID set is destroyed when process exits.
> >
> > Cc: Kevin Tian
> > CC: Jacob Pan
> > Cc: Alex Williamson
> > Cc: Eric Auger
> > Cc: Jean-Philippe Brucker
> > Cc: Joerg Roedel
> > Cc: Lu Baolu
> > Signed-off-by: Liu Yi L
> > Signed-off-by: Yi Sun
> > Signed-off-by: Jacob Pan
> > ---
> > v4 -> v5:
> > *) address comments from Eric Auger.
> > *) the comments for the PASID_FREE request is addressed in patch 5/15 of
> >this series.
> >
> > v3 -> v4:
> > *) address comments from v3, except the below comment against the range
> >of PASID_FREE request. needs more help on it.
> > "> +if (req.range.min > req.range.max)
> >
> > Is it exploitable that a user can spin the kernel for a long time in
> > the case of a free by calling this with [0, MAX_UINT] regardless of
> > their actual allocations?"
> >
> > https://lore.kernel.org/linux-iommu/20200702151832.048b4...@x1.home/
> >
> > v1 -> v2:
> > *) move the vfio_mm related code to be a seprate module
> > *) use a single structure for alloc/free, could support a range of
> > PASIDs
> > *) fetch vfio_mm at group_attach time instead of at iommu driver open
> > time
> > ---
> > drivers/vfio/Kconfig| 1 +
> > drivers/vfio/vfio_iommu_type1.c | 85
> +
> > drivers/vfio/vfio_pasid.c | 10 +
> > include/linux/vfio.h| 6 +++
> > include/uapi/linux/vfio.h | 37 ++
> > 5 files changed, 139 insertions(+)
> >
> > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig index
> > 3d8a108..95d90c6 100644
> > --- a/drivers/vfio/Kconfig
> > +++ b/drivers/vfio/Kconfig
> > @@ -2,6 +2,7 @@
> > config VFIO_IOMMU_TYPE1
> > tristate
> > depends on VFIO
> > + select VFIO_PASID if (X86)
> > default n
> >
> > config VFIO_IOMMU_SPAPR_TCE
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > b/drivers/vfio/vfio_iommu_type1.c index ed80104..55b4065 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -76,6 +76,7 @@ struct vfio_iommu {
> > booldirty_page_tracking;
> > boolpinned_page_dirty_scope;
> > struct iommu_nesting_info *nesting_info;
> > + struct vfio_mm *vmm;
> > };
> >
> > struct vfio_domain {
> > @@ -1937,6 +1938,11 @@ static void vfio_iommu_iova_insert_copy(struct
> > vfio_iommu *iommu,
> >
> > static void vfio_iommu_release_nesting_info(struct vfio_iommu *iommu)
> > {
> > + if (iommu->vmm) {
> > + vfio_mm_put(iommu->vmm);
> > + iommu->vmm = NULL;
> > + }
> > +
> > kfree(iommu->nesting_info);
> > iommu->nesting_info = NULL;
> > }
> > @@ -2071,6 +2077,26 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> > iommu->nesting_info);
> > if (ret)
> > goto out_detach;
> > +
> > + if (iommu->nesting_info->features &
> > + IOMMU_NESTING_FEAT_SYSWIDE_PASID)
> {
> > + struct vfio_mm *vmm;
> > + int sid;
> > +
> > + vmm = vfio_mm_get_from_task(current);
> > + if (IS_ERR(vmm)) {
> > + ret = PTR_ERR(vmm);
> > + goto out_detach;
> > + }
> > + iommu->vmm = vmm;
> > +
> > + sid = vfio_mm_ioasid_sid(vmm);
> > + ret = iommu_domain_set_attr(domain->domain,
> > + DOMAIN_ATTR_IOASID_SID,
> > + );
> > + if (ret)
> > + goto out_detach;
> > + }
> > }
> >
> > /* Get aperture info */
> > @@ -2855,6 +2881,63 @@ static int vfio_iommu_type1_dirty_pages(struct
> vfio_iommu *iommu,
> > return -EINVAL;
> > }
> >
> > +static int vfio_iommu_type1_pasid_alloc(struct vfio_iommu *iommu,
> > + unsigned int min,
> > + unsigned int max)
> > +{
> > + int ret = -EOPNOTSUPP;
> > +
> > + mutex_lock(>lock);
> > + if (iommu->vmm)
> > + ret = vfio_pasid_alloc(iommu->vmm, min, max);
> > + mutex_unlock(>lock);
> > + return ret;
> > +}
> > +
> > +static int vfio_iommu_type1_pasid_free(struct vfio_iommu *iommu,
> > + unsigned int min,
> > + unsigned int max)
> > +{
> > + int ret = -EOPNOTSUPP;
> > +
> > + mutex_lock(>lock);
> > + if (iommu->vmm) {
> > +
Re: [PATCH v5 07/15] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
Yi,
On 7/12/20 1:21 PM, Liu Yi L wrote:
> This patch allows user space to request PASID allocation/free, e.g. when
> serving the request from the guest.
>
> PASIDs that are not freed by userspace are automatically freed when the
> IOASID set is destroyed when process exits.
>
> Cc: Kevin Tian
> CC: Jacob Pan
> Cc: Alex Williamson
> Cc: Eric Auger
> Cc: Jean-Philippe Brucker
> Cc: Joerg Roedel
> Cc: Lu Baolu
> Signed-off-by: Liu Yi L
> Signed-off-by: Yi Sun
> Signed-off-by: Jacob Pan
> ---
> v4 -> v5:
> *) address comments from Eric Auger.
> *) the comments for the PASID_FREE request is addressed in patch 5/15 of
>this series.
>
> v3 -> v4:
> *) address comments from v3, except the below comment against the range
>of PASID_FREE request. needs more help on it.
> "> +if (req.range.min > req.range.max)
>
> Is it exploitable that a user can spin the kernel for a long time in
> the case of a free by calling this with [0, MAX_UINT] regardless of
> their actual allocations?"
> https://lore.kernel.org/linux-iommu/20200702151832.048b4...@x1.home/
>
> v1 -> v2:
> *) move the vfio_mm related code to be a seprate module
> *) use a single structure for alloc/free, could support a range of PASIDs
> *) fetch vfio_mm at group_attach time instead of at iommu driver open time
> ---
> drivers/vfio/Kconfig| 1 +
> drivers/vfio/vfio_iommu_type1.c | 85
> +
> drivers/vfio/vfio_pasid.c | 10 +
> include/linux/vfio.h| 6 +++
> include/uapi/linux/vfio.h | 37 ++
> 5 files changed, 139 insertions(+)
>
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index 3d8a108..95d90c6 100644
> --- a/drivers/vfio/Kconfig
> +++ b/drivers/vfio/Kconfig
> @@ -2,6 +2,7 @@
> config VFIO_IOMMU_TYPE1
> tristate
> depends on VFIO
> + select VFIO_PASID if (X86)
> default n
>
> config VFIO_IOMMU_SPAPR_TCE
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index ed80104..55b4065 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -76,6 +76,7 @@ struct vfio_iommu {
> booldirty_page_tracking;
> boolpinned_page_dirty_scope;
> struct iommu_nesting_info *nesting_info;
> + struct vfio_mm *vmm;
> };
>
> struct vfio_domain {
> @@ -1937,6 +1938,11 @@ static void vfio_iommu_iova_insert_copy(struct
> vfio_iommu *iommu,
>
> static void vfio_iommu_release_nesting_info(struct vfio_iommu *iommu)
> {
> + if (iommu->vmm) {
> + vfio_mm_put(iommu->vmm);
> + iommu->vmm = NULL;
> + }
> +
> kfree(iommu->nesting_info);
> iommu->nesting_info = NULL;
> }
> @@ -2071,6 +2077,26 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> iommu->nesting_info);
> if (ret)
> goto out_detach;
> +
> + if (iommu->nesting_info->features &
> + IOMMU_NESTING_FEAT_SYSWIDE_PASID) {
> + struct vfio_mm *vmm;
> + int sid;
> +
> + vmm = vfio_mm_get_from_task(current);
> + if (IS_ERR(vmm)) {
> + ret = PTR_ERR(vmm);
> + goto out_detach;
> + }
> + iommu->vmm = vmm;
> +
> + sid = vfio_mm_ioasid_sid(vmm);
> + ret = iommu_domain_set_attr(domain->domain,
> + DOMAIN_ATTR_IOASID_SID,
> + );
> + if (ret)
> + goto out_detach;
> + }
> }
>
> /* Get aperture info */
> @@ -2855,6 +2881,63 @@ static int vfio_iommu_type1_dirty_pages(struct
> vfio_iommu *iommu,
> return -EINVAL;
> }
>
> +static int vfio_iommu_type1_pasid_alloc(struct vfio_iommu *iommu,
> + unsigned int min,
> + unsigned int max)
> +{
> + int ret = -EOPNOTSUPP;
> +
> + mutex_lock(>lock);
> + if (iommu->vmm)
> + ret = vfio_pasid_alloc(iommu->vmm, min, max);
> + mutex_unlock(>lock);
> + return ret;
> +}
> +
> +static int vfio_iommu_type1_pasid_free(struct vfio_iommu *iommu,
> +unsigned int min,
> +unsigned int max)
> +{
> + int ret = -EOPNOTSUPP;
> +
> + mutex_lock(>lock);
> + if (iommu->vmm) {
> + vfio_pasid_free_range(iommu->vmm, min, max);
> + ret = 0;
> + }
> + mutex_unlock(>lock);
> + return ret;
> +}
> +
> +static int vfio_iommu_type1_pasid_request(struct vfio_iommu *iommu,
> +
