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

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

On Mon, 16 Jun 2025 12:34:55 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Mon, Jun 16, 2025 at 08:47:08AM -0600, Alex Williamson wrote:
> > 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.  
> 
> Broadly what I was going for was to just remove the iommufd stuff
> entirely from the DF layer rather than to half pretend there is an
> iommufd layer below it. This should ideally go as far as not having an
> iommufd_ctx at all. So things start to look really weird calling
> iommufd functions without an iommufd ctx.
> 
> > > 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,  
> 
> The container still exists in noiommu mode and internally does things,
> eg it has a container->noiommu indicationm and the vfio-noiommu ops to
> manage this.
> 
> The iommufd should not exist and should never be used. They are
> different cases.
> 
> If Jacob eventually does what I suggested in another email then we
> would have a noiommu special mode inside iommufd and it would look
> more like the container.

A concise fix would be nice for stable backports though, so even if we
want to move to testing noiommu in the device file layer or create a
special mode in iommufd, the smallest, most consistent initial fix
would be to continue the _group_open:

--- 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;
+		}
 	}


And add a noiommu exit branch to _iommufd_bind, symmetric to unbind.
Right?  Thanks,

Alex