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

[Alex Williamson <alex.williamson@redhat.com>]

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