Re: [PATCH v10 02/18] iommu: Introduce a replace API for device pasid

[Nicolin Chen <nicolinc@nvidia.com>]

On Fri, Mar 21, 2025 at 07:58:51AM +0800, Yi Liu wrote:
> > > +	entry = iommu_make_pasid_array_entry(domain, handle);
> > > +	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
> > > +			  XA_ZERO_ENTRY, GFP_KERNEL);
> > > +	if (xa_is_err(curr)) {
> > > +		ret = xa_err(curr);
> > > +		goto out_unlock;
> > > +	}
> > > +
> > > +	/*
> > > +	 * No domain (with or without handle) attached, hence not
> > > +	 * a replace case.
> > > +	 */
> > > +	if (!curr) {
> > > +		xa_release(&group->pasid_array, pasid);
> > > +		ret = -EINVAL;
> > > +		goto out_unlock;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Reusing handle is problematic as there are paths that refers
> > > +	 * the handle without lock. To avoid race, reject the callers that
> > > +	 * attempt it.
> > > +	 */
> > > +	if (handle && curr == entry) {
> > > +		WARN_ON(1);
> > > +		ret = -EINVAL;
> > > +		goto out_unlock;
> > > +	}
> > 
> > We rejected !handle and !curr cases. So it should be enough with:
> > 	if (curr == entry) {
> > ?
> 
> we only want to reject the case in which handle is valid and it is
> the same with old handle. If handle is null, I think it is ok to have the
> same domain.

But !handle is already rejected in the 3rd if validation:
+	if (dev_iommu_ops(dev) != domain->owner ||
+	    pasid == IOMMU_NO_PASID || !handle)
+		return -EINVAL;

So, it can't be NULL at this line, right?

Nic