Re: [RFC PATCH] driver core: Stop driver bind on NOTIFY_BAD

[Greg KH]

On Mon, Jun 26, 2017 at 01:35:50PM -0600, Alex Williamson wrote:
> Allow participants in the BUS_NOTIFY_BIND_DRIVER to prevent driver
> binding with a NOTIFY_BAD.  An example case where this might be useful
> is when we're dealing with IOMMU groups and userspace drivers.  We've
> defined that devices within the same IOMMU group are not necessarily
> DMA isolated from one another and therefore allowing those devices to
> be split between host and user drivers may compromise the kernel.  The
> vfio driver currently handles this with a BUG_ON when such a condition
> occurs.  A better solution is to prevent the case from occurring,
> which this change enables.
> 
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Suggested-by: Russell King <linux@xxxxxxxxxxxxxxx>
> ---
>  drivers/base/dd.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> For due diligence, none of the current notifier blocks registered with
> bus_register_notifier() return anything other than { 0, NOTIFY_OK,
> NOTIFY_DONE } except for the case of BUS_NOTIFY_ADD_DEVICE, where
> NOTIFY_BAD is possible for NULL data in keystone_platform_notifier()
> and an errno return is possible from tce_iommu_bus_notifier() and
> i2cdev_notifier_call().  device_add() also ignores the call chain
> return value, so these three cases are all ineffective at preventing
> anything.
> 
> If this is acceptable, I'll re-spin https://lkml.org/lkml/2017/6/20/681
> dropping the last 3 patches, instead using the patch below, plumbing
> the IOMMU group notifier to percolate notifier block returns, and
> simply return NOTIFY_BAD from vfio rather than mucking with
> driver_override.  Thanks
> 
> Alex 
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 4882f06d12df..32c1d841e8d9 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -265,9 +265,11 @@ static int driver_sysfs_add(struct device *dev)
>  {
>  	int ret;
>  
> -	if (dev->bus)
> -		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> -					     BUS_NOTIFY_BIND_DRIVER, dev);
> +	if (dev->bus) {
> +		if (blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> +				BUS_NOTIFY_BIND_DRIVER, dev) == NOTIFY_BAD)
> +			return -EINVAL;
> +	}

Ick, this seems really hacky.  Right now we do not do anything on any of
the bus notifiers, so why start doing it now?

How is this ever being called anyway?  Your driver should have rejected
being bound to the device at all, much earlier in your probe function.

Or are you somehow trying to keep userspace from manually binding the
driver to the device?  If so, why not just disable that functionality
for it (there is a bit somewhere for that...)

thanks,

greg k-h