| 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
| ||
|
Message-ID: <20250616134004.60105ad5.alex.williamson@redhat.com>
Date: Mon, 16 Jun 2025 13:40:04 -0600
From: Alex Williamson <alex.williamson@...hat.com>
To: Jason Gunthorpe <jgg@...dia.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, 16 Jun 2025 12:34:55 -0300
Jason Gunthorpe <jgg@...dia.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@...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.
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
Powered by blists - more mailing lists