Received-SPF: pass (google.com: domain of linux-kernel+bounces-203621-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <ae7c05b0-104e-4c71-99aa-fed2a4f209fb@linux.intel.com>
Date: Thu, 6 Jun 2024 13:33:29 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: baolu.lu@linux.intel.com, "iommu@lists.linux.dev"
 <iommu@lists.linux.dev>,
 "virtualization@lists.linux-foundation.org"
 <virtualization@lists.linux-foundation.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 Jason Gunthorpe <jgg@nvidia.com>
Subject: Re: [PATCH v6 01/10] iommu: Introduce domain attachment handle
To: "Tian, Kevin" <kevin.tian@intel.com>, Jason Gunthorpe <jgg@ziepe.ca>,
 Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
 Robin Murphy <robin.murphy@arm.com>,
 Jean-Philippe Brucker <jean-philippe@linaro.org>,
 Nicolin Chen <nicolinc@nvidia.com>, "Liu, Yi L" <yi.l.liu@intel.com>,
 Jacob Pan <jacob.jun.pan@linux.intel.com>,
 Joel Granados <j.granados@samsung.com>
References: <20240527040517.38561-1-baolu.lu@linux.intel.com>
 <20240527040517.38561-2-baolu.lu@linux.intel.com>
 <BN9PR11MB52767B3B1F441D9AA656AC258CF92@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Language: en-US
From: Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <BN9PR11MB52767B3B1F441D9AA656AC258CF92@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 6/5/24 4:02 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Monday, May 27, 2024 12:05 PM
>>
>> @@ -99,7 +99,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device
>> *dev, struct mm_struct *mm
>>
>>   	/* Search for an existing domain. */
>>   	list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) {
>> -		ret = iommu_attach_device_pasid(domain, dev, iommu_mm-
>>> pasid);
>> +		handle->handle.domain = domain;
>> +		ret = iommu_attach_device_pasid(domain, dev, iommu_mm-
>>> pasid,
>> +						&handle->handle);
> 
> move the setting of handle.domain into the helper?

Yes.

> 
>> @@ -3382,11 +3383,9 @@ int iommu_attach_device_pasid(struct
>> iommu_domain *domain,
>>   		}
>>   	}
>>
>> -	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
>> GFP_KERNEL);
>> -	if (curr) {
>> -		ret = xa_err(curr) ? : -EBUSY;
>> +	ret = xa_insert(&group->pasid_array, pasid, handle, GFP_KERNEL);
>> +	if (ret)
>>   		goto out_unlock;
>> -	}
> 
> this leads to a slightly different implication comparing to the old code.
> 
> the domain pointer was always tracked in the old code but now the handle
> is optional.
> 
> if iommu core only needs to know whether a pasid has been attached (as
> in this helper), it still works as xa_insert() will mark the entry as reserved
> if handle==NULL and next xa_insert() at the same index will fail due to
> the entry being reserved.
> 
> But if certain path (other than iopf) in the iommu core needs to know
> the exact domain pointer then this change breaks it.

The iommu core should not fetch the domain pointer in paths other than
attach/detach/replace. There is currently no reference counter for an
iommu domain, hence fetching the domain for other purposes will
potentially lead to a use-after-free issue.

> 
> Anyway some explanation is welcomed here for why this change is safe.
> 
>> @@ -3414,7 +3413,7 @@ void iommu_detach_device_pasid(struct
>> iommu_domain *domain, struct device *dev,
>>
>>   	mutex_lock(&group->mutex);
>>   	__iommu_remove_group_pasid(group, pasid, domain);
>> -	WARN_ON(xa_erase(&group->pasid_array, pasid) != domain);
>> +	xa_erase(&group->pasid_array, pasid);
> 
> if the entry is valid do we want to keep the WARN_ON() upon handle->domain?

The domain pointer has already been passed to the iommu driver in
__iommu_remove_group_pasid(). Therefore, the check for the pointer's
validity should be performed before calling
__iommu_remove_group_pasid(). Hence, in my view, using WARN_ON() around
xa_erase() is not very useful.

Best regards,
baolu