Re: [PATCH v8 12/12] iommu: Use refcount for fault data access

Baolu Lu
2023-12-12T05:12:01Z

On 12/11/23 11:24 PM, Jason Gunthorpe wrote:
> On Thu, Dec 07, 2023 at 02:43:08PM +0800, Lu Baolu wrote:
>> @@ -217,12 +250,9 @@ int iommu_page_response(struct device *dev,
>> if (!ops->page_response)
>> return -ENODEV;
>>
>> - mutex_lock(&param->lock);
>> - fault_param = param->fault_param;
>> - if (!fault_param) {
>> - mutex_unlock(&param->lock);
>> + fault_param = iopf_get_dev_fault_param(dev);
>> + if (!fault_param)
>> return -EINVAL;
>> - }
> The refcounting should work by passing around the fault_param object,
> not re-obtaining it from the dev from a work.
>
> The work should be locked to the iommu_fault_param that was active
> when the work was launched.
>
> When we get to iommu_page_response it does this:
>
> /* Only send response if there is a fault report pending */
> mutex_lock(&fault_param->lock);
> if (list_empty(&fault_param->faults)) {
> dev_warn_ratelimited(dev, "no pending PRQ, drop response\n");
> goto done_unlock;
> }
>
> Which determines that the iommu_fault_param is stale and pending
> free..

Yes, agreed. The iopf_fault_param should be passed in together with the
iopf_group. The reference count should be released in the
iopf_free_group(). These two helps could look like below:

int iommu_page_response(struct iopf_group *group,
struct iommu_page_response *msg)
{
bool needs_pasid;
int ret = -EINVAL;
struct iopf_fault *evt;
struct iommu_fault_page_request *prm;
struct device *dev = group->fault_param->dev;
const struct iommu_ops *ops = dev_iommu_ops(dev);
bool has_pasid = msg->flags & IOMMU_PAGE_RESP_PASID_VALID;
struct iommu_fault_param *fault_param = group->fault_param;

if (!ops->page_response)
return -ENODEV;

/* Only send response if there is a fault report pending */
mutex_lock(&fault_param->lock);
if (list_empty(&fault_param->faults)) {
dev_warn_ratelimited(dev, "no pending PRQ, drop response\n");
goto done_unlock;
}
/*
* Check if we have a matching page request pending to respond,
* otherwise return -EINVAL
*/
list_for_each_entry(evt, &fault_param->faults, list) {
prm = &evt->fault.prm;
if (prm->grpid != msg->grpid)
continue;

/*
* If the PASID is required, the corresponding request is
* matched using the group ID, the PASID valid bit and the PASID
* value. Otherwise only the group ID matches request and
* response.
*/
needs_pasid = prm->flags & IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID;
if (needs_pasid && (!has_pasid || msg->pasid != prm->pasid))
continue;

if (!needs_pasid && has_pasid) {
/* No big deal, just clear it. */
msg->flags &= ~IOMMU_PAGE_RESP_PASID_VALID;
msg->pasid = 0;
}

ret = ops->page_response(dev, evt, msg);
list_del(&evt->list);
kfree(evt);
break;
}

done_unlock:
mutex_unlock(&fault_param->lock);

return ret;
}

...

void iopf_free_group(struct iopf_group *group)
{
struct iopf_fault *iopf, *next;

list_for_each_entry_safe(iopf, next, &group->faults, list) {
if (!(iopf->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
kfree(iopf);
}

/* Pair with iommu_report_device_fault(). */
iopf_put_dev_fault_param(group->fault_param);
kfree(group);
}

Best regards,
baolu