diff mbox series

[v4,01/27] iommu/arm-smmu-v3: Check that the RID domain is S1 in SVA

Message ID 1-v4-e7091cdd9e8d+43b1-smmuv3_newapi_p2_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Update SMMUv3 to the modern iommu API (part 2/3) | expand

Commit Message

Jason Gunthorpe Jan. 26, 2024, 6:15 p.m. UTC 
This code only works if the RID domain is a S1 domain and has already
installed the cdtable.

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Shameerali Kolothum Thodi Jan. 30, 2024, 8:46 a.m. UTC | #1 
> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, January 26, 2024 6:15 PM
> To: iommu@lists.linux.dev; Joerg Roedel <joro@8bytes.org>; linux-arm-
> kernel@lists.infradead.org; Robin Murphy <robin.murphy@arm.com>; Will
> Deacon <will@kernel.org>
> Cc: Eric Auger <eric.auger@redhat.com>; Jean-Philippe Brucker <jean-
> philippe@linaro.org>; Moritz Fischer <mdf@kernel.org>; Michael Shavit
> <mshavit@google.com>; Nicolin Chen <nicolinc@nvidia.com>;
> patches@lists.linux.dev; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> Subject: [PATCH v4 01/27] iommu/arm-smmu-v3: Check that the RID domain is
> S1 in SVA
> 
> This code only works if the RID domain is a S1 domain and has already
> installed the cdtable.
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index 05722121f00e70..b4549d698f3569 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -387,7 +387,13 @@ static int __arm_smmu_sva_bind(struct device *dev,
> struct mm_struct *mm)
>  	struct arm_smmu_bond *bond;
>  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>  	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> -	struct arm_smmu_domain *smmu_domain =
> to_smmu_domain(domain);
> +	struct arm_smmu_domain *smmu_domain;
> +
> +	if (!(domain->type & __IOMMU_DOMAIN_PAGING))
> +		return -ENODEV;
> +	smmu_domain = to_smmu_domain(iommu_get_domain_for_dev(dev));

We already have the iommu_domain from above.

> +	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> +		return -ENODEV;

I think we need to do the above checks in arm_smmu_sva_remove_dev_pasid()
as well.

Thanks,
Shameer
Jason Gunthorpe Jan. 30, 2024, 4:04 p.m. UTC | #2 
On Tue, Jan 30, 2024 at 08:46:10AM +0000, Shameerali Kolothum Thodi wrote:

> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > index 05722121f00e70..b4549d698f3569 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > @@ -387,7 +387,13 @@ static int __arm_smmu_sva_bind(struct device *dev,
> > struct mm_struct *mm)
> >  	struct arm_smmu_bond *bond;
> >  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> >  	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> > -	struct arm_smmu_domain *smmu_domain =
> > to_smmu_domain(domain);
> > +	struct arm_smmu_domain *smmu_domain;
> > +
> > +	if (!(domain->type & __IOMMU_DOMAIN_PAGING))
> > +		return -ENODEV;
> > +	smmu_domain = to_smmu_domain(iommu_get_domain_for_dev(dev));
> 
> We already have the iommu_domain from above.

Yep
 
> > +	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> > +		return -ENODEV;
> 
> I think we need to do the above checks in arm_smmu_sva_remove_dev_pasid()
> as well.

The core won't call that unless a PASID is already attached, meaning
we already passed the above check in bind. So it shouldn't need to be
duplicated.

Further, at this instant all the RID attach paths are protected by:

	if (arm_smmu_master_sva_enabled(master))
		return -EBUSY;

Dropping the S1 check was a mistake introduced in commit 386fa64fd52b
("arm-smmu-v3/sva: Add SVA domain support")

The current logic for SMMUv3 requires that a calling driver assert the
SVA feature, which locks the STE, and then if the STE was a S1 you can
install a SVA PASID.

