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

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----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