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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <cfc8cc32-8c62-440d-946f-69cd855fedaf@linux.intel.com>
Date:   Wed, 13 Dec 2023 10:19:32 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     baolu.lu@...ux.intel.com, Joerg Roedel <joro@...tes.org>,
        Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Kevin Tian <kevin.tian@...el.com>,
        Jean-Philippe Brucker <jean-philippe@...aro.org>,
        Nicolin Chen <nicolinc@...dia.com>,
        Yi Liu <yi.l.liu@...el.com>,
        Jacob Pan <jacob.jun.pan@...ux.intel.com>,
        Longfang Liu <liulongfang@...wei.com>,
        Yan Zhao <yan.y.zhao@...el.com>, iommu@...ts.linux.dev,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 12/12] iommu: Use refcount for fault data access

On 12/12/23 11:18 PM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 01:07:17PM +0800, Baolu Lu wrote:
> 
>> 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;
> 
> We should never get here if this is the case, prevent the device from
> being added in the first place

Yeah, could move it to iopf_queue_add_device(). WARN and return failure
there if the driver is not ready for page request handling.

> 
>> 	/* 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);
> 
> I would have expected the group to free'd here? But regardless this
> looks like a good direction

Both work for me. We can decide it according to the needs of code later.

> 
> Jason

Best regards,
baolu