Re: [ARM IOMMU] IOMMU framework concurrency issue

[Zhenhua Huang <quic_zhenhuah@quicinc.com>]

On 2023/10/19 0:19, Jason Gunthorpe wrote:
> On Wed, Oct 18, 2023 at 04:34:20PM +0100, Robin Murphy wrote:
>> On 2023-10-17 17:33, Jason Gunthorpe wrote:
>>>
>>> eg make sure the iommu driver is fully registered before allowing any
>>> concurrent probes. Once the iommu driver is registered it will be able
>>> to catch the bus notifiers and serialize things properly.
>>
>> Ugh, I think I see at least how this happens for device which *don't* have
>> an IOMMU - because iommu_init_device() has to transiently allocate
>> dev->iommu in order to call ops->probe_device in order to discover
>> that the
> 
> Hmm! Is it essential though? That ordering was C&P from before, I
> didn't study it closely when I copied it..
> 
> I only checked some drivers, but something like this looked like it
> could resolve the situation you described - Zhenhua is that your
> situation, a non-probed device?

Thanks Jason and Robin.
Typo? you mean non-iommu device? Yes, it happens also for non-iommu 
device. In separated email I listed this situation:

Client device's probing:
of_iommu_configure  ---Thread 1
     struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
     if (fwspec) {
          if (fwspec->ops)
             ...

IOMMU probing:
iommu_device_register ---Thread 2
     .. bus_iommu_probe(iommu_buses[i]);
         __iommu_probe_device
             iommu_init_device
                 dev_iommu_get
                 ... *time window of concurrency*  ------(1)
                 dev_iommu_free

In above time window (1), dev->iommu allocated but not freed, if it's 
just accessed by client device's probing(Thread 1)..     crash happens.

I also want to mention from our side, it's *not only seen for non-iommu* 
device.

Patch seems good to me and in theory can cover the case I have met. I 
tested below based on 6.6-rc1 for sanity with minor changes(clean up 
tags etc). If you're OK I want to propagate into our tree and to see if 
it fixes issue?

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3bfc56d..3a207f3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -381,6 +381,16 @@ static u32 dev_iommu_get_max_pasids(struct device *dev)
         return min_t(u32, max_pasids, dev->iommu->iommu_dev->max_pasids);
  }

+void dev_iommu_priv_set(struct device *dev, void *priv)
+{
+       struct dev_iommu *dev_iommu;
+
+       dev_iommu = dev_iommu_get(dev);
+       if (WARN_ON(!dev_iommu))
+               return; // FIXME handle failure in drivers
+       dev->iommu->priv = priv;
+}
+
  /*
   * Init the dev->iommu and dev->iommu_group in the struct device and 
get the
   * driver probed
@@ -388,16 +398,12 @@ static u32 dev_iommu_get_max_pasids(struct device 
*dev)
  static int iommu_init_device(struct device *dev, const struct 
iommu_ops *ops)
  {
         struct iommu_device *iommu_dev;
+       struct dev_iommu *dev_iommu;
         struct iommu_group *group;
         int ret;
-       if (!dev_iommu_get(dev))
-               return -ENOMEM;
-
-       if (!try_module_get(ops->owner)) {
-               ret = -EINVAL;
-               goto err_free;
-       }
+       if (!try_module_get(ops->owner))
+               return -EINVAL;

         iommu_dev = ops->probe_device(dev);
         if (IS_ERR(iommu_dev)) {
@@ -405,6 +411,14 @@ static int iommu_init_device(struct device *dev, 
const struct iommu_ops *ops)
                 goto err_module_put;
         }

+       dev_iommu = dev_iommu_get(dev);
+       if (WARN_ON(!dev_iommu)) {
+               ret = -ENOMEM;
+               goto err_release;
+       }
+
+       dev_iommu->iommu_dev = iommu_dev;
+
         ret = iommu_device_link(iommu_dev, dev);
         if (ret)
                 goto err_release;
@@ -418,10 +432,9 @@ static int iommu_init_device(struct device *dev, 
const struct iommu_ops *ops)
         }
         dev->iommu_group = group;

-       dev->iommu->iommu_dev = iommu_dev;
-       dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
+       dev_iommu->max_pasids = dev_iommu_get_max_pasids(dev);
         if (ops->is_attach_deferred)
-               dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
+               dev_iommu->attach_deferred = ops->is_attach_deferred(dev);
         return 0;

  err_unlink:
@@ -431,8 +444,11 @@ static int iommu_init_device(struct device *dev, 
const struct iommu_ops *ops)
                 ops->release_device(dev);
  err_module_put:
         module_put(ops->owner);
-err_free:
-       dev_iommu_free(dev);
+       /*
+        * If probe_device allocated a dev->iommu and things failed later
+        * we just leave it. We don't yet have robust locking, there
+        * could be concurrent users.
+        */
         return ret;
  }

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index c50a769..21c15be 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -698,10 +698,7 @@ static inline void *dev_iommu_priv_get(struct 
device *dev)
                 return NULL;
  }

