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
