[PATCH] iommufd: Test attach before detaching pasid

[PATCH] iommufd: Test attach before detaching pasid

[Yi Liu]

Check if the pasid has been attached before going further in the detach
path. This makes this path more robust. Add a selftest as well.

Reported-by: Lai Yi <yi1.lai@linux.intel.com>
Closes: https://lore.kernel.org/linux-iommu/Z+X0tzxhiaupJT7b@ly-workstation/#t
Fixes: c0e301b2978d ("iommufd/device: Add pasid_attach array to track per-PASID attach")
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/device.c          | 7 +++++++
 tools/testing/selftests/iommu/iommufd.c | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 2307daad65c0..375dd262f431 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -644,6 +644,11 @@ iommufd_hw_pagetable_detach(struct iommufd_device *idev, ioasid_t pasid)
 
 	mutex_lock(&igroup->lock);
 	attach = xa_load(&igroup->pasid_attach, pasid);
+	if (WARN_ON(!attach)) {
+		mutex_unlock(&igroup->lock);
+		return NULL;
+	}
+
 	hwpt = attach->hwpt;
 	hwpt_paging = find_hwpt_paging(hwpt);
 
@@ -1001,6 +1006,8 @@ void iommufd_device_detach(struct iommufd_device *idev, ioasid_t pasid)
 	struct iommufd_hw_pagetable *hwpt;
 
 	hwpt = iommufd_hw_pagetable_detach(idev, pasid);
+	if (!hwpt)
+		return;
 	iommufd_hw_pagetable_put(idev->ictx, hwpt);
 	refcount_dec(&idev->obj.users);
 }
diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c
index 7eb7ee149f2b..1a8e85afe9aa 100644
--- a/tools/testing/selftests/iommu/iommufd.c
+++ b/tools/testing/selftests/iommu/iommufd.c
@@ -3074,6 +3074,12 @@ TEST_F(iommufd_device_pasid, pasid_attach)
 	uint32_t pasid = 100;
 	uint32_t viommu_id;
 
+	/*
+	 * Negative, detach pasid without attaching, this is not expected.
+	 * But it should not result in failure anyway.
+	 */
+	test_cmd_pasid_detach(pasid);
+
 	/* Allocate two nested hwpts sharing one common parent hwpt */
 	test_cmd_hwpt_alloc(self->device_id, self->ioas_id,
 			    IOMMU_HWPT_ALLOC_NEST_PARENT,
-- 
2.34.1

Re: [PATCH] iommufd: Test attach before detaching pasid

[Jason Gunthorpe]

On Fri, Mar 28, 2025 at 06:34:48AM -0700, Yi Liu wrote:
> Check if the pasid has been attached before going further in the detach
> path. This makes this path more robust. Add a selftest as well.
> 
> Reported-by: Lai Yi <yi1.lai@linux.intel.com>
> Closes: https://lore.kernel.org/linux-iommu/Z+X0tzxhiaupJT7b@ly-workstation/#t
> Fixes: c0e301b2978d ("iommufd/device: Add pasid_attach array to track per-PASID attach")
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommufd/device.c          | 7 +++++++
>  tools/testing/selftests/iommu/iommufd.c | 6 ++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 2307daad65c0..375dd262f431 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -644,6 +644,11 @@ iommufd_hw_pagetable_detach(struct iommufd_device *idev, ioasid_t pasid)
>  
>  	mutex_lock(&igroup->lock);
>  	attach = xa_load(&igroup->pasid_attach, pasid);
> +	if (WARN_ON(!attach)) {
> +		mutex_unlock(&igroup->lock);
> +		return NULL;
> +	}

We can't trigger a WARN_ON during the test suite!

#  RUN           iommufd_device_pasid.no_pasid.pasid_attach ...
[   23.723948] iommufd_mock iommufd_mock0: Adding to iommu group 0
[   23.724532] iommufd_mock iommufd_mock1: Adding to iommu group 1
[   23.724722] ------------[ cut here ]------------
[   23.725194] WARNING: CPU: 5 PID: 1169 at drivers/iommu/iommufd/device.c:647 iommufd_hw_pagetable_detach+0x1e7/0x230 [iommufd]
[   23.725350] Modules linked in: iommufd
[   23.725406] CPU: 5 UID: 0 PID: 1169 Comm: iommufd Not tainted 6.14.0-rc2+ #1

Just drop the WARN_ON check right?

I took it like that, please let me know

Jason

Re: [PATCH] iommufd: Test attach before detaching pasid

[Yi Liu]

On 2025/3/28 22:40, Jason Gunthorpe wrote:
> On Fri, Mar 28, 2025 at 06:34:48AM -0700, Yi Liu wrote:
>> Check if the pasid has been attached before going further in the detach
>> path. This makes this path more robust. Add a selftest as well.
>>
>> Reported-by: Lai Yi <yi1.lai@linux.intel.com>
>> Closes: https://lore.kernel.org/linux-iommu/Z+X0tzxhiaupJT7b@ly-workstation/#t
>> Fixes: c0e301b2978d ("iommufd/device: Add pasid_attach array to track per-PASID attach")
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/iommufd/device.c          | 7 +++++++
>>   tools/testing/selftests/iommu/iommufd.c | 6 ++++++
>>   2 files changed, 13 insertions(+)
>>
>> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
>> index 2307daad65c0..375dd262f431 100644
>> --- a/drivers/iommu/iommufd/device.c
>> +++ b/drivers/iommu/iommufd/device.c
>> @@ -644,6 +644,11 @@ iommufd_hw_pagetable_detach(struct iommufd_device *idev, ioasid_t pasid)
>>   
>>   	mutex_lock(&igroup->lock);
>>   	attach = xa_load(&igroup->pasid_attach, pasid);
>> +	if (WARN_ON(!attach)) {
>> +		mutex_unlock(&igroup->lock);
>> +		return NULL;
>> +	}
> 
> We can't trigger a WARN_ON during the test suite!
> 
> #  RUN           iommufd_device_pasid.no_pasid.pasid_attach ...
> [   23.723948] iommufd_mock iommufd_mock0: Adding to iommu group 0
> [   23.724532] iommufd_mock iommufd_mock1: Adding to iommu group 1
> [   23.724722] ------------[ cut here ]------------
> [   23.725194] WARNING: CPU: 5 PID: 1169 at drivers/iommu/iommufd/device.c:647 iommufd_hw_pagetable_detach+0x1e7/0x230 [iommufd]
> [   23.725350] Modules linked in: iommufd
> [   23.725406] CPU: 5 UID: 0 PID: 1169 Comm: iommufd Not tainted 6.14.0-rc2+ #1
> 
> Just drop the WARN_ON check right?
> 
> I took it like that, please let me know

looks good to me. thanks.

-- 
Regards,
Yi Liu