[PATCH v1 0/2] iommufd: Fix a small bug in fault.c

[PATCH v1 0/2] iommufd: Fix a small bug in fault.c

[Nicolin Chen]

There are a few patches in vIRQ series that rework the fault.c file. So,
we should fix this before that bigger series touches the same code.

And add missing coverage for IOMMU_FAULT_QUEUE_ALLOC in iommufd_fail_nth.

Thanks!
Nicolin

Nicolin Chen (2):
  iommufd/fault: Fix out_fput in iommufd_fault_alloc()
  iommufd/selftest: Cover IOMMU_FAULT_QUEUE_ALLOC in iommufd_fail_nth

 drivers/iommu/iommufd/fault.c                    |  2 --
 tools/testing/selftests/iommu/iommufd_fail_nth.c | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v1 1/2] iommufd/fault: Fix out_fput in iommufd_fault_alloc()

[Nicolin Chen]

As fput() calls the file->f_op->release op, where fault obj and ictx are
getting released, there is no need to release these two after fput() one
more time, which would result in imbalanced refcounts:
  refcount_t: decrement hit 0; leaking memory.
  WARNING: CPU: 48 PID: 2369 at lib/refcount.c:31 refcount_warn_saturate+0x60/0x230
  Call trace:
   refcount_warn_saturate+0x60/0x230 (P)
   refcount_warn_saturate+0x60/0x230 (L)
   iommufd_fault_fops_release+0x9c/0xe0 [iommufd]
  ...
  VFS: Close: file count is 0 (f_op=iommufd_fops [iommufd])
  WARNING: CPU: 48 PID: 2369 at fs/open.c:1507 filp_flush+0x3c/0xf0
  Call trace:
   filp_flush+0x3c/0xf0 (P)
   filp_flush+0x3c/0xf0 (L)
   __arm64_sys_close+0x34/0x98
  ...
  imbalanced put on file reference count
  WARNING: CPU: 48 PID: 2369 at fs/file.c:74 __file_ref_put+0x100/0x138
  Call trace:
   __file_ref_put+0x100/0x138 (P)
   __file_ref_put+0x100/0x138 (L)
   __fput_sync+0x4c/0xd0

Drop those two lines to fix the warnings above.

Fixes: 07838f7fd529 ("iommufd: Add iommufd fault object")
Cc: stable@vger.kernel.org
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/fault.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index 053b0e30f55a..1fe804e28a86 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -420,8 +420,6 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
 	put_unused_fd(fdno);
 out_fput:
 	fput(filep);
-	refcount_dec(&fault->obj.users);
-	iommufd_ctx_put(fault->ictx);
 out_abort:
 	iommufd_object_abort_and_destroy(ucmd->ictx, &fault->obj);
 
-- 
2.43.0

[PATCH v1 2/2] iommufd/selftest: Cover IOMMU_FAULT_QUEUE_ALLOC in iommufd_fail_nth

[Nicolin Chen]

This was missing in the series introducing the fault object. Thus, add it.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 tools/testing/selftests/iommu/iommufd_fail_nth.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/testing/selftests/iommu/iommufd_fail_nth.c b/tools/testing/selftests/iommu/iommufd_fail_nth.c
index 22f6fd5f0f74..64b1f8e1b0cf 100644
--- a/tools/testing/selftests/iommu/iommufd_fail_nth.c
+++ b/tools/testing/selftests/iommu/iommufd_fail_nth.c
@@ -615,7 +615,12 @@ TEST_FAIL_NTH(basic_fail_nth, access_pin_domain)
 /* device.c */
 TEST_FAIL_NTH(basic_fail_nth, device)
 {
+	struct iommu_hwpt_selftest data = {
+		.iotlb = IOMMU_TEST_IOTLB_DEFAULT,
+	};
 	struct iommu_test_hw_info info;
+	uint32_t fault_id, fault_fd;
+	uint32_t fault_hwpt_id;
 	uint32_t ioas_id;
 	uint32_t ioas_id2;
 	uint32_t stdev_id;
@@ -678,6 +683,15 @@ TEST_FAIL_NTH(basic_fail_nth, device)
 	if (_test_cmd_vdevice_alloc(self->fd, viommu_id, idev_id, 0, &vdev_id))
 		return -1;
 
+	if (_test_ioctl_fault_alloc(self->fd, &fault_id, &fault_fd))
+		return -1;
+	close(fault_fd);
+
+	if (_test_cmd_hwpt_alloc(self->fd, idev_id, hwpt_id, fault_id,
+				 IOMMU_HWPT_FAULT_ID_VALID, &fault_hwpt_id,
+				 IOMMU_HWPT_DATA_SELFTEST, &data, sizeof(data)))
+		return -1;
+
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v1 1/2] iommufd/fault: Fix out_fput in iommufd_fault_alloc()

