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

Jason Gunthorpe
2022-11-07T13:19:55Z

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.

So, let's wait and see if a need arises first. I'm reluctant to add
options to disable kernel security without really good reasons.

Jason