linux-kernel - Re: [PATCH v4 4/7] iommufd: Destroy vdevice on idevice destroy

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: <aHHG/H6IT9lvYy8x@Asurada-Nvidia>
Date: Fri, 11 Jul 2025 19:22:52 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Xu Yilun <yilun.xu@...ux.intel.com>
CC: <jgg@...dia.com>, <jgg@...pe.ca>, <kevin.tian@...el.com>,
	<will@...nel.org>, <aneesh.kumar@...nel.org>, <iommu@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, <joro@...tes.org>, <robin.murphy@....com>,
	<shuah@...nel.org>, <aik@....com>, <dan.j.williams@...el.com>,
	<baolu.lu@...ux.intel.com>, <yilun.xu@...el.com>
Subject: Re: [PATCH v4 4/7] iommufd: Destroy vdevice on idevice destroy

On Wed, Jul 09, 2025 at 12:02:31PM +0800, Xu Yilun wrote:
> Destroy iommufd_vdevice (vdev) on iommufd_idevice (idev) destruction so
> that vdev can't outlive idev.
> 
> idev represents the physical device bound to iommufd, while the vdev
> represents the virtual instance of the physical device in the VM. The
> lifecycle of the vdev should not be longer than idev. This doesn't
> cause real problem on existing use cases cause vdev doesn't impact the
> physical device, only provides virtualization information. But to
> extend vdev for Confidential Computing (CC), there are needs to do
> secure configuration for the vdev, e.g. TSM Bind/Unbind. These
> configurations should be rolled back on idev destroy, or the external
> driver (VFIO) functionality may be impact.
> 
> The idev is created by external driver so its destruction can't fail.
> The idev implements pre_destroy() op to actively remove its associated
> vdev before destroying itself. There are 3 cases on idev pre_destroy():
> 
>   1. vdev is already destroyed by userspace. No extra handling needed.
>   2. vdev is still alive. Use iommufd_object_tombstone_user() to
>      destroy vdev and tombstone the vdev ID.
>   3. vdev is being destroyed by userspace. The vdev ID is already
>      freed, but vdev destroy handler is not completed. This requires
>      multi-threads syncing - vdev holds idev's short term users
>      reference until vdev destruction completes, idev leverages
>      existing wait_shortterm mechanism for syncing.
> 
> Originally-by: Nicolin Chen <nicolinc@...dia.com>
> Suggested-by: Jason Gunthorpe <jgg@...dia.com>
> Reviewed-by: Lu Baolu <baolu.lu@...ux.intel.com>
> Co-developed-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@...nel.org>
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@...nel.org>
> Signed-off-by: Xu Yilun <yilun.xu@...ux.intel.com>

Reviewed-by: Nicolin Chen <nicolinc@...dia.com>

With a nit:

> @@ -155,6 +182,19 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
>  	get_device(idev->dev);
>  	vdev->viommu = viommu;
>  	refcount_inc(&viommu->obj.users);
> +	/*
> +	 * A short term users reference is held on the idev so long as we have
> +	 * the pointer. iommufd_device_pre_destroy() will revoke it before the
> +	 * idev real destruction.
> +	 */
> +	vdev->idev = idev;

[..]

>  out_put_idev:
> -	iommufd_put_object(ucmd->ictx, &idev->obj);
> +	if (rc)
> +		iommufd_put_object(ucmd->ictx, &idev->obj);

So, this actually holds both the short term user and (covertly)
the regular user too.

Though it doesn't hurt to do that, holding the regular one seems
to be useless, because its refcount check is way behind this new
pre_destroy op:

183         if (flags & REMOVE_WAIT_SHORTTERM) {                                                                                                                                                                                                                                                                                                             
184                 ret = iommufd_object_dec_wait_shortterm(ictx, to_destroy);
==>			/* pre_destroy op */
...
214         if (!refcount_dec_if_one(&obj->users)) {
215                 ret = -EBUSY;
216                 goto err_xa;
217         }

So, I think we could just do, exactly reflecting the comments:
+	vdev->idev = idev;
+	refcount_inc(&idev->obj.shortterm_users);

Then, keep the exit patch unchanged.
out_put_idev:
	iommufd_put_object(ucmd->ictx, &idev->obj);

Thanks
Nicolin