Re: [PATCH v6 01/14] iommu: Introduce a replace API for device pasid

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

On 2025/1/16 13:48, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>> Sent: Wednesday, January 15, 2025 10:44 PM
>>
>> On Wed, Jan 15, 2025 at 04:43:41AM +0000, Tian, Kevin wrote:
>>>> From: Jason Gunthorpe <jgg@nvidia.com>
>>>> Sent: Tuesday, January 14, 2025 9:46 PM
>>>>
>>>> On Tue, Jan 14, 2025 at 08:10:41AM +0000, Tian, Kevin wrote:
>>>>
>>>>>>> +	ret = __iommu_set_group_pasid(domain, group, pasid, curr-
>>>>>>> domain);
>>>>>>> +	if (ret)
>>>>>>> +		WARN_ON(handle != xa_store(&group->pasid_array,
>> pasid,
>>>>>>> +					   curr, GFP_KERNEL));
>>>>>>
>>>>>> I wonder about the ordering here, is it OK to have PRIs being
>>>>>> delivered to a domain that failed to attach? What cleans up that race
>>>>>> condition with domain free?
>>>>>>
>>>>>> Should we replace the domain then set the xarray? (and same
>> ordering
>>>>>> question for normal attach)
>>>>>
>>>>> That makes sense to me.
>>>>>
>>>>> But I don't think there is a problem with attach. xa_insert() will
>>>>> return error if an entry already exists. So there won't be any
>>>>> PRI being delivered at __iommu_set_group_pasid(), no matter
>>>>> it succeeds or not.
>>>>
>>>> It has the same issue:
>>>>
>>>> 	ret = xa_insert(&group->pasid_array, pasid, handle, GFP_KERNEL);
>>>> 	if (ret)
>>>> 		goto out_unlock;
>>>>
>>>> 	ret = __iommu_set_group_pasid(domain, group, pasid);
>>>>          .. Concurrently a PRI event is pushed to the domain ..
>>>> 	if (ret)
>>>> 		xa_erase(&group->pasid_array, pasid);
>>>>
>>>>          .. Now what? Who fences the PRI event thread before the caller
>>>> 	   frees the domain ..?
>>>>
>>>> We arranged things so that detatch would fence the PRI, if detach is
>>>> not called then there is no fence..
>>>>
>>>
>>> Though I'm fine to change the order in attach too as it looks more
>>> reasonable logically, I'm trying to understand the actual impact of
>>> the original order (e.g. is the change worth of a Fix tag?)
>>>
>>> If there is no detach happened before then it's the 1st attach to
>>> a faultable domain and PRI will be enabled right before this function
>>> hence no fence required.
>>>
>>> Would a sane device trigger PRI in this window?
>>
>> I would say this is not a "sane" scenario, this is a theoretical race
>> triggerable by the device. Perhaps a VFIO user can force the device to
>> trigger this race and exploit the kernel.
>>
>>> If it's a detach-then-attach flow, detach will do the fence anyway
>>> before the attach.
>>
>> The issue is the error, once we do the xa_insert() then any faults will
>> get routed to our domain and the fault path threads will hold pointers
>> to the domain. Once the xa_insert() is done we must flush the fault
>> path threads before allowing the domain to be freed.
>>
>> If __iommu_set_group_pasid() fails then we do an xa_erase() but
>> nothing will flush the fault threads.
>>
> 
> If __iommu_set_group_pasid() fails iommufd will attempt to disable
> PRI which reaches iopf_queue_remove_device(). The latter auto
> responds to the list of pending faults.
> 
> But this is not a reliable assumption e.g. when multiple functions
> (VFs and PF) share a single PRI entity.

I think iommufd fault only supports PF so far. VF is not supported. :) But
it's better not make decisions based on this limitation when considering
the PRI flushing issue.

> So I agree it's conceptually clearer to swap the order of updating
> xarray and doing attach.
> 
> btw in reality this won't trigger any issue on VT-d. The spec says
> that PRI upon a non-present PASID entry (the state before attach
> succeeds) is auto-responded by HW as 'Invalid Request'. So the
> entire software faulting path won't triggered at all but this might
> be a vendor specific behavior...

I'll swap the order between the group->pasid_array and
__iommu_set_group_pasid() in both the PASID attach and replace path.

I think the RID attach/replace path have the same problem since the
PRI forwarding path also retrieves the iommu_attach_handle from the
group->pasid_array. Even worse, I didn't see the RID path set the
handle to group->pasid_array. @Baolu, perhaps you can help to fix
the RID path? :)

-- 
Regards,
Yi Liu