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

[Jacob Pan <jacob.pan@linux.microsoft.com>]

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