Re: [PATCH v2 2/2] vfio: Fix unbalanced vfio_df_close call in no-iommu mode

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Jun 13, 2025 at 04:31:03PM -0600, Alex Williamson wrote:
> On Tue,  3 Jun 2025 08:23:43 -0700
> Jacob Pan <jacob.pan@linux.microsoft.com> wrote:
> 
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > 
> > For devices with no-iommu enabled in IOMMUFD VFIO compat mode, the group
> > open path skips vfio_df_open(), leaving open_count at 0. This causes a
> > warning in vfio_assert_device_open(device) when vfio_df_close() is called
> > during group close.
> > 
> > The correct behavior is to skip only the IOMMUFD bind in the device open
> > path for no-iommu devices. Commit 6086efe73498 omitted vfio_df_open(),
> > which was too broad. This patch restores the previous behavior, ensuring
> > the vfio_df_open is called in the group open path.
> > 
> > Fixes: 6086efe73498 ("vfio-iommufd: Move noiommu compat validation out of vfio_iommufd_bind()")
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > Tested-by: Jacob Pan <jacob.pan@linux.microsoft.com>
> > Signed-off-by: Jacob Pan <jacob.pan@linux.microsoft.com>
> > ---
> > v2: Use a fix from Jason
> > ---
> >  drivers/vfio/group.c     | 10 +++++-----
> >  drivers/vfio/iommufd.c   |  3 ---
> >  drivers/vfio/vfio_main.c | 26 ++++++++++++++++----------
> >  3 files changed, 21 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> > index c321d442f0da..8f5fe8a392de 100644
> > --- a/drivers/vfio/group.c
> > +++ b/drivers/vfio/group.c
> > @@ -192,18 +192,18 @@ static int vfio_df_group_open(struct vfio_device_file *df)
> >  		 * implies they expected translation to exist
> >  		 */
> >  		if (!capable(CAP_SYS_RAWIO) ||
> > -		    vfio_iommufd_device_has_compat_ioas(device, df->iommufd))
> > +		    vfio_iommufd_device_has_compat_ioas(device, df->iommufd)) {
> >  			ret = -EPERM;
> > -		else
> > -			ret = 0;
> > -		goto out_put_kvm;
> > +			goto out_put_kvm;
> > +		}
> >  	}
> >  
> >  	ret = vfio_df_open(df);
> >  	if (ret)
> >  		goto out_put_kvm;
> >  
> > -	if (df->iommufd && device->open_count == 1) {
> > +	if (df->iommufd && device->open_count == 1 &&
> > +	    !vfio_device_is_noiommu(device)) {
> 
> Why do we need this?

What I was trying to do is put all the logic about noiommu into only
vfio_df..open/close functions instead of sprikling it into a bunch of
other functions. That seemed to be the right point to make this cut.

> int vfio_iommufd_compat_attach_ioas(struct vfio_device *vdev,
>                                     struct iommufd_ctx *ictx)
> {
>         u32 ioas_id;
>         int ret;
> 
>         lockdep_assert_held(&vdev->dev_set->lock);
> 
>         /* compat noiommu does not need to do ioas attach */
>         if (vfio_device_is_noiommu(vdev))
>                 return 0;

So this should be removed, I missed it

> > @@ -54,9 +54,6 @@ void vfio_df_iommufd_unbind(struct vfio_device_file *df)
> >  
> >  	lockdep_assert_held(&vdev->dev_set->lock);
> >  
> > -	if (vfio_device_is_noiommu(vdev))
> > -		return;
> > -
> 
> Why not keep this and add similar to vfio_df_iommufd_bind()?  It seems
> cleaner to me.  Thanks,

Same as above, we don't check for noiommu in bind, so we should not
check it in unbind to have a symetrical API design.

With this patch we move toward the vfio_df..open/close functions being
symmetrical in their decision making.

Jason