[PATCH rc] iommu/arm-smmu-v3: Do not use GFP_KERNEL under as spinlock

[Will Deacon]

Hi Jason,

On Thu, Feb 08, 2024 at 10:27:29AM -0400, Jason Gunthorpe wrote:
> On Thu, Feb 08, 2024 at 12:58:47PM +0000, Will Deacon wrote:
> > On Fri, Feb 02, 2024 at 10:28:05AM -0400, Jason Gunthorpe wrote:
> > > If the SMMU is configured to use a two level CD table then
> > > arm_smmu_write_ctx_desc() allocates a CD table leaf internally using
> > > GFP_KERNEL. Due to recent changes this is being done under a spinlock to
> > > iterate over the device list - thus it will trigger a sleeping while
> > > atomic warning:
> > > 
> > >   arm_smmu_sva_set_dev_pasid()
> > >     mutex_lock(&sva_lock);
> > >     __arm_smmu_sva_bind()
> > >      arm_smmu_mmu_notifier_get()
> > >       spin_lock_irqsave()
> > >       arm_smmu_write_ctx_desc()
> > > 	arm_smmu_get_cd_ptr()
> > >          arm_smmu_alloc_cd_leaf_table()
> > > 	  dmam_alloc_coherent(GFP_KERNEL)
> > > 
> > > This is a 64K high order allocation and really should not be done
> > > atomically.
> > > 
> > > The proper answer is to sequence CD programming in a way that does not
> > > require the device list or a spinlock. Part two of my lager series does
> > > this and already solves this bug.
> > > 
> > > In the mean time we can solve the functional issue by preallocating the CD
> > > entry outside any locking. CD leafs are effectively never freed so this
> > > guarantees the arm_smmu_write_ctx_desc() inside the lock will not take the
> > > allocation path.
> > > 
> > > Fixes: 24503148c545 ("iommu/arm-smmu-v3: Refactor write_ctx_desc")
> > > Reported-by: Dan Carpenter <dan.carpenter at linaro.org>
> > > Closes: https://lore.kernel.org/all/4e25d161-0cf8-4050-9aa3-dfa21cd63e56@moroto.mountain/
> > > Signed-off-by: Jason Gunthorpe <jgg at nvidia.com>
> > > ---
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 5 +++++
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c     | 2 +-
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h     | 2 ++
> > >  3 files changed, 8 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..068d60732e6481 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
> > > @@ -588,6 +588,11 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
> > >  {
> > >  	int ret = 0;
> > >  	struct mm_struct *mm = domain->mm;
> > > +	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> > > +
> > > +	/* Preallocate the required leafs outside locks */
> > > +	if (!arm_smmu_get_cd_ptr(master, id))
> > > +		return -ENOMEM;
> > 
> > What guarantees that all the masters in the domain share the same l2 CD
> > table?
> 
> Nothing, in fact they don't.
> 
> However, a multi-master RID domain is something that doesn't fully
> work with SVA today. This code is functionally/security wrong to
> install the PASID into all the masters that share the RID domain.
> 
> In practice that doesn't happen because the iommu group size when
> PASID is enabled is restricted to 1 and the DMA API is the thing that
> has created a unique per-group domain to be able to turn SVA on at
> all.
> 
> So, the RID domain's list has one master entry that is the same as the
> set_dev_pasid's master.
> 
> Which is why this patch works, it prealloctes the only master that
> matters for the only configuration where SVA works and is used today.
> 
> iommufd could violate these assumptions which is part of why I'm
> fixing all of this in preparation to enable PASID support for all
> drivers in iommufd.

Damn, all the surrounding rework has really left this poor code in a bit
of a state. Could you please respin your fix, but with an additional change
to arm_smmu_mmu_notifier_get() so that it returns an error if there is
more than one device in the domain, with some of the above in a comment?

Cheers,

Will