[Intel-gfx] [PATCH 04/10] vfio: Move storage of allow_unsafe_interrupts to vfio_main.c

[Jason Gunthorpe]

On Mon, Nov 07, 2022 at 08:18:53AM -0700, Alex Williamson wrote:
> On Mon, 7 Nov 2022 09:19:43 -0400
> Jason Gunthorpe <jgg at nvidia.com> wrote:
> 
> > On Mon, Oct 31, 2022 at 04:45:26PM -0600, Alex Williamson wrote:
> > 
> > > > It is one idea, it depends how literal you want to be on "module
> > > > parameters are ABI". IMHO it is a weak form of ABI and the need of
> > > > this paramter in particular is not that common in modern times, AFAIK.
> > > > 
> > > > So perhaps we just also expose it through vfio.ko and expect people to
> > > > migrate. That would give a window were both options are available.  
> > > 
> > > That might be best.  Ultimately this is an opt-out of a feature that
> > > has security implications, so I'd rather error on the side of requiring
> > > the user to re-assert that opt-out.  It seems the potential good in
> > > eliminating stale or unnecessary options outweighs any weak claims of
> > > preserving an ABI for a module that's no longer in service.  
> > 
> > Ok, lets do this
> > 
> > --- a/drivers/vfio/vfio_main.c
> > +++ b/drivers/vfio/vfio_main.c
> > @@ -55,6 +55,11 @@ static struct vfio {
> >  bool vfio_allow_unsafe_interrupts;
> >  EXPORT_SYMBOL_GPL(vfio_allow_unsafe_interrupts);
> >  
> > +module_param_named(allow_unsafe_interrupts,
> > +                  vfio_allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR);
> > +MODULE_PARM_DESC(allow_unsafe_interrupts,
> > +                "Enable VFIO IOMMU support for on platforms without interrupt remapping support.");
> > +
> >  static DEFINE_XARRAY(vfio_device_set_xa);
> >  static const struct file_operations vfio_group_fops;
> > 
> > > However, I'd question whether vfio is the right place for that new
> > > module option.  As proposed, vfio is only passing it through to
> > > iommufd, where an error related to lack of the hardware feature is
> > > masked behind an -EPERM by the time it gets back to vfio, making any
> > > sort of advisory to the user about the module option convoluted.  It
> > > seems like iommufd should own the option to opt-out universally, not
> > > just through the vfio use case.  Thanks,  
> > 
> > My thinking is this option shouldn't exist at all in other iommufd
> > users. eg I don't see value in VDPA supporting it.
> 
> I disagree, the IOMMU interface is responsible for isolating the
> device, this option doesn't make any sense to live in vfio-main, which
> is the reason it was always a type1 option.  

You just agreed to this above?

> If vdpa doesn't allow full device access such that it can guarantee
> that a device cannot generate a DMA that can spoof MSI, then it
> sounds like the flag we pass when attaching a device to iommfd
> should to reflect this difference in usage.

VDPA allows arbitary DMA just like VFIO. At most VDPA limits the MMIO
touches.

> The driver either requires full isolation, default, or can indicate a
> form of restricted DMA programming that prevents interrupt spoofing.
> The policy whether to permit unsafe configurations should exist in one
> place, iommufd.

iommufd doesn't know the level of unsafely the external driver is
creating, and IMHO we don't actually want to enable this more
widely. So I don't want to see a global kernel wide flag at this point
until we get reason to make more than just VFIO insecure.

Jason