[Yi Liu]

On 2024/12/3 16:02, Nicolin Chen wrote:
> As fput() calls the file->f_op->release op, where fault obj and ictx are
> getting released, there is no need to release these two after fput() one
> more time, which would result in imbalanced refcounts:
>    refcount_t: decrement hit 0; leaking memory.
>    WARNING: CPU: 48 PID: 2369 at lib/refcount.c:31 refcount_warn_saturate+0x60/0x230
>    Call trace:
>     refcount_warn_saturate+0x60/0x230 (P)
>     refcount_warn_saturate+0x60/0x230 (L)
>     iommufd_fault_fops_release+0x9c/0xe0 [iommufd]
>    ...
>    VFS: Close: file count is 0 (f_op=iommufd_fops [iommufd])
>    WARNING: CPU: 48 PID: 2369 at fs/open.c:1507 filp_flush+0x3c/0xf0
>    Call trace:
>     filp_flush+0x3c/0xf0 (P)
>     filp_flush+0x3c/0xf0 (L)
>     __arm64_sys_close+0x34/0x98
>    ...
>    imbalanced put on file reference count
>    WARNING: CPU: 48 PID: 2369 at fs/file.c:74 __file_ref_put+0x100/0x138
>    Call trace:
>     __file_ref_put+0x100/0x138 (P)
>     __file_ref_put+0x100/0x138 (L)
>     __fput_sync+0x4c/0xd0
> 
> Drop those two lines to fix the warnings above.
> 
> Fixes: 07838f7fd529 ("iommufd: Add iommufd fault object")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>   drivers/iommu/iommufd/fault.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
> index 053b0e30f55a..1fe804e28a86 100644
> --- a/drivers/iommu/iommufd/fault.c
> +++ b/drivers/iommu/iommufd/fault.c
> @@ -420,8 +420,6 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
>   	put_unused_fd(fdno);
>   out_fput:
>   	fput(filep);
> -	refcount_dec(&fault->obj.users);
> -	iommufd_ctx_put(fault->ictx);
>   out_abort:
>   	iommufd_object_abort_and_destroy(ucmd->ictx, &fault->obj);
>   

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

-- 
Regards,
Yi Liu

Re: [PATCH v1 0/2] iommufd: Fix a small bug in fault.c

[Jason Gunthorpe]

On Tue, Dec 03, 2024 at 12:02:53AM -0800, Nicolin Chen wrote:
> There are a few patches in vIRQ series that rework the fault.c file. So,
> we should fix this before that bigger series touches the same code.
> 
> And add missing coverage for IOMMU_FAULT_QUEUE_ALLOC in iommufd_fail_nth.
> 
> Thanks!
> Nicolin
> 
> Nicolin Chen (2):
>   iommufd/fault: Fix out_fput in iommufd_fault_alloc()
>   iommufd/selftest: Cover IOMMU_FAULT_QUEUE_ALLOC in iommufd_fail_nth

Applied to for-rc, thanks

Jason