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: <20250616153455.GK1174925@nvidia.com>
Date: Mon, 16 Jun 2025 12:34: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 Mon, Jun 16, 2025 at 08:47:08AM -0600, Alex Williamson wrote:
> On Fri, 13 Jun 2025 21:15:55 -0300
> Jason Gunthorpe <jgg@...dia.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@...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.
> 
> 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.

Jason