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

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 13 Jun 2025 21:15:55 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> 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.

Alternatively we could be consistent about breaking out of the
vfio/iommufd.c functions that aren't relevant to noiommu.  The
container side handles noiommu internally, why should iommufd push
handling up to the device file layer?  We're really just missing the
bind path.

TBH, it seems like special casing iommufd in the device file layer is
what led to the issue introduced in 6086efe73498.

> > 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.

Or check it in bind since we already check it in unbind.  Either way,
symmetry.
 
> With this patch we move toward the vfio_df..open/close functions being
> symmetrical in their decision making.

But is it?  We special case all the iommufd paths to filter out noiommu
but it's inconsistent with the legacy paths.  Thanks,

Alex