Re: [PATCH v5 11/14] iommu/amd: Introduce gDomID-to-hDomID Mapping and handle parent domain invalidation

[Suthikulpanit, Suravee]

On 11/19/2025 7:02 AM, Jason Gunthorpe wrote:
> On Thu, Nov 13, 2025 at 12:36:07PM -0800, Nicolin Chen wrote:
> > > +	curr = xa_cmpxchg(&aviommu->gdomid_array,
> > > +			  ndom->gdom_id, NULL, gdom_info, GFP_ATOMIC);
> > > +	if (curr) {
> > > +		if (xa_err(curr)) {
> > > +			ret = -EINVAL;
> > > +			goto out_err_gdom_info;
> > > +		} else {
> > > +			/* The gDomID already exist */
> > > +			pr_debug("%s: Found gdom_id=%#x, hdom_id=%#x\n",
> > > +				 __func__, ndom->gdom_id, curr->hdom_id);
> > > +			refcount_inc(&curr->users);
> > > +			ndom->gdom_info = curr;
> >
> > This looks racy..
>
> Yes
>
> > When a gDomID is shared between two nested domains, a concurrent
> > nested_domain_free() could enter before refcount_inc(), and call
> > refcount_dec_and_test() or even free the curr and ndom.
> >
> > Then, this refcount_inc() will blow up, or curr/ndom will UAF.
> >
> > Actually, I don't see where amd_iommu_alloc_domain_nested() gets
> > used in this series.. I assume AMD will use the iommufd's vIOMMU
> > infrastructure directly which doesn't mutex across nested domain
> > allocation/free calls.
> >
> > So, the entire thing here should hold xa_lock(), use xas_load()
> > for the existing curr and use xas_store() to store gdom_info if
> > !curr, and xa_unlock() after gdom_info is fully initialized.
>
> No need for xas functions.. You can use the __ functions..
>
> A helper function like this will do the job:
>
> static void *xa_load_or_alloc_locked(struct xarray *xa, unsigned long index, size_t sz)
> {
>          void *elm, *res;
>
>          elm = xa_load(xa, index);
>          if (elm)
>                  return elm;
>
> 	xa_unlock(xa);
>          elm = kzalloc(sz, GFP_KERNEL);
> 	xa_lock(xa);
>          if (!elm)
>                  return ERR_PTR(-ENOMEM);
>
>          res = __xa_cmpxchg(xa, index, NULL, elm, GFP_KERNEL);
>          if (xa_is_err(res))
>                  res = ERR_PTR(xa_err(res));
>
>          if (res) {
>                  kfree(elm);
>                  return res;
>          }
>
>          return elm;
> }
>
> Call like
>
>   xa_lock(&aviommu->gdomid_array);
>   elm = *xa_load_or_alloc_locked(..)
>   if (IS_ERR(elm))
>     ..
>   elm->refcount++;
>   xa_unlock(&aviommu->gdomid_array);
>
> Needs more bits if you want to use refcount_t

Thanks for the guide. I have rework this and sent out V6.

Suravee