On Thu, 15 Dec 2022 10:25:11 -0400
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> On Wed, Dec 14, 2022 at 02:42:29PM -0700, Alex Williamson wrote:
> > On Wed, 14 Dec 2022 13:24:52 -0800
> > Steve Sistare <steven.sistare@xxxxxxxxxx> wrote:
> >
> > > Fix bugs in the interfaces that allow the underlying memory object of an
> > > iova range to be mapped in a new address space. They allow userland to
> > > indefinitely block vfio mediated device kernel threads, and do not
> > > propagate the locked_vm count to a new mm.
> > >
> > > The fixes impose restrictions that eliminate waiting conditions, so
> > > revert the dead code:
> > > commit 898b9eaeb3fe ("vfio/type1: block on invalid vaddr")
> > > commit 487ace134053 ("vfio/type1: implement notify callback")
> > > commit ec5e32940cc9 ("vfio: iommu driver notify callback")
> > >
> > > Changes in V2 (thanks Alex):
> > > * do not allow group attach while vaddrs are invalid
> > > * add patches to delete dead code
> > > * add WARN_ON for never-should-happen conditions
> > > * check for changed mm in unmap.
> > > * check for vfio_lock_acct failure in remap
> > >
> > > Changes in V3 (ditto!):
> > > * return errno at WARN_ON sites, and make it unique
> > > * correctly check for dma task mm change
> > > * change dma owner to current when vaddr is updated
> > > * add Fixes to commit messages
> > > * refactored new code in vfio_dma_do_map
> > >
> > > Changes in V4:
> > > * misc cosmetic changes
> > >
> > > Steve Sistare (5):
> > > vfio/type1: exclude mdevs from VFIO_UPDATE_VADDR
> > > vfio/type1: prevent locked_vm underflow
> > > vfio/type1: revert "block on invalid vaddr"
> > > vfio/type1: revert "implement notify callback"
> > > vfio: revert "iommu driver notify callback"
> > >
> > > drivers/vfio/container.c | 5 -
> > > drivers/vfio/vfio.h | 7 --
> > > drivers/vfio/vfio_iommu_type1.c | 199 +++++++++++++++++++---------------------
> > > include/uapi/linux/vfio.h | 15 +--
> > > 4 files changed, 103 insertions(+), 123 deletions(-)
> > >
> >
> > Looks ok to me. Jason, Kevin, I'd appreciate your reviews regarding
> > whether this sufficiently addresses the outstanding issues to keep this
> > interface around until we have a re-implementation in iommufd. Thanks,
>
> Well, patch 3 undeniably solves the deadlock problem.
>
> I still don't like that we are keeping this, an interface that doesn't
> support mdevs has no future and can't really be used in any general
> purpose way.
>
> Can we at least protect it with a kconfig && CONFIG_EXPERIMENTAL so it
> is disabled in most builds?
We no longer have that tool, CONFIG_EXPERIMENTAL hasn't existed for
almost a decade.
I don't think anyone would argue this as a perfect solution, but at the
same time we're putting Steve in a position where we want to remove the
vulnerabilities, we're divesting development in type1 with the
introduction of iommufd, and there's clearly some utility remaining in
the interface, even without mdev support.
It's not that different from migration in fact. In order to migrate a
VM with vfio devices, all those devices need to support migration. In
order to live update a VM, all the device need to support live update,
where we have an implementation of generic support here for non-mdev
devices.
It looks like we need some more refinement of the patches, but I think
there's enough here to show there's an interim, restricted use case
solution short of reverting the feature entirely. Thanks,
Alex
