Patch "iommu/arm-smmu: Defer probe of clients after smmu device bound" has been added to the 6.6-stable tree

DISCUSSION Robin Murphy 2 participants 3 messages Dec 10, 2024 1 message not in the archive

Subsystems (3)

Stable branch IOMMU Arm SMMU drivers

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Robin Murphy

author reviewer

2024-12-10 12:14 UTC #1 lore raw

Quoted 7 lines · Sasha Levin

This is a note to let you know that I've just added the patch titled
iommu/arm-smmu: Defer probe of clients after smmu device bound
to the 6.6-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

Quoted 6 lines

The filename of the patch is:
iommu-arm-smmu-defer-probe-of-clients-after-smmu-dev.patch
and it can be found in the queue-6.6 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

FWIW the correct resolution for cherry-picking this directly is the logically-straightforward one, as below (git is mostly just confused by the context)

Cheers,
Robin.

----->8-----

drivers/iommu/arm/arm-smmu/arm-smmu.c +0 -0

diff --cc drivers/iommu/arm/arm-smmu/arm-smmu.c
index d6d1a2a55cc0,14618772a3d6..000000000000
@@@ -1357,10 -1435,19 +1357,21 @@@ static struct iommu_device *arm_smmu_pr
  		fwspec = dev_iommu_fwspec_get(dev);
  		if (ret)
  			goto out_free;
 -	} else {
 +	} else if (fwspec && fwspec->ops == &arm_smmu_ops) {
  		smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
+
+ 		/*
+ 		 * Defer probe if the relevant SMMU instance hasn't finished
+ 		 * probing yet. This is a fragile hack and we'd ideally
+ 		 * avoid this race in the core code. Until that's ironed
+ 		 * out, however, this is the most pragmatic option on the
+ 		 * table.
+ 		 */
+ 		if (!smmu)
+ 			return ERR_PTR(dev_err_probe(dev, -EPROBE_DEFER,
+ 						"smmu dev has not bound yet\n"));
 +	} else {
 +		return ERR_PTR(-ENODEV);
  	}
  
  	ret = -EINVAL;

Quoted 82 lines

commit 62dc845a353efab2254480df8ae7d06175627313
Author: Pratyush Brahma <[email protected]>
Date:   Fri Oct 4 14:34:28 2024 +0530
iommu/arm-smmu: Defer probe of clients after smmu device bound
[ Upstream commit 229e6ee43d2a160a1592b83aad620d6027084aad ]
Null pointer dereference occurs due to a race between smmu driver probe and client driver probe, when of_dma_configure() for client is called after the iommu_device_register() for smmu driver probe has executed but before the driver_bound() for smmu driver has been called.
Following is how the race occurs:
T1:Smmu device probe T2: Client device probe
really_probe()
arm_smmu_device_probe()
iommu_device_register()
really_probe() platform_dma_configure() of_dma_configure() of_dma_configure_id() of_iommu_configure() iommu_probe_device() iommu_init_device() arm_smmu_probe_device() arm_smmu_get_by_fwnode()
driver_find_device_by_fwnode() driver_find_device() next_device() klist_next()
/* null ptr
assigned to smmu */
/* null ptr dereference
while smmu->streamid_mask */
driver_bound()
klist_add_tail()
When this null smmu pointer is dereferenced later in arm_smmu_probe_device, the device crashes.
Fix this by deferring the probe of the client device until the smmu device has bound to the arm smmu driver.
Fixes 021bb8420d44 ("iommu/arm-smmu: Wire up generic configuration support")
Cc [email protected]
Co-developed-by Prakash Gupta <[email protected]>
Signed-off-by Prakash Gupta <[email protected]>
Signed-off-by Pratyush Brahma <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
[will: Add comment]
Signed-off-by Will Deacon <[email protected]>
Signed-off-by Sasha Levin <[email protected]>
drivers/iommu/arm/arm-smmu/arm-smmu.c +11 -0
index 8203a06014d71..b40ffa1ec2db6 100644
@@ -1354,6 +1354,17 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
  			goto out_free;
  	} else {
  		smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
+
+		/*
+		 * Defer probe if the relevant SMMU instance hasn't finished
+		 * probing yet. This is a fragile hack and we'd ideally
+		 * avoid this race in the core code. Until that's ironed
+		 * out, however, this is the most pragmatic option on the
+		 * table.
+		 */
+		if (!smmu)
+			return ERR_PTR(dev_err_probe(dev, -EPROBE_DEFER,
+						"smmu dev has not bound yet\n"));
  	}
 
  	ret = -EINVAL;

