Re: [PATCH] iommu: Avoid more races around device probe

[André Draszik]

Hi Robin,

On Wed, 2023-11-15 at 18:25 +0000, Robin Murphy wrote:
> It turns out there are more subtle races beyond just the main part of
> __iommu_probe_device() itself running in parallel - the
> dev_iommu_free()
> on the way out of an unsuccessful probe can still manage to trip up
> concurrent accesses to a device's fwspec. Thus, extend the scope of
> iommu_probe_device_lock() to also serialise fwspec creation and
> initial
> retrieval.
> 
> Reported-by: Zhenhua Huang <quic_zhenhuah@xxxxxxxxxxx>
> Link:
> https://lore.kernel.org/linux-iommu/e2e20e1c-6450-4ac5-9804-b0000acdf7de@xxxxxxxxxxx/
> Fixes: 01657bc14a39 ("iommu: Avoid races around device probe")
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>

Reviewed-by: André Draszik <andre.draszik@xxxxxxxxxx>
Tested-by: André Draszik <andre.draszik@xxxxxxxxxx> (using continuous
boot test loop)

I like that this is easily back-portable to 6.1, thanks for this patch
:-)

Cheers,
André


> ---
> 
> This is my idea of a viable fix, since it does not need a 700-line
> diffstat to make the code do what it was already *trying* to do
> anyway.
> This stuff should fundamentally not be hanging off driver probe in
> the
> first place, so I'd rather get on with removing the underlying
> brokenness than waste time and effort polishing it any further.
> 
>  drivers/acpi/scan.c      |  7 ++++++-
>  drivers/iommu/iommu.c    | 20 ++++++++++----------
>  drivers/iommu/of_iommu.c | 12 +++++++++---
>  include/linux/iommu.h    |  1 +
>  4 files changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index fa5dd71a80fa..02bb2cce423f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1568,17 +1568,22 @@ static const struct iommu_ops
> *acpi_iommu_configure_id(struct device *dev,
>  	int err;
>  	const struct iommu_ops *ops;
>  
> +	/* Serialise to make dev->iommu stable under our potential
> fwspec */
> +	mutex_lock(&iommu_probe_device_lock);
>  	/*
>  	 * If we already translated the fwspec there is nothing left
> to do,
>  	 * return the iommu_ops.
>  	 */
>  	ops = acpi_iommu_fwspec_ops(dev);
> -	if (ops)
> +	if (ops) {
> +		mutex_unlock(&iommu_probe_device_lock);
>  		return ops;
> +	}
>  
>  	err = iort_iommu_configure_id(dev, id_in);
>  	if (err && err != -EPROBE_DEFER)
>  		err = viot_iommu_configure(dev);
> +	mutex_unlock(&iommu_probe_device_lock);
>  
>  	/*
>  	 * If we have reason to believe the IOMMU driver missed the
> initial
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f17a1113f3d6..e0c962648dde 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -485,11 +485,12 @@ static void iommu_deinit_device(struct device
> *dev)
>  	dev_iommu_free(dev);
>  }
>  
> +DEFINE_MUTEX(iommu_probe_device_lock);
> +
>  static int __iommu_probe_device(struct device *dev, struct list_head
> *group_list)
>  {
>  	const struct iommu_ops *ops = dev->bus->iommu_ops;
>  	struct iommu_group *group;
> -	static DEFINE_MUTEX(iommu_probe_device_lock);
>  	struct group_device *gdev;
>  	int ret;
>  
> @@ -502,17 +503,15 @@ static int __iommu_probe_device(struct device
> *dev, struct list_head *group_list
>  	 * probably be able to use device_lock() here to minimise
> the scope,
>  	 * but for now enforcing a simple global ordering is fine.
>  	 */
> -	mutex_lock(&iommu_probe_device_lock);
> +	lockdep_assert_held(&iommu_probe_device_lock);
>  
>  	/* Device is probed already if in a group */
> -	if (dev->iommu_group) {
> -		ret = 0;
> -		goto out_unlock;
> -	}
> +	if (dev->iommu_group)
> +		return 0;
>  
>  	ret = iommu_init_device(dev, ops);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
>  	group = dev->iommu_group;
>  	gdev = iommu_group_alloc_device(group, dev);
> @@ -548,7 +547,6 @@ static int __iommu_probe_device(struct device
> *dev, struct list_head *group_list
>  			list_add_tail(&group->entry, group_list);
>  	}
>  	mutex_unlock(&group->mutex);
> -	mutex_unlock(&iommu_probe_device_lock);
>  
>  	if (dev_is_pci(dev))
>  		iommu_dma_set_pci_32bit_workaround(dev);
> @@ -562,8 +560,6 @@ static int __iommu_probe_device(struct device
> *dev, struct list_head *group_list
>  	iommu_deinit_device(dev);
>  	mutex_unlock(&group->mutex);
>  	iommu_group_put(group);
> -out_unlock:
> -	mutex_unlock(&iommu_probe_device_lock);
>  
>  	return ret;
>  }
> @@ -573,7 +569,9 @@ int iommu_probe_device(struct device *dev)
>  	const struct iommu_ops *ops;
>  	int ret;
>  
> +	mutex_lock(&iommu_probe_device_lock);
>  	ret = __iommu_probe_device(dev, NULL);
> +	mutex_unlock(&iommu_probe_device_lock);
>  	if (ret)
>  		return ret;
>  
> @@ -1822,7 +1820,9 @@ static int probe_iommu_group(struct device
> *dev, void *data)
>  	struct list_head *group_list = data;
>  	int ret;
>  
> +	mutex_lock(&iommu_probe_device_lock);
>  	ret = __iommu_probe_device(dev, group_list);
> +	mutex_unlock(&iommu_probe_device_lock);
>  	if (ret == -ENODEV)
>  		ret = 0;
>  
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 157b286e36bf..c25b4ae6aeee 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -112,16 +112,20 @@ const struct iommu_ops
> *of_iommu_configure(struct device *dev,
>  					   const u32 *id)
>  {
>  	const struct iommu_ops *ops = NULL;
> -	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> +	struct iommu_fwspec *fwspec;
>  	int err = NO_IOMMU;
>  
>  	if (!master_np)
>  		return NULL;
>  
> +	/* Serialise to make dev->iommu stable under our potential
> fwspec */
> +	mutex_lock(&iommu_probe_device_lock);
> +	fwspec = dev_iommu_fwspec_get(dev);
>  	if (fwspec) {
> -		if (fwspec->ops)
> +		if (fwspec->ops) {
> +			mutex_unlock(&iommu_probe_device_lock);
>  			return fwspec->ops;
> -
> +		}
>  		/* In the deferred case, start again from scratch */
>  		iommu_fwspec_free(dev);
>  	}
> @@ -155,6 +159,8 @@ const struct iommu_ops *of_iommu_configure(struct
> device *dev,
>  		fwspec = dev_iommu_fwspec_get(dev);
>  		ops    = fwspec->ops;
>  	}
> +	mutex_unlock(&iommu_probe_device_lock);
> +
>  	/*
>  	 * If we have reason to believe the IOMMU driver missed the
> initial
>  	 * probe for dev, replay it to get things in order.
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ec289c1016f5..6291aa7b079b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -845,6 +845,7 @@ static inline void dev_iommu_priv_set(struct
> device *dev, void *priv)
>  	dev->iommu->priv = priv;
>  }
>  
> +extern struct mutex iommu_probe_device_lock;
>  int iommu_probe_device(struct device *dev);
>  
>  int iommu_dev_enable_feature(struct device *dev, enum
> iommu_dev_features f);