Re: [PATCH v2 4/4] iommu: Swap the order of setting group->pasid_array and calling attach op of iommu drivers

[Yi Liu <yi.l.liu@intel.com>]

On 2025/2/22 01:27, Nicolin Chen wrote:
> On Fri, Feb 21, 2025 at 06:33:35AM -0800, Yi Liu wrote:
>> The current implementation stores entry to the group->pasid_array before
>> the underlying iommu driver has successfully set the new domain. This can
>> lead to issues where PRIs are received on the new domain before the attach
>> operation is completed.
>>
>> This patch swaps the order of operations to ensure that the domain is set
>> in the underlying iommu driver before updating the group->pasid_array.
>>
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> 
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> A nit:
> 
>> ---
>>   drivers/iommu/iommu.c | 44 +++++++++++++++++++++++++++++++------------
>>   1 file changed, 32 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index eff5f678883b..73555e1cf016 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3388,13 +3388,25 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>   
>>   	entry = iommu_make_pasid_array_entry(domain, handle);
>>   
>> -	ret = xa_insert(&group->pasid_array, pasid, entry, GFP_KERNEL);
>> +	ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, GFP_KERNEL);
> 
> Maybe xa_reserve() pairing xa_release()? Same thing though..

there is slight difference. xa_reserve() will return 0 if there is already
a valid entry. xa_insert() shall fail with EBUSY. We want to catch such
failures here. You may notice that the replace function uses xa_reserve()
as it intend to replace existing entry.

>>   	if (ret)
>>   		goto out_unlock;
>>   
>>   	ret = __iommu_set_group_pasid(domain, group, pasid);
>> -	if (ret)
>> -		xa_erase(&group->pasid_array, pasid);
>> +	if (ret) {
>> +		xa_release(&group->pasid_array, pasid);
>> +		goto out_unlock;
>> +	}
>> +
>> +	/*
>> +	 * The xa_insert() above reserved the memory, and the group->mutex is
> 
> Just in case we would like update the patch, reword "xa_insert".
> 
>> +	 * held, this cannot fail. The new domain cannot be visible until the
>> +	 * operation succeeds as we cannot tolerate PRIs becoming concurrently
>> +	 * queued and then failing attach.
>> +	 */
>> +	WARN_ON(xa_is_err(xa_store(&group->pasid_array,
>> +				   pasid, entry, GFP_KERNEL)));
>> +
>>   out_unlock:
>>   	mutex_unlock(&group->mutex);
>>   	return ret;
>> @@ -3509,19 +3521,27 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
>>   
>>   	mutex_lock(&group->mutex);
>>   	entry = iommu_make_pasid_array_entry(domain, handle);
>> -	ret = xa_insert(&group->pasid_array, IOMMU_NO_PASID, entry, GFP_KERNEL);
>> +	ret = xa_insert(&group->pasid_array,
>> +			IOMMU_NO_PASID, XA_ZERO_ENTRY, GFP_KERNEL);
> 
> Ditto.
> 
>>   	if (ret)
>> -		goto err_unlock;
>> +		goto out_unlock;
>>   
>>   	ret = __iommu_attach_group(domain, group);
>> -	if (ret)
>> -		goto err_erase;
>> -	mutex_unlock(&group->mutex);
>> +	if (ret) {
>> +		xa_release(&group->pasid_array, IOMMU_NO_PASID);
>> +		goto out_unlock;
>> +	}
>>   
>> -	return 0;
>> -err_erase:
>> -	xa_erase(&group->pasid_array, IOMMU_NO_PASID);
>> -err_unlock:
>> +	/*
>> +	 * The xa_insert() above reserved the memory, and the group->mutex is
> 
> "xa_insert" here too.
> 
> Thanks
> Nicolin
> 

-- 
Regards,
Yi Liu