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: <aHUzCAM8NKuFYbj3@yilunxu-OptiPlex-7050>
Date: Tue, 15 Jul 2025 00:40:40 +0800
From: Xu Yilun <yilun.xu@...ux.intel.com>
To: Nicolin Chen <nicolinc@...dia.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 Sun, Jul 13, 2025 at 01:47:45AM +0800, Xu Yilun wrote:
> > [..]
> > 
> > >  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);
> 
> I think this makes things more clear, we have 3 types of refcounts:
> 
> 1. shortterm_users + 1, users + 1, temporary refcount, can't be
>    revoked by referenced object.
> 2. users + 1, long term refcount, can't be revoked either.
> 3. shortterm_users + 1, can be revoked by referenced object.
> 
> > 
> > Then, keep the exit patch unchanged.
> > out_put_idev:
> > 	iommufd_put_object(ucmd->ictx, &idev->obj);
> 
> Yeah, this is the part I like the most.

Sorry, I feel the actual coding is not as good as I image, the

  refcount_dec(&idev->obj.shortterm_users)

can't be called in iommufd_vdevice_abort() cause it may lead to idev
free but we will then do

  mutex_unlock(idev->igroup->lock)

I think the following change makes things more complex...


diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
index 702ae248df17..bdd5a5227cbf 100644
--- a/drivers/iommu/iommufd/viommu.c
+++ b/drivers/iommu/iommufd/viommu.c
@@ -128,7 +128,8 @@ void iommufd_vdevice_destroy(struct iommufd_object *obj)
        mutex_lock(&idev->igroup->lock);
        iommufd_vdevice_abort(obj);
        mutex_unlock(&idev->igroup->lock);
-       iommufd_put_object(idev->ictx, &idev->obj);
+       refcount_dec(&idev->obj.shortterm_users);
+       wake_up_interruptible_all(&vdev->viommu->ictx->destroy_wait);
 }

 int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
@@ -185,6 +186,7 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
         * idev real destruction.
         */
        vdev->idev = idev;
+       refcount_inc(&idev->obj.shortterm_users);

        /*
         * iommufd_device_destroy() delays until idev->vdev is NULL before
@@ -207,12 +209,12 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
        goto out_unlock_igroup;

 out_abort:
+       refcount_dec(&idev->obj.shortterm_users);
        iommufd_object_abort_and_destroy(ucmd->ictx, &vdev->obj);
 out_unlock_igroup:
        mutex_unlock(&idev->igroup->lock);
 out_put_idev:
-       if (rc)
-               iommufd_put_object(ucmd->ictx, &idev->obj);
+       iommufd_put_object(ucmd->ictx, &idev->obj);
 out_put_viommu:
        iommufd_put_object(ucmd->ictx, &viommu->obj);
        return rc;


Thanks,
Yilun