Thanks,
Jason
Shameerali Kolothum Thodi Jan. 30, 2024, 5:11 p.m. UTC | #3 
> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, January 30, 2024 4:05 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: iommu@lists.linux.dev; Joerg Roedel <joro@8bytes.org>; linux-arm-
> kernel@lists.infradead.org; Robin Murphy <robin.murphy@arm.com>; Will
> Deacon <will@kernel.org>; Eric Auger <eric.auger@redhat.com>; Jean-
> Philippe Brucker <jean-philippe@linaro.org>; Moritz Fischer
> <mdf@kernel.org>; Michael Shavit <mshavit@google.com>; Nicolin Chen
> <nicolinc@nvidia.com>; patches@lists.linux.dev
> Subject: Re: [PATCH v4 01/27] iommu/arm-smmu-v3: Check that the RID
> domain is S1 in SVA
> 
> On Tue, Jan 30, 2024 at 08:46:10AM +0000, Shameerali Kolothum Thodi
> wrote:
> 
> > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > > index 05722121f00e70..b4549d698f3569 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> > > @@ -387,7 +387,13 @@ static int __arm_smmu_sva_bind(struct device
> *dev,
> > > struct mm_struct *mm)
> > >  	struct arm_smmu_bond *bond;
> > >  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> > >  	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> > > -	struct arm_smmu_domain *smmu_domain =
> > > to_smmu_domain(domain);
> > > +	struct arm_smmu_domain *smmu_domain;
> > > +
> > > +	if (!(domain->type & __IOMMU_DOMAIN_PAGING))
> > > +		return -ENODEV;
> > > +	smmu_domain =
> to_smmu_domain(iommu_get_domain_for_dev(dev));
> >
> > We already have the iommu_domain from above.
> 
> Yep
> 
> > > +	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> > > +		return -ENODEV;
> >
> > I think we need to do the above checks in
> arm_smmu_sva_remove_dev_pasid()
> > as well.
> 
> The core won't call that unless a PASID is already attached, meaning
> we already passed the above check in bind. So it shouldn't need to be
> duplicated.

I think it does in the error path.  See __iommu_set_group_pasid() call.
I haven't tested what happens if that returns error because of identity 
domain and then __iommu_remove_group_pasid() is called.

So as an exported function, still think it is better to check for domain
type before accessing smmu_domain there.

Thanks,
Shameer
Jason Gunthorpe Jan. 30, 2024, 7:03 p.m. UTC | #4 
On Tue, Jan 30, 2024 at 05:11:36PM +0000, Shameerali Kolothum Thodi wrote:
> > 
> > > > +	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> > > > +		return -ENODEV;
> > >
> > > I think we need to do the above checks in
> > arm_smmu_sva_remove_dev_pasid()
> > > as well.
> > 
> > The core won't call that unless a PASID is already attached, meaning
> > we already passed the above check in bind. So it shouldn't need to be
> > duplicated.
> 
> I think it does in the error path.  See __iommu_set_group_pasid() call.
> I haven't tested what happens if that returns error because of identity 
> domain and then __iommu_remove_group_pasid() is called.

You are correct, but this is a problem in the core code :(

Driver should not have to deal with this unbalance. A set/remove
pairing should be enforced by the core.

--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3481,15 +3481,24 @@ EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
 static int __iommu_set_group_pasid(struct iommu_domain *domain,
                                   struct iommu_group *group, ioasid_t pasid)
 {
+       struct group_device *last_gdev;
        struct group_device *device;
        int ret = 0;
 
        for_each_group_device(group, device) {
                ret = domain->ops->set_dev_pasid(domain, device->dev, pasid);
                if (ret)
-                       break;
+                       goto err_revert;
        }
+       return 0;
 
+err_revert:
+       last_gdev = device;
+       for_each_group_device(group, device) {
+               if (device == last_gdev)
+                       break;
+               dev_iommu_ops(device->dev)->remove_dev_pasid(device->dev, pasid);
+       }
        return ret;
 }
 
@@ -3538,10 +3547,8 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
        }
 
        ret = __iommu_set_group_pasid(domain, group, pasid);
-       if (ret) {
-               __iommu_remove_group_pasid(group, pasid);
+       if (ret)
                xa_erase(&group->pasid_array, pasid);
-       }
 out_unlock:
        mutex_unlock(&group->mutex);
        return ret;

> So as an exported function, still think it is better to check for domain
> type before accessing smmu_domain there.

After part 2 the remove path doesn't touch the RID so I prefer to
leave it in part 1 and it will be fully safe in part 2. It is just
more code that part 2 has to remove.

Thanks,
Jason
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 05722121f00e70..b4549d698f3569 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -387,7 +387,13 @@  static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 	struct arm_smmu_bond *bond;
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct arm_smmu_domain *smmu_domain;
+
+	if (!(domain->type & __IOMMU_DOMAIN_PAGING))
+		return -ENODEV;
+	smmu_domain = to_smmu_domain(iommu_get_domain_for_dev(dev));
+	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
+		return -ENODEV;
 
 	if (!master || !master->sva_enabled)
 		return -ENODEV;