linux-kernel - Re: [PATCH 1/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: <20250603092549.4fbbed92@DESKTOP-0403QTC.>
Date: Tue, 3 Jun 2025 09:25:49 -0700
From: Jacob Pan <jacob.pan@...ux.microsoft.com>
To: Yi Liu <yi.l.liu@...el.com>
Cc: Jason Gunthorpe <jgg@...dia.com>, <linux-kernel@...r.kernel.org>,
 "iommu@...ts.linux.dev" <iommu@...ts.linux.dev>, Alex Williamson
 <alex.williamson@...hat.com>, "Zhang Yu" <zhangyu1@...rosoft.com>, Easwar
 Hariharan <eahariha@...ux.microsoft.com>, jacob.pan@...ux.microsoft.com
Subject: Re: [PATCH 1/2] vfio: Fix unbalanced vfio_df_close call in no-iommu
 mode

Hi Yi,

On Wed, 28 May 2025 15:16:57 +0800
Yi Liu <yi.l.liu@...el.com> wrote:

> On 2025/5/27 08:05, Jason Gunthorpe wrote:
> > On Fri, May 16, 2025 at 09:45:21AM -0700, Jacob Pan wrote:  
> >> For no-iommu enabled devices working under IOMMUFD VFIO compat
> >> mode, the group open path does not call vfio_df_open() and the
> >> open_count is 0. So calling vfio_df_close() in the group close
> >> path will trigger warning in vfio_assert_device_open(device);
> >>
> >> E.g. The following warning can be seen by running VFIO test.
> >> https://github.com/awilliam/tests/blob/master/vfio-noiommu-pci-device-open.c
> >> CONFIG_VFIO_CONTAINER = n
> >> [   29.094781] vfio-pci 0000:02:01.0: vfio-noiommu device opened
> >> by user (vfio-noiommu-pc:164) Failed to get device info
> >> [   29.096540] ------------[ cut here ]------------
> >> [   29.096616] WARNING: CPU: 1 PID: 164 at
> >> drivers/vfio/vfio_main.c:487 vfio_df_close+0xac/0xb4
> >>
> >> This patch adds checks for no-iommu mode and open_count to skip
> >> calling vfio_df_close.  
> 
> thanks for catching it. :)
> 
> >> Signed-off-by: Jacob Pan <jacob.pan@...ux.microsoft.com>
> >> ---
> >>   drivers/vfio/group.c | 7 ++++---
> >>   1 file changed, 4 insertions(+), 3 deletions(-)  
> > 
> > Sorry, this should have a fixes line:
> > 
> > I think it is probably
> > 
> > Fixes: 6086efe73498 ("vfio-iommufd: Move noiommu compat validation
> > out of vfio_iommufd_bind()")
> > 
> > By the look of it, since that is what started skipping the
> > vfio_df_open()
> > 
> > But after looking at that patch I'm now doubting that this is the
> > right fix.
> > 
> > Previously we'd still do vfio_df_device_first_open(), just the
> > vfio_df_iommufd_bind() was skipped.
> > 
> > Now we skip all of vfio_df_device_first_open() which also means we
> > skip:
> > 
> > 	if (!try_module_get(device->dev->driver->owner))
> > 		return -ENODEV;
> > 
> > and
> > 	if (device->ops->open_device) {
> > 		ret = device->ops->open_device(device);
> > 
> > Which seems wrong to me?? We only want to skip the bind, we should
> > still do open_device! At least that is how it was before 6086e
> > 
> > So.. This may not be the right fix.  
> 
> yes. this makes sense. If not opened, userspace is not able to use the
> device.
> 
Put this bug aside for now, I'm still unclear on why we do not allow
bind for no-IOMMU devices. Per my understanding, no-IOMMU only means no
translation. But since device still has been granted access, we should
be able to allow binding device in no-IOMMU mode with IOMMU-FD
context while simply disallowing IOAS attachment?

The reason I am asking is that I am working on enabling cdev with
noiommu mode based on Yi's patch
(https://lore.kernel.org/kvm/20230601082413.22a55ac4.alex.williamson@redhat.com/),
it seems having iommufd implicit ownership model is key to enable PCI
HOT RESET. Our goal is to leverage persistent iommufd context for kexec
handle off (KHO) usage, we currently have noiommu mode. This requires
binding of device with iommufd ctx (can be marked as persistent) AFAIK,
any suggestions?"


Thanks,

Jacob