| 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
| ||
|
Message-ID: <aFPbo6RI-D4T7nXE@google.com>
Date: Thu, 19 Jun 2025 09:42:59 +0000
From: Pranjal Shrivastava <praan@...gle.com>
To: Nicolin Chen <nicolinc@...dia.com>
Cc: jgg@...dia.com, 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,
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 v6 07/25] iommufd/access: Add internal APIs for HW queue
to use
On Sat, Jun 14, 2025 at 12:14:32AM -0700, Nicolin Chen wrote:
> Now, access->ops can be NULL, to support an internal use case for the new
> HW queue object. Since an access object in this case will be allocated by
> an inernal iommufd object, the refcount on the ictx should be skipped, so
> as not to deadlock the release of the ictx as it would otherwise wait for
> the release of the access first during the release of the internal object
> that could wait for the release of ictx:
> ictx --releases--> hw_queue --releases--> access
> ^ |
> |_________________releases________________v
>
> Add a set of lightweight internal APIs to unlink access and ictx:
> ictx --releases--> hw_queue --releases--> access
>
> Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> ---
> drivers/iommu/iommufd/iommufd_private.h | 8 ++++
> drivers/iommu/iommufd/device.c | 59 +++++++++++++++++++++----
> 2 files changed, 58 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index 4a375a8c9216..468717d5e5bc 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -484,6 +484,14 @@ void iopt_remove_access(struct io_pagetable *iopt,
> struct iommufd_access *access, u32 iopt_access_list_id);
> void iommufd_access_destroy_object(struct iommufd_object *obj);
>
> +/* iommufd_access for internal use */
> +struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx);
> +#define iommufd_access_destroy_internal(ictx, access) \
> + iommufd_object_destroy_user(ictx, &(access)->obj)
> +int iommufd_access_attach_internal(struct iommufd_access *access,
> + struct iommufd_ioas *ioas);
> +#define iommufd_access_detach_internal(access) iommufd_access_detach(access)
> +
> struct iommufd_eventq {
> struct iommufd_object obj;
> struct iommufd_ctx *ictx;
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 9293722b9cff..ad33f1e41a24 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
> if (access->ioas)
> WARN_ON(iommufd_access_change_ioas(access, NULL));
> mutex_unlock(&access->ioas_lock);
> - iommufd_ctx_put(access->ictx);
> + if (access->ops)
> + iommufd_ctx_put(access->ictx);
Purely relying on access->ops being NULL feels a bit hacky to me..
> +}
> +
> +static struct iommufd_access *__iommufd_access_create(struct iommufd_ctx *ictx)
> +{
> + struct iommufd_access *access;
> +
> + /*
> + * There is no uAPI for the access object, but to keep things symmetric
> + * use the object infrastructure anyhow.
> + */
> + access = iommufd_object_alloc(ictx, access, IOMMUFD_OBJ_ACCESS);
> + if (IS_ERR(access))
> + return access;
> +
> + /* The calling driver is a user until iommufd_access_destroy() */
> + refcount_inc(&access->obj.users);
> + mutex_init(&access->ioas_lock);
> + return access;
> +}
> +
> +struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx)
> +{
> + struct iommufd_access *access;
> +
> + access = __iommufd_access_create(ictx);
> + if (IS_ERR(access))
> + return access;
> + access->iova_alignment = PAGE_SIZE;
Maybe setting acces->ictx = NULL; explicitly here would be a clear
demarcation between the new API for "internal" v/s the original one.
Else, I definitely believe we should have a comment mentioning that
access->ictx is NULL for internal.
> +
> + iommufd_object_finalize(ictx, &access->obj);
> + return access;
> }
>
> /**
> @@ -1106,11 +1138,7 @@ iommufd_access_create(struct iommufd_ctx *ictx,
> {
> struct iommufd_access *access;
>
> - /*
> - * There is no uAPI for the access object, but to keep things symmetric
> - * use the object infrastructure anyhow.
> - */
> - access = iommufd_object_alloc(ictx, access, IOMMUFD_OBJ_ACCESS);
> + access = __iommufd_access_create(ictx);
> if (IS_ERR(access))
> return access;
>
> @@ -1122,13 +1150,10 @@ iommufd_access_create(struct iommufd_ctx *ictx,
> else
> access->iova_alignment = 1;
>
> - /* The calling driver is a user until iommufd_access_destroy() */
> - refcount_inc(&access->obj.users);
> access->ictx = ictx;
> iommufd_ctx_get(ictx);
> iommufd_object_finalize(ictx, &access->obj);
> *id = access->obj.id;
> - mutex_init(&access->ioas_lock);
> return access;
> }
> EXPORT_SYMBOL_NS_GPL(iommufd_access_create, "IOMMUFD");
> @@ -1173,6 +1198,22 @@ int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
> }
> EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, "IOMMUFD");
>
> +int iommufd_access_attach_internal(struct iommufd_access *access,
> + struct iommufd_ioas *ioas)
> +{
> + int rc;
> +
> + mutex_lock(&access->ioas_lock);
> + if (WARN_ON(access->ioas)) {
> + mutex_unlock(&access->ioas_lock);
> + return -EINVAL;
> + }
> +
> + rc = iommufd_access_change_ioas(access, ioas);
> + mutex_unlock(&access->ioas_lock);
> + return rc;
> +}
> +
> int iommufd_access_replace(struct iommufd_access *access, u32 ioas_id)
> {
> int rc;
> --
> 2.43.0
>
Powered by blists - more mailing lists