Greg KH

maintainer

2024-12-10 12:19 UTC #2 lore raw

Quoted 39 lines · Robin Murphy

Quoted 7 lines · Sasha Levin
This is a note to let you know that I've just added the patch titled
iommu/arm-smmu: Defer probe of clients after smmu device bound
to the 6.6-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
Quoted 6 lines
The filename of the patch is:
iommu-arm-smmu-defer-probe-of-clients-after-smmu-dev.patch
and it can be found in the queue-6.6 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
FWIW the correct resolution for cherry-picking this directly is the logically-straightforward one, as below (git is mostly just confused by the context)
Cheers,
Robin.
----->8-----
drivers/iommu/arm/arm-smmu/arm-smmu.c +5 -0
diff --cc drivers/iommu/arm/arm-smmu/arm-smmu.c
index d6d1a2a55cc0,14618772a3d6..000000000000
@@@ -1357,10 -1435,19 +1357,21 @@@ static struct iommu_device *arm_smmu_pr
  		fwspec = dev_iommu_fwspec_get(dev);
  		if (ret)
  			goto out_free;
 -	} else {
 +	} else if (fwspec && fwspec->ops == &arm_smmu_ops) {
  		smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
+
+ 		/*
+ 		 * Defer probe if the relevant SMMU instance hasn't finished
+ 		 * probing yet. This is a fragile hack and we'd ideally
+ 		 * avoid this race in the core code. Until that's ironed

drivers/iommu/arm/arm-smmu/arm-smmu.c

Quoted 10 lines

drivers/iommu/arm/arm-smmu/arm-smmu.c +6 -0

+ 		 * out, however, this is the most pragmatic option on the
+ 		 * table.
+ 		 */
+ 		if (!smmu)
+ 			return ERR_PTR(dev_err_probe(dev, -EPROBE_DEFER,
+ 						"smmu dev has not bound yet\n"));
 +	} else {
 +		return ERR_PTR(-ENODEV);
  	}
  	ret = -EINVAL;a

Can you resend this in a patch that we can apply as-is?

thanks,

greg k-h

Robin Murphy

author reviewer

2024-12-10 12:44 UTC #3 lore raw

Quoted 44 lines · Greg KH

Quoted 43 lines · Robin Murphy
Quoted 7 lines · Sasha Levin
This is a note to let you know that I've just added the patch titled
iommu/arm-smmu: Defer probe of clients after smmu device bound
to the 6.6-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
Quoted 6 lines
The filename of the patch is:
iommu-arm-smmu-defer-probe-of-clients-after-smmu-dev.patch
and it can be found in the queue-6.6 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
FWIW the correct resolution for cherry-picking this directly is the logically-straightforward one, as below (git is mostly just confused by the context)
Cheers,
Robin.
----->8-----
drivers/iommu/arm/arm-smmu/arm-smmu.c +9 -0
diff --cc drivers/iommu/arm/arm-smmu/arm-smmu.c
index d6d1a2a55cc0,14618772a3d6..000000000000
@@@ -1357,10 -1435,19 +1357,21 @@@ static struct iommu_device *arm_smmu_pr
   		fwspec = dev_iommu_fwspec_get(dev);
   		if (ret)
   			goto out_free;
  -	} else {
  +	} else if (fwspec && fwspec->ops == &arm_smmu_ops) {
   		smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
+
+ 		/*
+ 		 * Defer probe if the relevant SMMU instance hasn't finished
+ 		 * probing yet. This is a fragile hack and we'd ideally
+ 		 * avoid this race in the core code. Until that's ironed
+ 		 * out, however, this is the most pragmatic option on the
+ 		 * table.
+ 		 */
+ 		if (!smmu)

Quoted 8 lines

drivers/iommu/arm/arm-smmu/arm-smmu.c
Quoted 6 lines
drivers/iommu/arm/arm-smmu/arm-smmu.c +2 -0
+ 			return ERR_PTR(dev_err_probe(dev, -EPROBE_DEFER,
+ 						"smmu dev has not bound yet\n"));
  +	} else {
  +		return ERR_PTR(-ENODEV);
   	}
   	ret = -EINVAL;a
Can you resend this in a patch that we can apply as-is?

Hope I got the tagging right :)

https://lore.kernel.org/stable/acd8068372673fb881aa9e13531470669c985519.1733834302.git.robin.murphy@arm.com/

Cheers,
Robin.

Quoted 4 lines

thanks,
greg k-h