linux-kernel - Re: [PATCH v2 2/2] vfio: Fix unbalanced vfio_df

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: <20250614001555.GR1174925@nvidia.com>
Date: Fri, 13 Jun 2025 21:15:55 -0300
From: Jason Gunthorpe <jgg@...dia.com>
To: Alex Williamson <alex.williamson@...hat.com>
Cc: Jacob Pan <jacob.pan@...ux.microsoft.com>, linux-kernel@...r.kernel.org,
	"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"Liu, Yi L" <yi.l.liu@...el.com>, Zhang Yu <zhangyu1@...rosoft.com>,
	Easwar Hariharan <eahariha@...ux.microsoft.com>,
	Saurabh Sengar <ssengar@...ux.microsoft.com>
Subject: Re: [PATCH v2 2/2] vfio: Fix unbalanced vfio_df_close call in
 no-iommu mode

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@...ux.microsoft.com> wrote:
> 
> > From: Jason Gunthorpe <jgg@...dia.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@...dia.com>
> > Tested-by: Jacob Pan <jacob.pan@...ux.microsoft.com>
> > Signed-off-by: Jacob Pan <jacob.pan@...ux.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