-static inline void dev_iommu_priv_set(struct device *dev, void *priv)
-{
-       dev->iommu->priv = priv;
-}
+void dev_iommu_priv_set(struct device *dev, void *priv);

  int iommu_probe_device(struct device *dev);

Thanks,
Zhenhua

> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 47172f1084d8fd..580e74afdb0765 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -386,6 +386,16 @@ static u32 dev_iommu_get_max_pasids(struct device *dev)
>   	return min_t(u32, max_pasids, dev->iommu->iommu_dev->max_pasids);
>   }
>   
> +void dev_iommu_priv_set(struct device *dev, void *priv)
> +{
> +	struct dev_iommu *dev_iommu;
> +
> +	dev_iommu = dev_iommu_get(dev);
> +	if (WARN_ON(!dev_iommu))
> +		return; // FIXME handle failure in drivers
> +	dev->iommu->priv = priv;
> +}
> +
>   /*
>    * Init the dev->iommu and dev->iommu_group in the struct device and get the
>    * driver probed
> @@ -393,12 +403,10 @@ static u32 dev_iommu_get_max_pasids(struct device *dev)
>   static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
>   {
>   	struct iommu_device *iommu_dev;
> +	struct dev_iommu *dev_iommu;
>   	struct iommu_group *group;
>   	int ret;
>   
> -	if (!dev_iommu_get(dev))
> -		return -ENOMEM;
> -
>   	if (!try_module_get(ops->owner)) {
>   		ret = -EINVAL;
>   		goto err_free;
> @@ -409,7 +417,14 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
>   		ret = PTR_ERR(iommu_dev);
>   		goto err_module_put;
>   	}
> -	dev->iommu->iommu_dev = iommu_dev;
> +
> +	dev_iommu = dev_iommu_get(dev);
> +	if (WARN_ON(!dev_iommu)) {
> +		ret = -ENOMEM;
> +		goto err_release;
> +	}
> +
> +	dev_iommu->iommu_dev = iommu_dev;
>   
>   	ret = iommu_device_link(iommu_dev, dev);
>   	if (ret)
> @@ -424,9 +439,9 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
>   	}
>   	dev->iommu_group = group;
>   
> -	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
> +	dev_iommu->max_pasids = dev_iommu_get_max_pasids(dev);
>   	if (ops->is_attach_deferred)
> -		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
> +		dev_iommu->attach_deferred = ops->is_attach_deferred(dev);
>   	return 0;
>   
>   err_unlink:
> @@ -438,7 +453,11 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
>   	module_put(ops->owner);
>   err_free:
>   	dev->iommu->iommu_dev = NULL;
> -	dev_iommu_free(dev);
> +	/*
> +	 * If probe_device allocated a dev->iommu and things failed later
> +	 * we just leave it. We don't yet have robust locking, there
> +	 * could be concurrent users.
> +	 */
>   	return ret;
>   }
>   
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 68c9be9293e4c0..5c25c378a13ece 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -713,10 +713,7 @@ static inline void *dev_iommu_priv_get(struct device *dev)
>   		return NULL;
>   }
>   
> -static inline void dev_iommu_priv_set(struct device *dev, void *priv)
> -{
> -	dev->iommu->priv = priv;
> -}
> +void dev_iommu_priv_set(struct device *dev, void *priv);
>   
>   int iommu_probe_device(struct device *dev);
>