linux-kernel - Re: [PATCH v5 09/29] iommufd: Do not unmap an owned iopt

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250528170845.GX61950@nvidia.com>
Date: Wed, 28 May 2025 14:08:45 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: kevin.tian@...el.com, corbet@....net, will@...nel.org,
	bagasdotme@...il.com, robin.murphy@....com, joro@...tes.org,
	thierry.reding@...il.com, vdumpa@...dia.com, jonathanh@...dia.com,
	shuah@...nel.org, jsnitsel@...hat.com, nathan@...nel.org,
	peterz@...radead.org, yi.l.liu@...el.com, mshavit@...gle.com,
	praan@...gle.com, zhangzekun11@...wei.com, iommu@...ts.linux.dev,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-tegra@...r.kernel.org,
	linux-kselftest@...r.kernel.org, patches@...ts.linux.dev,
	mochs@...dia.com, alok.a.tiwari@...cle.com, vasant.hegde@....com,
	dwmw2@...radead.org, baolu.lu@...ux.intel.com
Subject: Re: [PATCH v5 09/29] iommufd: Do not unmap an owned iopt_area

On Sat, May 17, 2025 at 08:21:26PM -0700, Nicolin Chen wrote:
> Do not allow to unmap an iopt_area that is owned by an object, giving this
> owner a choice to lock the mapping in the iopt.
> 
> It will be used by vIOMMU's HW QUEUE structure that must prevent the queue
> memory from being unmapped in the nesting parent IO page table, until user
> space destroys the HW QUEUE object first.
> 
> Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> ---
>  drivers/iommu/iommufd/io_pagetable.h |  7 ++++---
>  drivers/iommu/iommufd/device.c       |  8 +++++---
>  drivers/iommu/iommufd/io_pagetable.c |  6 ++++++
>  drivers/iommu/iommufd/pages.c        | 12 ++++++++++--
>  4 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/io_pagetable.h b/drivers/iommu/iommufd/io_pagetable.h
> index c115a51d9384..2f2a47a2f9ee 100644
> --- a/drivers/iommu/iommufd/io_pagetable.h
> +++ b/drivers/iommu/iommufd/io_pagetable.h
> @@ -48,6 +48,7 @@ struct iopt_area {
>  	int iommu_prot;
>  	bool prevent_access : 1;
>  	unsigned int num_accesses;
> +	unsigned int num_owners;
>  };

Owner seems like the wrong word here..

Fundamentally this is supporting accesses that don't have an unmap op
callback.

It could be done differently, with the below just create your access
with a null ops. Prevent mdev drivers from doing this when they create
their accesses. We don't need to optimize this failure case so it is
fine to scan the xarray. All we really need to do here is eliminate
the WARN_ON.

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 2111bad72c720f..b88354de299449 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -1206,13 +1206,14 @@ EXPORT_SYMBOL_NS_GPL(iommufd_access_replace, "IOMMUFD");
  * run in the future. Due to this a driver must not create locking that prevents
  * unmap to complete while iommufd_access_destroy() is running.
  */
-void iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
+int iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
 				 unsigned long length)
 {
 	struct iommufd_ioas *ioas =
 		container_of(iopt, struct iommufd_ioas, iopt);
 	struct iommufd_access *access;
 	unsigned long index;
+	int ret = 0;
 
 	xa_lock(&ioas->iopt.access_list);
 	xa_for_each(&ioas->iopt.access_list, index, access) {
@@ -1220,12 +1221,17 @@ void iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
 			continue;
 		xa_unlock(&ioas->iopt.access_list);
 
-		access->ops->unmap(access->data, iova, length);
+		if (access->ops && access->ops->unmap)
+			access->ops->unmap(access->data, iova, length);
+		else
+			ret = -EBUSY;
 
 		iommufd_put_object(access->ictx, &access->obj);
 		xa_lock(&ioas->iopt.access_list);
 	}
 	xa_unlock(&ioas->iopt.access_list);
+
+	return ret;
 }
 
 /**
diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
index 8a790e597e1253..f1cb809500fa19 100644
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -740,11 +740,16 @@ static int iopt_unmap_iova_range(struct io_pagetable *iopt, unsigned long start,
 			up_write(&iopt->iova_rwsem);
 			up_read(&iopt->domains_rwsem);
 
-			iommufd_access_notify_unmap(iopt, area_first, length);
+			rc = iommufd_access_notify_unmap(iopt, area_first, length);
+			if (rc)
+				goto out_unmapped;
+
 			/* Something is not responding to unmap requests. */
 			tries++;
-			if (WARN_ON(tries > 100))
-				return -EDEADLOCK;
+			if (WARN_ON(tries > 100)) {
+				rc = -EDEADLOCK;
+				goto out_unmapped;
+			}
 			goto again;
 		}
 
@@ -766,6 +771,7 @@ static int iopt_unmap_iova_range(struct io_pagetable *iopt, unsigned long start,
 out_unlock_iova:
 	up_write(&iopt->iova_rwsem);
 	up_read(&iopt->domains_rwsem);
+out_unmapped:
 	if (unmapped)
 		*unmapped = unmapped_bytes;
 	return rc;