[PATCH v2 1/2] vfio: Prevent open_count decrement to negative

[PATCH v2 1/2] vfio: Prevent open_count decrement to negative

[Jacob Pan]

When vfio_df_close() is called with open_count=0, it triggers a warning in
vfio_assert_device_open() but still decrements open_count to -1. This
allows a subsequent open to incorrectly pass the open_count == 0 check,
leading to unintended behavior, such as setting df->access_granted = true.

For example, running an IOMMUFD compat no-IOMMU device with VFIO tests
(https://github.com/awilliam/tests/blob/master/vfio-noiommu-pci-device-open.c)
results in a warning and a failed VFIO_GROUP_GET_DEVICE_FD ioctl on the
first run, but the second run succeeds incorrectly.

Add checks to avoid decrementing open_count below zero.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Jacob Pan <jacob.pan@linux.microsoft.com>
---
v2: Added Reviewed-by tags
---
 drivers/vfio/vfio_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 1fd261efc582..5046cae05222 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -583,7 +583,8 @@ void vfio_df_close(struct vfio_device_file *df)
 
 	lockdep_assert_held(&device->dev_set->lock);
 
-	vfio_assert_device_open(device);
+	if (!vfio_assert_device_open(device))
+		return;
 	if (device->open_count == 1)
 		vfio_df_device_last_close(df);
 	device->open_count--;
-- 
2.34.1

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

[Jacob Pan]

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)) {
 		ret = vfio_iommufd_compat_attach_ioas(device, df->iommufd);
 		if (ret)
 			goto out_close_device;
diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
index c8c3a2d53f86..26c9c3068c77 100644
--- a/drivers/vfio/iommufd.c
+++ b/drivers/vfio/iommufd.c
@@ -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;
-
 	if (vdev->ops->unbind_iommufd)
 		vdev->ops->unbind_iommufd(vdev);
 }
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 5046cae05222..ac2dbd4e5d04 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -506,17 +506,19 @@ static int vfio_df_device_first_open(struct vfio_device_file *df)
 {
 	struct vfio_device *device = df->device;
 	struct iommufd_ctx *iommufd = df->iommufd;
-	int ret;
+	int ret = 0;
 
 	lockdep_assert_held(&device->dev_set->lock);
 
 	if (!try_module_get(device->dev->driver->owner))
 		return -ENODEV;
 
-	if (iommufd)
-		ret = vfio_df_iommufd_bind(df);
-	else
+	if (iommufd) {
+		if (!vfio_device_is_noiommu(device))
+			ret = vfio_df_iommufd_bind(df);
+	} else {
 		ret = vfio_device_group_use_iommu(device);
+	}
 	if (ret)
 		goto err_module_put;
 
@@ -528,10 +530,12 @@ static int vfio_df_device_first_open(struct vfio_device_file *df)
 	return 0;
 
 err_unuse_iommu:
-	if (iommufd)
-		vfio_df_iommufd_unbind(df);
-	else
+	if (iommufd) {
+		if (!vfio_device_is_noiommu(device))
+			vfio_df_iommufd_unbind(df);
+	} else {
 		vfio_device_group_unuse_iommu(device);
+	}
 err_module_put:
 	module_put(device->dev->driver->owner);
 	return ret;
@@ -546,10 +550,12 @@ static void vfio_df_device_last_close(struct vfio_device_file *df)
 
 	if (device->ops->close_device)
 		device->ops->close_device(device);
-	if (iommufd)
-		vfio_df_iommufd_unbind(df);
-	else
+	if (iommufd) {
+		if (!vfio_device_is_noiommu(device))
+			vfio_df_iommufd_unbind(df);
+	} else {
 		vfio_device_group_unuse_iommu(device);
+	}
 	module_put(device->dev->driver->owner);
 }
 
-- 
2.34.1

Re: [PATCH v2 1/2] vfio: Prevent open_count decrement to negative

[Alex Williamson]

Hi Jacob,

On Tue,  3 Jun 2025 08:23:42 -0700
Jacob Pan <jacob.pan@linux.microsoft.com> wrote:

