Re: [PATCH] vfio/type1: Cleanup remaining vaddr removal/update fragments

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

On Mon, 12 Dec 2022 19:35:40 -0400
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Mon, Dec 12, 2022 at 04:29:48PM -0700, Alex Williamson wrote:
> > On Mon, 12 Dec 2022 19:08:57 -0400
> > Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >   
> > > On Mon, Dec 12, 2022 at 02:26:51PM -0700, Alex Williamson wrote:  
> > > > On Mon, 12 Dec 2022 15:59:11 -0500
> > > > Steven Sistare <steven.sistare@oracle.com> wrote:
> > > >     
> > > > > On 12/12/2022 10:58 AM, Alex Williamson wrote:    
> > > > > > On Mon, 12 Dec 2022 09:17:54 -0400
> > > > > > Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > > > > >       
> > > > > >> On Sat, Dec 10, 2022 at 09:14:06AM -0500, Steven Sistare wrote:
> > > > > >>      
> > > > > >>> Thank you for your thoughtful response.  Rather than debate the degree of
> > > > > >>> of vulnerability, I propose an alternate solution.  The technical crux of
> > > > > >>> the matter is support for mediated devices.          
> > > > > >>
> > > > > >> I'm not sure I'm convinced about that. It is easy to make problematic
> > > > > >> situations with mdevs, but that doesn't mean other cases don't exist
> > > > > >> too eg what happens if userspace suspends and then immediately does
> > > > > >> something to trigger a domain attachment? Doesn't it still deadlock
> > > > > >> the kernel?      
> > > > > > 
> > > > > > The opportunity for that to deadlock isn't obvious to me, a replay
> > > > > > would be stalled waiting for invalid vaddrs, but this is essentially
> > > > > > the user deadlocking themselves.  There's also code there to handle the
> > > > > > process getting killed while waiting, making it interruptible.  Thanks,      
> > > > > 
> > > > > I will submit new patches tomorrow to exclude mdevs.  Almost done.    
> > > > 
> > > > I've dropped the removal commits from my next branch in the interim.    
> > > 
> > > Woah, please don't do that - I already built and sent pull requests
> > > assuming this, there are conflicts.  
> > 
> > I've done merges both ways with your iommufd pull request and don't see
> > any conflicts relative to these changes.  Kconfig, Makefile, and
> > vfio_main.c related to virq integration and group extraction are the
> > only conflicts.   
> 
> I got an extra hunk in the header file
> 
> > Besides, it's already pushed and I don't have any references to the
> > old head, so someone would need to provide it if we wanted to keep
> > the old hashes.  
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/tag/?h=for-linus-iommufd-merged
> https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/commit/?h=for-linus-iommufd-merged&id=e9a1f0f32d86c05f01878a0448384a46a453abc7

Ok, I do still have that reference around.  Thanks.

> > > Why would we not revert everything from 6.2 - that is what we agreed
> > > to do?  
> > 
> > The decision to revert was based on the current interface being buggy,
> > abandoned, and re-implemented.  It doesn't seem that there's much future
> > for the current interface, but Steve has stepped up to restrict the
> > current implementation to non-mdev devices, which resolves your concern
> > regarding unlimited user blocking of kernel threads afaict, and we'll
> > see what he does with locked memory.    
> 
> Except nobody has seen this yet, and it can't go into 6.2 at this
> point (see Linus's rather harsh remarks on late work for v6.2)

We already outlined earlier in this thread the criteria that prompted
us to tag the revert for stable, which was Steve's primary objection in
the short term.  I can't in good faith push forward with a revert,
including stable, if Steve is working on a proposal to resolve the
issues prompting us to accelerate the code removal.  Depending on the
scope of Steve's proposal, I think we might be able to still consider
this a fix for v6.2.  Thanks,

Alex