| Message ID | 20220104015644.2294354-5-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Fix BUG_ON in vfio_iommu_group_notifier() | expand |
On Tue, Jan 04, 2022 at 09:56:34AM +0800, Lu Baolu wrote: > Multiple platform devices may be placed in the same IOMMU group because > they cannot be isolated from each other. These devices must either be > entirely under kernel control or userspace control, never a mixture. This > checks and sets DMA ownership during driver binding, and release the > ownership during driver unbinding. > > The device driver may set a new flag (no_kernel_api_dma) to skip calling > iommu_device_use_dma_api() during the binding process. For instance, the > userspace framework drivers (vfio etc.) which need to manually claim > their own dma ownership when assigning the device to userspace. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > include/linux/platform_device.h | 1 + > drivers/base/platform.c | 20 ++++++++++++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h > index 17fde717df68..8f0eaafcef47 100644 > --- a/include/linux/platform_device.h > +++ b/include/linux/platform_device.h > @@ -210,6 +210,7 @@ struct platform_driver { > struct device_driver driver; > const struct platform_device_id *id_table; > bool prevent_deferred_probe; > + bool no_kernel_api_dma; > }; > > #define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index b4d36b46ab2e..d5171e44d903 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -30,6 +30,7 @@ > #include <linux/property.h> > #include <linux/kmemleak.h> > #include <linux/types.h> > +#include <linux/iommu.h> > > #include "base.h" > #include "power/power.h" > @@ -1451,9 +1452,16 @@ static void platform_shutdown(struct device *_dev) > > static int platform_dma_configure(struct device *dev) > { > + struct platform_driver *drv = to_platform_driver(dev->driver); > enum dev_dma_attr attr; > int ret = 0; > > + if (!drv->no_kernel_api_dma) { > + ret = iommu_device_use_dma_api(dev); > + if (ret) > + return ret; > + } > + > if (dev->of_node) { > ret = of_dma_configure(dev, dev->of_node, true); > } else if (has_acpi_companion(dev)) { > @@ -1461,9 +1469,20 @@ static int platform_dma_configure(struct device *dev) > ret = acpi_dma_configure(dev, attr); > } > > + if (ret && !drv->no_kernel_api_dma) > + iommu_device_unuse_dma_api(dev); So you are now going to call this for every platform driver _unless_ they set this flag? And having "negative" flags is rough to parse at times. Yes, we have "prevent_deferred_probe" already here, so maybe keep this in the same nameing scheme and use "prevent_dma_api"? And it would be good to document this somewhere as to what this means. thanks, greg k-h
On Mon, Feb 14, 2022 at 10:59:50AM +0100, Greg Kroah-Hartman wrote: > > + if (ret && !drv->no_kernel_api_dma) > > + iommu_device_unuse_dma_api(dev); > > So you are now going to call this for every platform driver _unless_ > they set this flag? Yes, it is necessary because VFIO supports platform devices as well and needs to ensure security. Conflicting kernel driver attachements must be blocked, just like for PCI. Jason
On Mon, Feb 14, 2022 at 09:18:53AM -0400, Jason Gunthorpe wrote: > On Mon, Feb 14, 2022 at 10:59:50AM +0100, Greg Kroah-Hartman wrote: > > > > + if (ret && !drv->no_kernel_api_dma) > > > + iommu_device_unuse_dma_api(dev); > > > > So you are now going to call this for every platform driver _unless_ > > they set this flag? > > Yes, it is necessary because VFIO supports platform devices as well > and needs to ensure security. Conflicting kernel driver attachements > must be blocked, just like for PCI. A platform device shouldn't be using VFIO, but ugh, oh well, that ship has sailed :( And stop it with the "security" mess, do not give people a false sense of it here please. thanks, greg k-h
On Mon, Feb 14, 2022 at 02:37:15PM +0100, Greg Kroah-Hartman wrote: > On Mon, Feb 14, 2022 at 09:18:53AM -0400, Jason Gunthorpe wrote: > > On Mon, Feb 14, 2022 at 10:59:50AM +0100, Greg Kroah-Hartman wrote: > > > > > > + if (ret && !drv->no_kernel_api_dma) > > > > + iommu_device_unuse_dma_api(dev); > > > > > > So you are now going to call this for every platform driver _unless_ > > > they set this flag? > > > > Yes, it is necessary because VFIO supports platform devices as well > > and needs to ensure security. Conflicting kernel driver attachements > > must be blocked, just like for PCI. > > A platform device shouldn't be using VFIO, but ugh, oh well, that ship > has sailed :( I don't know why you say that, but yes, this is was set long ago. > And stop it with the "security" mess, do not give people a false sense > of it here please. I'm confused by what you mean. This is all about what we tend to refer to as DMA security - meaning a device's DMA can be controled by a hostile environment and not impact the integrity of the kernel. Jason
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h index 17fde717df68..8f0eaafcef47 100644 --- a/include/linux/platform_device.h +++ b/include/linux/platform_device.h @@ -210,6 +210,7 @@ struct platform_driver { struct device_driver driver; const struct platform_device_id *id_table; bool prevent_deferred_probe; + bool no_kernel_api_dma; }; #define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ diff --git a/drivers/base/platform.c b/drivers/base/platform.c index b4d36b46ab2e..d5171e44d903 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -30,6 +30,7 @@ #include <linux/property.h> #include <linux/kmemleak.h> #include <linux/types.h> +#include <linux/iommu.h> #include "base.h" #include "power/power.h" @@ -1451,9 +1452,16 @@ static void platform_shutdown(struct device *_dev) static int platform_dma_configure(struct device *dev) { + struct platform_driver *drv = to_platform_driver(dev->driver); enum dev_dma_attr attr; int ret = 0; + if (!drv->no_kernel_api_dma) { + ret = iommu_device_use_dma_api(dev); + if (ret) + return ret; + } + if (dev->of_node) { ret = of_dma_configure(dev, dev->of_node, true); } else if (has_acpi_companion(dev)) { @@ -1461,9 +1469,20 @@ static int platform_dma_configure(struct device *dev) ret = acpi_dma_configure(dev, attr); } + if (ret && !drv->no_kernel_api_dma) + iommu_device_unuse_dma_api(dev); + return ret; } +static void platform_dma_cleanup(struct device *dev) +{ + struct platform_driver *drv = to_platform_driver(dev->driver); + + if (!drv->no_kernel_api_dma) + iommu_device_unuse_dma_api(dev); +} + static const struct dev_pm_ops platform_dev_pm_ops = { SET_RUNTIME_PM_OPS(pm_generic_runtime_suspend, pm_generic_runtime_resume, NULL) USE_PLATFORM_PM_SLEEP_OPS @@ -1478,6 +1497,7 @@ struct bus_type platform_bus_type = { .remove = platform_remove, .shutdown = platform_shutdown, .dma_configure = platform_dma_configure, + .dma_cleanup = platform_dma_cleanup, .pm = &platform_dev_pm_ops, }; EXPORT_SYMBOL_GPL(platform_bus_type);
Multiple platform devices may be placed in the same IOMMU group because they cannot be isolated from each other. These devices must either be entirely under kernel control or userspace control, never a mixture. This checks and sets DMA ownership during driver binding, and release the ownership during driver unbinding. The device driver may set a new flag (no_kernel_api_dma) to skip calling iommu_device_use_dma_api() during the binding process. For instance, the userspace framework drivers (vfio etc.) which need to manually claim their own dma ownership when assigning the device to userspace. Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- include/linux/platform_device.h | 1 + drivers/base/platform.c | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+)