> When vfio_df_close() is called with open_count=0, it triggers a warning in
> vfio_assert_device_open() but still decrements open_count to -1. This
> allows a subsequent open to incorrectly pass the open_count == 0 check,
> leading to unintended behavior, such as setting df->access_granted = true.
> 
> For example, running an IOMMUFD compat no-IOMMU device with VFIO tests
> (https://github.com/awilliam/tests/blob/master/vfio-noiommu-pci-device-open.c)
> results in a warning and a failed VFIO_GROUP_GET_DEVICE_FD ioctl on the
> first run, but the second run succeeds incorrectly.
> 
> Add checks to avoid decrementing open_count below zero.

The example above suggests to me that this is a means by which we could
see this, but in reality it seems it is the only means by which we can
create this scenario, right?

Why does VFIO_GROUP_GET_DEVICE_FD fail on the first iteration?  It
seems like things are pretty broken, we won't have access_granted set,
but I don't spot why the ioctl fails.

Doesn't this also (begin) to fix 6086efe73498 as well?  I think that
introduced skipping vfio_df_open() entirely for noiommu devices.  It
seems like this should have a Fixes: tag and the warning in the
assertion was unreachable until 608... so maybe it should be linked
here.  Thanks,

Alex


> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Jacob Pan <jacob.pan@linux.microsoft.com>
> ---
> v2: Added Reviewed-by tags
> ---
>  drivers/vfio/vfio_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 1fd261efc582..5046cae05222 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -583,7 +583,8 @@ void vfio_df_close(struct vfio_device_file *df)
>  
>  	lockdep_assert_held(&device->dev_set->lock);
>  
> -	vfio_assert_device_open(device);
> +	if (!vfio_assert_device_open(device))
> +		return;
>  	if (device->open_count == 1)
>  		vfio_df_device_last_close(df);
>  	device->open_count--;

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

[Alex Williamson]

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?

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;


>  		ret = vfio_iommufd_compat_attach_ioas(device, df->iommufd);
>  		if (ret)
>  			goto out_close_device;
> diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> index c8c3a2d53f86..26c9c3068c77 100644
> --- a/drivers/vfio/iommufd.c
> +++ b/drivers/vfio/iommufd.c
> @@ -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,

Alex

>  	if (vdev->ops->unbind_iommufd)
>  		vdev->ops->unbind_iommufd(vdev);
>  }
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 5046cae05222..ac2dbd4e5d04 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -506,17 +506,19 @@ static int vfio_df_device_first_open(struct vfio_device_file *df)
>  {
>  	struct vfio_device *device = df->device;
>  	struct iommufd_ctx *iommufd = df->iommufd;
> -	int ret;
> +	int ret = 0;
>  
>  	lockdep_assert_held(&device->dev_set->lock);
>  
>  	if (!try_module_get(device->dev->driver->owner))
>  		return -ENODEV;
>  
> -	if (iommufd)
> -		ret = vfio_df_iommufd_bind(df);
> -	else
> +	if (iommufd) {
> +		if (!vfio_device_is_noiommu(device))
> +			ret = vfio_df_iommufd_bind(df);
> +	} else {
>  		ret = vfio_device_group_use_iommu(device);
> +	}
>  	if (ret)
>  		goto err_module_put;
>  
> @@ -528,10 +530,12 @@ static int vfio_df_device_first_open(struct vfio_device_file *df)
>  	return 0;
>  
>  err_unuse_iommu:
> -	if (iommufd)
> -		vfio_df_iommufd_unbind(df);
> -	else
> +	if (iommufd) {
> +		if (!vfio_device_is_noiommu(device))
> +			vfio_df_iommufd_unbind(df);
> +	} else {
>  		vfio_device_group_unuse_iommu(device);
> +	}
>  err_module_put:
>  	module_put(device->dev->driver->owner);
>  	return ret;
> @@ -546,10 +550,12 @@ static void vfio_df_device_last_close(struct vfio_device_file *df)
>  
>  	if (device->ops->close_device)
>  		device->ops->close_device(device);
> -	if (iommufd)
> -		vfio_df_iommufd_unbind(df);
> -	else
> +	if (iommufd) {
> +		if (!vfio_device_is_noiommu(device))
> +			vfio_df_iommufd_unbind(df);
> +	} else {
>  		vfio_device_group_unuse_iommu(device);
> +	}
>  	module_put(device->dev->driver->owner);
>  }
>  

Re: [PATCH v2 1/2] vfio: Prevent open_count decrement to negative

