[PATCH v8 07/12] iommu/arm-smmu-v3: Share process page tables

[Jean-Philippe Brucker]

On Mon, Jul 13, 2020 at 09:22:37PM +0100, Will Deacon wrote:
> > +static struct arm_smmu_ctx_desc *arm_smmu_share_asid(u16 asid)
> > +{
> > +	struct arm_smmu_ctx_desc *cd;
> >  
> > -	xa_erase(&asid_xa, cd->asid);
> > +	cd = xa_load(&asid_xa, asid);
> > +	if (!cd)
> > +		return NULL;
> > +
> > +	if (cd->mm) {
> > +		/* All devices bound to this mm use the same cd struct. */
> > +		refcount_inc(&cd->refs);
> > +		return cd;
> > +	}
> 
> How do you handle racing against a concurrent arm_smmu_free_asid() here?

Patch 8 adds an asid_lock to deal with this, but it should be introduced
in this patch. There is a potential use-after-free here, if
arm_smmu_domain_free() runs concurrently.

> 
> > +__maybe_unused
> > +static struct arm_smmu_ctx_desc *arm_smmu_alloc_shared_cd(struct mm_struct *mm)
> > +{
> > +	u16 asid;
> > +	int ret = 0;
> > +	u64 tcr, par, reg;
> > +	struct arm_smmu_ctx_desc *cd;
> > +	struct arm_smmu_ctx_desc *old_cd = NULL;
> > +
> > +	lockdep_assert_held(&sva_lock);
> 
> Please don't bother with these for static functions (but I can see the
> value in having them for functions with external callers).
> 
> > +
> > +	asid = mm_context_get(mm);
> > +	if (!asid)
> > +		return ERR_PTR(-ESRCH);
> > +
> > +	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> > +	if (!cd) {
> > +		ret = -ENOMEM;
> > +		goto err_put_context;
> > +	}
> > +
> > +	arm_smmu_init_cd(cd);
> > +
> > +	old_cd = arm_smmu_share_asid(asid);
> > +	if (IS_ERR(old_cd)) {
> > +		ret = PTR_ERR(old_cd);
> > +		goto err_free_cd;
> > +	} else if (old_cd) {
> 
> Don't need the 'else'
> 
> > +		if (WARN_ON(old_cd->mm != mm)) {
> > +			ret = -EINVAL;
> > +			goto err_free_cd;
> > +		}
> > +		kfree(cd);
> > +		mm_context_put(mm);
> > +		return old_cd;
> 
> This is a bit messy. Can you consolidate the return path so that ret is a
> pointer and you have an 'int err', e.g.:
> 
> 	return err < 0 ? ERR_PTR(err) : ret;

Sure, I think it looks a little nicer this way

Thanks,
Jean