| Message ID | 20220104015644.2294354-3-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 |
All these bus callouts still looks horrible and just create tons of boilerplate code.
On Tue, Jan 04, 2022 at 02:08:36AM -0800, Christoph Hellwig wrote: > All these bus callouts still looks horrible and just create tons of > boilerplate code. Yes, Lu - Greg asked questions then didn't respond to their answers meaning he accepts them, you should stick with the v4 version. Jason
On Tue, Jan 04, 2022 at 08:39:11AM -0400, Jason Gunthorpe wrote: > On Tue, Jan 04, 2022 at 02:08:36AM -0800, Christoph Hellwig wrote: > > All these bus callouts still looks horrible and just create tons of > > boilerplate code. > > Yes, Lu - Greg asked questions then didn't respond to their answers > meaning he accepts them, you should stick with the v4 version. Trying to catch up on emails from the break, that was way down my list of things to get back to as it's messy and non-obvious. I'll revisit it again after 5.17-rc1 is out, this is too late for that merge window anyway. thanks, greg k-h
Hi Greg, On 1/4/22 9:04 PM, Greg Kroah-Hartman wrote: > On Tue, Jan 04, 2022 at 08:39:11AM -0400, Jason Gunthorpe wrote: >> On Tue, Jan 04, 2022 at 02:08:36AM -0800, Christoph Hellwig wrote: >>> All these bus callouts still looks horrible and just create tons of >>> boilerplate code. >> >> Yes, Lu - Greg asked questions then didn't respond to their answers >> meaning he accepts them, you should stick with the v4 version. > > Trying to catch up on emails from the break, that was way down my list > of things to get back to as it's messy and non-obvious. I'll revisit it > again after 5.17-rc1 is out, this is too late for that merge window > anyway. In this series we want to add calls into the iommu subsystem during device driver binding/unbinding, so that the device DMA ownership conflict (kernel driver vs. user-space) could be detected and avoided before calling into device driver's .probe(). In this v5 series, we implemented this in the affected buses (amba/ platform/fsl-mc/pci) which are known to support assigning devices to user space through the vfio framework currently. And more buses are possible to be affected in the future if they also want to support device assignment. Christoph commented that this will create boilerplate code in various bus drivers. Back to v4 of this series (please refer to below link [1]), we added this call in the driver core if buses have provided the dma_configure() callback (please refer to below link [2]). Which would you prefer, or any other suggestions? We need your guide to move this series ahead. Please help to suggest. [1] https://lore.kernel.org/linux-iommu/20211217063708.1740334-1-baolu.lu@linux.intel.com/ [2] https://lore.kernel.org/linux-iommu/20211217063708.1740334-3-baolu.lu@linux.intel.com/ Best regards, baolu
On Tue, Feb 08, 2022 at 01:55:29PM +0800, Lu Baolu wrote: > Hi Greg, > > On 1/4/22 9:04 PM, Greg Kroah-Hartman wrote: > > On Tue, Jan 04, 2022 at 08:39:11AM -0400, Jason Gunthorpe wrote: > > > On Tue, Jan 04, 2022 at 02:08:36AM -0800, Christoph Hellwig wrote: > > > > All these bus callouts still looks horrible and just create tons of > > > > boilerplate code. > > > > > > Yes, Lu - Greg asked questions then didn't respond to their answers > > > meaning he accepts them, you should stick with the v4 version. > > > > Trying to catch up on emails from the break, that was way down my list > > of things to get back to as it's messy and non-obvious. I'll revisit it > > again after 5.17-rc1 is out, this is too late for that merge window > > anyway. > > In this series we want to add calls into the iommu subsystem during > device driver binding/unbinding, so that the device DMA ownership > conflict (kernel driver vs. user-space) could be detected and avoided > before calling into device driver's .probe(). > > In this v5 series, we implemented this in the affected buses (amba/ > platform/fsl-mc/pci) which are known to support assigning devices to > user space through the vfio framework currently. And more buses are > possible to be affected in the future if they also want to support > device assignment. Christoph commented that this will create boilerplate > code in various bus drivers. > > Back to v4 of this series (please refer to below link [1]), we added > this call in the driver core if buses have provided the dma_configure() > callback (please refer to below link [2]). > > Which would you prefer, or any other suggestions? We need your guide to > move this series ahead. Please help to suggest. > > [1] https://lore.kernel.org/linux-iommu/20211217063708.1740334-1-baolu.lu@linux.intel.com/ > [2] https://lore.kernel.org/linux-iommu/20211217063708.1740334-3-baolu.lu@linux.intel.com/ Let me look over the series again this afternooon. thanks, greg k-h
On Tue, Jan 04, 2022 at 02:08:36AM -0800, Christoph Hellwig wrote: > All these bus callouts still looks horrible and just create tons of > boilerplate code. I can't remember anymore what one vs. the other looks like. Having an explicit "opt-in" for a bus is good, in that no code breaks and only if you want to use this do you have to set the flag. Why do you want it to be "always"? thanks, greg k-h
On Tue, Jan 04, 2022 at 09:56:32AM +0800, Lu Baolu wrote: > The bus_type structure defines dma_configure() callback for bus drivers > to configure DMA on the devices. This adds the paired dma_cleanup() > callback and calls it during driver unbinding so that bus drivers can do > some cleanup work. > > One use case for this paired DMA callbacks is for the bus driver to check > for DMA ownership conflicts during driver binding, where multiple devices > belonging to a same IOMMU group (the minimum granularity of isolation and > protection) may be assigned to kernel drivers or user space respectively. > > Without this change, for example, the vfio driver has to listen to a bus > BOUND_DRIVER event and then BUG_ON() in case of dma ownership conflict. > This leads to bad user experience since careless driver binding operation > may crash the system if the admin overlooks the group restriction. Aside > from bad design, this leads to a security problem as a root user, even with > lockdown=integrity, can force the kernel to BUG. > > With this change, the bus driver could check and set the DMA ownership in > driver binding process and fail on ownership conflicts. The DMA ownership > should be released during driver unbinding. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h index a039ab809753..d8b29ccd07e5 100644 --- a/include/linux/device/bus.h +++ b/include/linux/device/bus.h @@ -59,6 +59,8 @@ struct fwnode_handle; * bus supports. * @dma_configure: Called to setup DMA configuration on a device on * this bus. + * @dma_cleanup: Called to cleanup DMA configuration on a device on + * this bus. * @pm: Power management operations of this bus, callback the specific * device driver's pm-ops. * @iommu_ops: IOMMU specific operations for this bus, used to attach IOMMU @@ -103,6 +105,7 @@ struct bus_type { int (*num_vf)(struct device *dev); int (*dma_configure)(struct device *dev); + void (*dma_cleanup)(struct device *dev); const struct dev_pm_ops *pm; diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 9eaaff2f556c..2bcbd358eda3 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -662,6 +662,8 @@ static int really_probe(struct device *dev, struct device_driver *drv) if (dev->bus) blocking_notifier_call_chain(&dev->bus->p->bus_notifier, BUS_NOTIFY_DRIVER_NOT_BOUND, dev); + if (dev->bus->dma_cleanup) + dev->bus->dma_cleanup(dev); pinctrl_bind_failed: device_links_no_driver(dev); devres_release_all(dev); @@ -1205,6 +1207,9 @@ static void __device_release_driver(struct device *dev, struct device *parent) else if (drv->remove) drv->remove(dev); + if (dev->bus->dma_cleanup) + dev->bus->dma_cleanup(dev); + device_links_driver_cleanup(dev); devres_release_all(dev);
The bus_type structure defines dma_configure() callback for bus drivers to configure DMA on the devices. This adds the paired dma_cleanup() callback and calls it during driver unbinding so that bus drivers can do some cleanup work. One use case for this paired DMA callbacks is for the bus driver to check for DMA ownership conflicts during driver binding, where multiple devices belonging to a same IOMMU group (the minimum granularity of isolation and protection) may be assigned to kernel drivers or user space respectively. Without this change, for example, the vfio driver has to listen to a bus BOUND_DRIVER event and then BUG_ON() in case of dma ownership conflict. This leads to bad user experience since careless driver binding operation may crash the system if the admin overlooks the group restriction. Aside from bad design, this leads to a security problem as a root user, even with lockdown=integrity, can force the kernel to BUG. With this change, the bus driver could check and set the DMA ownership in driver binding process and fail on ownership conflicts. The DMA ownership should be released during driver unbinding. Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- include/linux/device/bus.h | 3 +++ drivers/base/dd.c | 5 +++++ 2 files changed, 8 insertions(+)