[ARM IOMMU] IOMMU framework concurrency issue

[ARM IOMMU] IOMMU framework concurrency issue

[Zhenhua Huang]

Dear experts,

Saw a few crashes in our projects because of concurrency between (1) and 
(2).

bus notifier or bus_iommu_probe:
	__iommu_probe_device
		acquire iommu_probe_device_lock only
		iommu_init_device()
			//touch dev->iommu (1)
			dev_iommu_get()
			->probe_device()

Client device probing path:
of_dma_configure			
	of_iommu_configure
		//touch dev->iommu  (2)
		...


We already have 01657bc14a39 ("iommu: Avoid races around device probe") 
and the big comment in __iommu_probe_device() refers to adopt 
device_lock() further. Notice your big effort to utilize it, and IMO it 
can address above issue(which protects dev->iommu):
https://lore.kernel.org/all/0-v2-d2762acaf50a+16d-iommu_group_locking2_jgg@nvidia.com/T/#md11b80c9e5c90ab97904f1bddecc21d97c296c0d
But from your discussion it was pointed out "which already violates 
*other* IOMMU API assumptions", because of some drivers directly called 
of_dma_configure() w/o acquiring device_lock().

Could you please provide your points of view how to address this further?

One example of crash logs, checking dump found dev->iommu already freed:
[ 0.898000][ T77] Unable to handle kernel paging request at virtual 
address 70c7e81d817765bc
…
[ 0.898219][ T77] pc : iommu_fwspec_init+0x30/0xc4
[ 0.898223][ T77] lr : of_iommu_xlate+0x58/0xe0
…
[ 0.898252][ T77] Call trace:
[ 0.898253][ T77] iommu_fwspec_init+0x30/0xc4
[ 0.898255][ T77] of_iommu_xlate+0x58/0xe0
[ 0.898257][ T77] of_iommu_configure+0x16c/0x224
[ 0.898260][ T77] of_dma_configure_id+0x1cc/0x244
[ 0.898265][ T77] platform_dma_configure+0x34/0x80
[ 0.898267][ T77] really_probe+0x110/0x384
[ 0.898271][ T77] __driver_probe_device+0xb4/0xe4
[ 0.898274][ T77] driver_probe_device+0x44/0x210
[ 0.898277][ T77] __device_attach_driver+0x144/0x170
[ 0.898280][ T77] bus_for_each_drv+0x9c/0xec
[ 0.898282][ T77] __device_attach_async_helper+0x78/0xd0
[ 0.898285][ T77] async_run_entry_fn+0x44/0x118
[ 0.898287][ T77] process_one_work+0x1e4/0x43c
[ 0.898290][ T77] worker_thread+0x25c/0x430
[ 0.898293][ T77] kthread+0x104/0x1d4
[ 0.898295][ T77] ret_from_fork+0x10/0x20

Thanks,
Zhenhua

Re: [ARM IOMMU] IOMMU framework concurrency issue

[Jason Gunthorpe]

On Tue, Oct 17, 2023 at 07:10:23PM +0800, Zhenhua Huang wrote:
> Dear experts,
> 
> Saw a few crashes in our projects because of concurrency between (1) and
> (2).
> 
> bus notifier or bus_iommu_probe:
> 	__iommu_probe_device
> 		acquire iommu_probe_device_lock only
> 		iommu_init_device()
> 			//touch dev->iommu (1)
> 			dev_iommu_get()
> 			->probe_device()
> 
> Client device probing path:
> of_dma_configure			
> 	of_iommu_configure
> 		//touch dev->iommu  (2)
> 		...
> 
> 
> We already have 01657bc14a39 ("iommu: Avoid races around device probe") and
> the big comment in __iommu_probe_device() refers to adopt device_lock()
> further. Notice your big effort to utilize it, and IMO it can address above
> issue(which protects dev->iommu):

I think something else has gone wrong here, you should not be able to
get to any really_probe() before the iommu side has done its part.

Even with proper device locking the poor device that is racing isn't
going to work properly as the IOMMU won't be guarenteed to be
consistently configured.