[Jason Gunthorpe]

On Fri, Jun 13, 2025 at 04:31:00PM -0600, Alex Williamson wrote:
> Hi Jacob,
> 
> On Tue,  3 Jun 2025 08:23:42 -0700
> Jacob Pan <jacob.pan@linux.microsoft.com> wrote:
> 
> > When vfio_df_close() is called with open_count=0, it triggers a warning in
> > vfio_assert_device_open() but still decrements open_count to -1. This
> > allows a subsequent open to incorrectly pass the open_count == 0 check,
> > leading to unintended behavior, such as setting df->access_granted = true.
> > 
> > For example, running an IOMMUFD compat no-IOMMU device with VFIO tests
> > (https://github.com/awilliam/tests/blob/master/vfio-noiommu-pci-device-open.c)
> > results in a warning and a failed VFIO_GROUP_GET_DEVICE_FD ioctl on the
> > first run, but the second run succeeds incorrectly.
> > 
> > Add checks to avoid decrementing open_count below zero.
> 
> The example above suggests to me that this is a means by which we could
> see this, but in reality it seems it is the only means by which we can
> create this scenario, right?

I understood this as an assertion hit because of the bug fixed in
patch 2 and thus the missed assertion error handling flow was noticed.

Obviously the assertion should never happen, but if it does we should
try to recover better than we currently do.

Jason

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

[Jason Gunthorpe]

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.

> 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

Re: [PATCH v2 1/2] vfio: Prevent open_count decrement to negative

[Alex Williamson]

On Fri, 13 Jun 2025 21:09:26 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Fri, Jun 13, 2025 at 04:31:00PM -0600, Alex Williamson wrote:
> > Hi Jacob,
> > 
> > On Tue,  3 Jun 2025 08:23:42 -0700
> > Jacob Pan <jacob.pan@linux.microsoft.com> wrote:
> >   
> > > When vfio_df_close() is called with open_count=0, it triggers a warning in
> > > vfio_assert_device_open() but still decrements open_count to -1. This
> > > allows a subsequent open to incorrectly pass the open_count == 0 check,
> > > leading to unintended behavior, such as setting df->access_granted = true.
> > > 
> > > For example, running an IOMMUFD compat no-IOMMU device with VFIO tests
> > > (https://github.com/awilliam/tests/blob/master/vfio-noiommu-pci-device-open.c)
> > > results in a warning and a failed VFIO_GROUP_GET_DEVICE_FD ioctl on the
> > > first run, but the second run succeeds incorrectly.
> > > 
> > > Add checks to avoid decrementing open_count below zero.  
> > 
> > The example above suggests to me that this is a means by which we could
> > see this, but in reality it seems it is the only means by which we can
> > create this scenario, right?  
> 
> I understood this as an assertion hit because of the bug fixed in
> patch 2 and thus the missed assertion error handling flow was noticed.
> 
> Obviously the assertion should never happen, but if it does we should
> try to recover better than we currently do.

Certainly.  My statement is trying to determine the scope of the issue
from a stable perspective.  Maybe I'm interpreting "[f]or example" too
broadly, but I think this is unreachable outside of the specific
described scenario, ie. using iommufd in compatibility mode with
no-iommu.  Further, it only became reachable with 6086efe73498.

In any case, it fixes something and we should attribute that something,
whether it's 6086efe73498 or we want to reach back to when the assert
was introduced and claim it should have had a return even if it was
unreachable.

It seems these patches should also be re-ordered if not rolled into
one.  Fixing the issue in 2/ makes this once again unreachable, so I
don't mind it coming along as a "also handle this error case better."
This alone doesn't really do much.  Thanks,

Alex

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

[Alex Williamson]

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.

TBH, it seems like special casing iommufd in the device file layer is
what led to the issue introduced in 6086efe73498.

> > 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.

Or check it in bind since we already check it in unbind.  Either way,
symmetry.
 
> 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,

Alex

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

[Jason Gunthorpe]

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.

Jason

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

[Alex Williamson]

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

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

[Jason Gunthorpe]

On Mon, Jun 16, 2025 at 01:40:04PM -0600, Alex Williamson wrote:
> --- 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,

Just comparing to the original

+	if (iommufd) {
+		if (!vfio_device_is_noiommu(device))
+			ret = vfio_df_iommufd_bind(df);

Isn't being captured, so it needs another hunk in
vfio_df_iommufd_bind()

Jason

Re: [PATCH v2 1/2] vfio: Prevent open_count decrement to negative

[Jacob Pan]

Hi Alex,

On Mon, 16 Jun 2025 08:40:01 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Fri, 13 Jun 2025 21:09:26 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Fri, Jun 13, 2025 at 04:31:00PM -0600, Alex Williamson wrote:  
> > > Hi Jacob,
> > > 
> > > On Tue,  3 Jun 2025 08:23:42 -0700
> > > Jacob Pan <jacob.pan@linux.microsoft.com> wrote:
> > >     
> > > > When vfio_df_close() is called with open_count=0, it triggers a
> > > > warning in vfio_assert_device_open() but still decrements
> > > > open_count to -1. This allows a subsequent open to incorrectly
> > > > pass the open_count == 0 check, leading to unintended behavior,
> > > > such as setting df->access_granted = true.
> > > > 
> > > > For example, running an IOMMUFD compat no-IOMMU device with
> > > > VFIO tests
> > > > (https://github.com/awilliam/tests/blob/master/vfio-noiommu-pci-device-open.c)
> > > > results in a warning and a failed VFIO_GROUP_GET_DEVICE_FD
> > > > ioctl on the first run, but the second run succeeds incorrectly.
> > > > 
> > > > Add checks to avoid decrementing open_count below zero.    
> > > 
> > > The example above suggests to me that this is a means by which we
> > > could see this, but in reality it seems it is the only means by
> > > which we can create this scenario, right?    
> > 
> > I understood this as an assertion hit because of the bug fixed in
> > patch 2 and thus the missed assertion error handling flow was
> > noticed.
> > 
> > Obviously the assertion should never happen, but if it does we
> > should try to recover better than we currently do.  
> 
> Certainly.  My statement is trying to determine the scope of the issue
> from a stable perspective.  Maybe I'm interpreting "[f]or example" too
> broadly, but I think this is unreachable outside of the specific
> described scenario, ie. using iommufd in compatibility mode with
> no-iommu.  Further, it only became reachable with 6086efe73498.
> 
> In any case, it fixes something and we should attribute that
> something, whether it's 6086efe73498 or we want to reach back to when
> the assert was introduced and claim it should have had a return even
> if it was unreachable.
> 
> It seems these patches should also be re-ordered if not rolled into
> one.  Fixing the issue in 2/ makes this once again unreachable, so I
> don't mind it coming along as a "also handle this error case better."
> This alone doesn't really do much.  Thanks,
> 
IMHO, this is an independent exception handling fix, perhaps I just add
the missing tag below?

Fixes: 05f37e1c03b6 ("vfio: Pass struct vfio_device_file * to
vfio_device_open/close()")

Thanks,

Jacob

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

[Jacob Pan]

Hi Jason,

On Mon, 16 Jun 2025 17:05:58 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Mon, Jun 16, 2025 at 01:40:04PM -0600, Alex Williamson wrote:
> > --- 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,  
> 
> Just comparing to the original
> 
> +	if (iommufd) {
> +		if (!vfio_device_is_noiommu(device))
> +			ret = vfio_df_iommufd_bind(df);
> 
> Isn't being captured, so it needs another hunk in
> vfio_df_iommufd_bind()
> 
OK, will send out v3 with this concise fix. I am also working on the
noiommu special mode as you suggested, then we can allow bind again.

Thanks,

Jacob

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

[Jason Gunthorpe]

On Wed, Jun 18, 2025 at 04:11:36PM -0700, Jacob Pan wrote:
> Hi Jason,
> 
> On Mon, 16 Jun 2025 17:05:58 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Mon, Jun 16, 2025 at 01:40:04PM -0600, Alex Williamson wrote:
> > > --- 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,  
> > 
> > Just comparing to the original
> > 
> > +	if (iommufd) {
> > +		if (!vfio_device_is_noiommu(device))
> > +			ret = vfio_df_iommufd_bind(df);
> > 
> > Isn't being captured, so it needs another hunk in
> > vfio_df_iommufd_bind()
> > 
> OK, will send out v3 with this concise fix. I am also working on the
> noiommu special mode as you suggested, then we can allow bind again.

Sure sounds good

Jason