Re: [PATCH v4 05/16] drivers/iommu: Take a ref to the IOMMU driver prior to ->add_device()

[Greg Kroah-Hartman]

On Thu, Dec 19, 2019 at 12:03:41PM +0000, Will Deacon wrote:
> To avoid accidental removal of an active IOMMU driver module, take a
> reference to the driver module in 'iommu_probe_device()' immediately
> prior to invoking the '->add_device()' callback and hold it until the
> after the device has been removed by '->remove_device()'.
> 
> Suggested-by: Joerg Roedel <joro@xxxxxxxxxx>
> Signed-off-by: Will Deacon <will@xxxxxxxxxx>
> ---
>  drivers/iommu/iommu.c | 19 +++++++++++++++++--
>  include/linux/iommu.h |  4 +++-
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 7c92197d53f3..bc8edf90e729 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -22,6 +22,7 @@
>  #include <linux/bitops.h>
>  #include <linux/property.h>
>  #include <linux/fsl/mc.h>
> +#include <linux/module.h>
>  #include <trace/events/iommu.h>
>  
>  static struct kset *iommu_group_kset;
> @@ -185,10 +186,21 @@ int iommu_probe_device(struct device *dev)
>  	if (!iommu_get_dev_param(dev))
>  		return -ENOMEM;
>  
> +	if (!try_module_get(ops->owner)) {
> +		ret = -EINVAL;
> +		goto err_free_dev_param;
> +	}
> +
>  	ret = ops->add_device(dev);
>  	if (ret)
> -		iommu_free_dev_param(dev);
> +		goto err_module_put;
> +
> +	return 0;
>  
> +err_module_put:
> +	module_put(ops->owner);
> +err_free_dev_param:
> +	iommu_free_dev_param(dev);
>  	return ret;
>  }
>  
> @@ -199,7 +211,10 @@ void iommu_release_device(struct device *dev)
>  	if (dev->iommu_group)
>  		ops->remove_device(dev);
>  
> -	iommu_free_dev_param(dev);
> +	if (dev->iommu_param) {
> +		module_put(ops->owner);
> +		iommu_free_dev_param(dev);
> +	}
>  }
>  
>  static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index f2223cbb5fd5..e9f94d3f7a04 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -246,9 +246,10 @@ struct iommu_iotlb_gather {
>   * @sva_get_pasid: Get PASID associated to a SVA handle
>   * @page_response: handle page request response
>   * @cache_invalidate: invalidate translation caches
> - * @pgsize_bitmap: bitmap of all possible supported page sizes
>   * @sva_bind_gpasid: bind guest pasid and mm
>   * @sva_unbind_gpasid: unbind guest pasid and mm
> + * @pgsize_bitmap: bitmap of all possible supported page sizes
> + * @owner: Driver module providing these ops
>   */
>  struct iommu_ops {
>  	bool (*capable)(enum iommu_cap);
> @@ -318,6 +319,7 @@ struct iommu_ops {
>  	int (*sva_unbind_gpasid)(struct device *dev, int pasid);
>  
>  	unsigned long pgsize_bitmap;
> +	struct module *owner;

Everyone is always going to forget to set this field.  I don't think you
even set it for all of the different iommu_ops possible in this series,
right?

The "trick" we did to keep people from having to remember this is to do
what we did for the bus registering functions.

Look at pci_register_driver in pci.h:
#define pci_register_driver(driver)             \
        __pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)

Then we set the .owner field in the "real" __pci_register_driver() call.

Same thing for USB and lots, if not all, other driver register
functions.

You can do the same thing here, and I would recommend it.

No need to stop this series from happening now, just an add-on that is
easy to make to ensure that no one ever forgets to set this field
properly.

thanks,

greg k-h