This seems like you need to resolve boot time ordering in your
platform? (I don't know exactly how ARM works here though)

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.

Jason

Re: [ARM IOMMU] IOMMU framework concurrency issue

[Zhenhua Huang]

Thanks Jason for your reply.

On 2023/10/18 0:33, Jason Gunthorpe wrote:
> On Tue, Oct 17, 2023 at 07:10:23PM +0800, Zhenhua Huang wrote:
>> Dear experts,
>>
>> Saw a few crashes in our projects because of concurrency between (1) and
>> (2).
>>
>> bus notifier or bus_iommu_probe:
>> 	__iommu_probe_device
>> 		acquire iommu_probe_device_lock only
>> 		iommu_init_device()
>> 			//touch dev->iommu (1)
>> 			dev_iommu_get()
>> 			->probe_device()
>>
>> Client device probing path:
>> of_dma_configure			
>> 	of_iommu_configure
>> 		//touch dev->iommu  (2)
>> 		...
>>
>>
>> We already have 01657bc14a39 ("iommu: Avoid races around device probe") and
>> the big comment in __iommu_probe_device() refers to adopt device_lock()
>> further. Notice your big effort to utilize it, and IMO it can address above
>> issue(which protects dev->iommu):
> 
> I think something else has gone wrong here, you should not be able to
> get to any really_probe() before the iommu side has done its part.

Regarding "iommu side" Are you talking about client dev->iommu set up or 
arm-smmu driver probing? should be the latter? It seems not 
guaranteed... because 1st we have an option to disable device link check 
which creates dependency, 2nd also, for example, if we call 
of_platform_populate() in ancestor node to add child node device.. 
Ancestor node doesn't know if the dependency of child not is met.

BTW, Even for non-iommu binding client device which having no iommu 
dependency at all, the issue was also reported:
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.

Thanks,
Zhenhua

> 
> Even with proper device locking the poor device that is racing isn't
> going to work properly as the IOMMU won't be guarenteed to be
> consistently configured.
> 
> This seems like you need to resolve boot time ordering in your
> platform? (I don't know exactly how ARM works here though)
> 
> 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. >
> Jason

Re: [ARM IOMMU] IOMMU framework concurrency issue

[Robin Murphy]

On 2023-10-17 17:33, Jason Gunthorpe wrote:
> On Tue, Oct 17, 2023 at 07:10:23PM +0800, Zhenhua Huang wrote:
>> Dear experts,
>>
>> Saw a few crashes in our projects because of concurrency between (1) and
>> (2).
>>
>> bus notifier or bus_iommu_probe:
>> 	__iommu_probe_device
>> 		acquire iommu_probe_device_lock only
>> 		iommu_init_device()
>> 			//touch dev->iommu (1)
>> 			dev_iommu_get()
>> 			->probe_device()
>>
>> Client device probing path:
>> of_dma_configure			
>> 	of_iommu_configure
>> 		//touch dev->iommu  (2)
>> 		...
>>
>>
>> We already have 01657bc14a39 ("iommu: Avoid races around device probe") and
>> the big comment in __iommu_probe_device() refers to adopt device_lock()
>> further. Notice your big effort to utilize it, and IMO it can address above
>> issue(which protects dev->iommu):
> 
> I think something else has gone wrong here, you should not be able to
> get to any really_probe() before the iommu side has done its part.
> 
> Even with proper device locking the poor device that is racing isn't
> going to work properly as the IOMMU won't be guarenteed to be
> consistently configured.
> 
> This seems like you need to resolve boot time ordering in your
> platform? (I don't know exactly how ARM works here though)
> 
> 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 device doesn't actually have an IOMMU (and thus free dev->iommu 
again). That still leaves a window where dev_iommu_fwspec_get() from 
elsewhere could return a pointer which becomes a UAF bomb, I guess 
b54240ad4943 didn't close it completely.

However I think my bus ops series might also happen to fix this, since 
with that we shouldn't get as far as that dev_iommu_get() unless we 
found ops which we can expect to be valid for the given device, so we 
should no longer be doing the allocate/free cycle except in the rare 
case that ->probe_device() suffers an unexpected genuine failure.

Thanks,
Robin.

Re: [ARM IOMMU] IOMMU framework concurrency issue

[Jason Gunthorpe]

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?

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);
 

Re: [ARM IOMMU] IOMMU framework concurrency issue

[Zhenhua Huang]

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);
>   

Re: [ARM IOMMU] IOMMU framework concurrency issue

[Jason Gunthorpe]

On Thu, Oct 19, 2023 at 04:21:47PM +0800, Zhenhua Huang wrote:
> 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.

I see. So there are only two locks we currently have that could
resolve that - the device_lock or the iommu_probe_device_lock

The device_lock path is what my prior series did, in your case you
already have the device_lock on of_dma_configure_id() so you just need
it on the bus path.

The iommu_probe_device_lock is what I guess Robin was thinking of with
the of_xlate rework.. I looked a little and it seems like quite a
thorny problem.. The ARM SMMU drivers seem to model the correct design
using the iommu_fwspec to pass data from of_xlate to probe, while a
whole bunch of other drivers decided to put the first half of their
probe functions into of_xlate!

To untangle this to use the iommu_probe_device_lock the of_xlate would
have to stop using the struct dev (ie so it cannot touch the
dev->iommu any more) and all the dev->iommu touches in the of/acpi
code reorganized into function arguments to iommu_probe which would
then store them into the dev->iommu under the lock. Ie stop using
dev->iommu as some temporary scratch pad to shuffle data around prior
to probing. Pass iommu_fwspec as an arg to iommu_probe and a new
ops->probe_fwspec. Stop calling dev_iommu_priv_set from of_xlate ops.

> 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?

I don't think this patch can solve the races with of_xlate vs probe,
that is just wrongly locked.

I assume my series you linked to comprehensively fixes this?

FWIW I don't have the energy to try and fix all the wonky drivers
properly so I don't plan to revisit it.

Jason

Re: [ARM IOMMU] IOMMU framework concurrency issue

[Zhenhua Huang]

On 2023/10/19 23:15, Jason Gunthorpe wrote:
> On Thu, Oct 19, 2023 at 04:21:47PM +0800, Zhenhua Huang wrote:
>> 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.
> 
> I see. So there are only two locks we currently have that could
> resolve that - the device_lock or the iommu_probe_device_lock
> 
> The device_lock path is what my prior series did, in your case you
> already have the device_lock on of_dma_configure_id() so you just need
> it on the bus path.

Exactly.

> 
> The iommu_probe_device_lock is what I guess Robin was thinking of with
> the of_xlate rework.. I looked a little and it seems like quite a
> thorny problem.. The ARM SMMU drivers seem to model the correct design
> using the iommu_fwspec to pass data from of_xlate to probe, while a
> whole bunch of other drivers decided to put the first half of their
> probe functions into of_xlate!
> 
> To untangle this to use the iommu_probe_device_lock the of_xlate would
> have to stop using the struct dev (ie so it cannot touch the
> dev->iommu any more) and all the dev->iommu touches in the of/acpi
> code reorganized into function arguments to iommu_probe which would
> then store them into the dev->iommu under the lock. Ie stop using
> dev->iommu as some temporary scratch pad to shuffle data around prior
> to probing. Pass iommu_fwspec as an arg to iommu_probe and a new
> ops->probe_fwspec. Stop calling dev_iommu_priv_set from of_xlate ops.
> 
>> 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?
> 
> I don't think this patch can solve the races with of_xlate vs probe,
> that is just wrongly locked.
> 
> I assume my series you linked to comprehensively fixes this?
> 

Yeah, I thought your previous series can comprehensively cover it. But 
it seems based on your discussion with Robin, it needs fix for all wonky 
drivers and I indeed see lots of drivers directly call of_dma_configure 
etc... Instead, Shall we firstly adopt the lockless solution?

1. It actually closed concurrency window for non-iommu device
2. for iommu device, I checked for arm smmu and haven't seen concurrency 
case as well?

But it also needs your expert view if there is any impact from other 
smmu drivers :) For our case, it looks like to be working well..

Thanks,
Zhenhua

> FWIW I don't have the energy to try and fix all the wonky drivers
> properly so I don't plan to revisit it.
> 
> Jason