[Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation

[Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation

[Ashish Mhetre]

Multiple iommu domains and iommu groups are getting created for the devices
sharing same SID. It is expected for devices sharing same SID to be in same
iommu group and same iommu domain.
This is leading to context faults when one device is accessing IOVA from
other device which shouldn't be the case for devices sharing same SID.
Fix this by protecting iommu domain and iommu group creation with mutexes.

Ashish Mhetre (1):
  iommu: Fix race condition during default domain allocation

Krishna Reddy (1):
  iommu/arm-smmu: Fix race condition during iommu_group creation

 drivers/iommu/arm/arm-smmu/arm-smmu.c | 6 +++++-
 drivers/iommu/iommu.c                 | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.7.4

[Patch V2 1/2] iommu: Fix race condition during default domain allocation

[Ashish Mhetre]

Domain is getting created more than once during asynchronous multiple
display heads(devices) probe. All the display heads share same SID and
are expected to be in same domain. As iommu_alloc_default_domain() call
is not protected, it ends up in creating two domains for two display
devices which should ideally be in same domain.
iommu_alloc_default_domain() checks whether domain is already allocated for
given iommu group, but due to this race the check condition is failing and
two different domains are getting created.
This is leading to context faults when one device is accessing the IOVA
mapped by other device.
Fix this by protecting iommu_alloc_default_domain() call with group->mutex.
With this fix serialization will happen only for the devices sharing same
group. Also, only first device in group will hold the mutex till group is
created and for rest of the devices it will just check for existing domain
and then release the mutex.

Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
---
Changes since V1:
- Update the commit message per Will's suggestion

 drivers/iommu/iommu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 808ab70..2700500 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -273,7 +273,9 @@ int iommu_probe_device(struct device *dev)
 	 * support default domains, so the return value is not yet
 	 * checked.
 	 */
+	mutex_lock(&group->mutex);
 	iommu_alloc_default_domain(group, dev);
+	mutex_unlock(&group->mutex);
 
 	if (group->default_domain) {
 		ret = __iommu_attach_device(group->default_domain, dev);
-- 
2.7.4

[Patch V2 2/2] iommu/arm-smmu: Fix race condition during iommu_group creation

[Ashish Mhetre]

From: Krishna Reddy <vdumpa@nvidia.com>

iommu_group is getting created more than once during asynchronous multiple
display heads(devices) probe on Tegra194 SoC. All the display heads share
same SID and are expected to be in same iommu_group.
As arm_smmu_device_group() is not protecting group creation across devices,
it is leading to multiple groups creation across devices with same SID and
subsequent IOMMU faults.
During race, the iommu_probe_device() call for two display devices is
ending up in arm_smmu_device_group() twice and hence two groups are getting
created. Ideally after group creation for first display device, same group
should be used by second display device.
This race is leading to context faults when one display device is accessing
IOVA from other display device which shouldn't be the case for devices
sharing same SID.
Fix this by protecting group creation with smmu->stream_map_mutex.

Signed-off-by: Krishna Reddy <vdumpa@nvidia.com>
---
Changes since V1:
- Update the commit message per Will's suggestion

 drivers/iommu/arm/arm-smmu/arm-smmu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 6f72c4d..21af179 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1458,6 +1458,7 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
 	struct iommu_group *group = NULL;
 	int i, idx;
 
+	mutex_lock(&smmu->stream_map_mutex);
 	for_each_cfg_sme(cfg, fwspec, i, idx) {
 		if (group && smmu->s2crs[idx].group &&
 		    group != smmu->s2crs[idx].group)
@@ -1466,8 +1467,10 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
 		group = smmu->s2crs[idx].group;
 	}
 
-	if (group)
+	if (group) {
+		mutex_unlock(&smmu->stream_map_mutex);
 		return iommu_group_ref_get(group);
+	}
 
 	if (dev_is_pci(dev))
 		group = pci_device_group(dev);
@@ -1481,6 +1484,7 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
 		for_each_cfg_sme(cfg, fwspec, i, idx)
 			smmu->s2crs[idx].group = group;
 
+	mutex_unlock(&smmu->stream_map_mutex);
 	return group;
 }
 
-- 
2.7.4

Re: [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation

[Ashish Mhetre]

On 6/18/2021 2:00 AM, Ashish Mhetre wrote:
> Multiple iommu domains and iommu groups are getting created for the devices
> sharing same SID. It is expected for devices sharing same SID to be in same
> iommu group and same iommu domain.
> This is leading to context faults when one device is accessing IOVA from
> other device which shouldn't be the case for devices sharing same SID.
> Fix this by protecting iommu domain and iommu group creation with mutexes.
> 
> Ashish Mhetre (1):
>    iommu: Fix race condition during default domain allocation
> 
> Krishna Reddy (1):
>    iommu/arm-smmu: Fix race condition during iommu_group creation
> 
>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 6 +++++-
>   drivers/iommu/iommu.c                 | 2 ++
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 

Hi,

Can you please help in reviewing this V2 change? Please let me know if 
anymore changes are required.

Thanks,
Ashish Mhetre

Re: [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation

[Will Deacon]

On Fri, Jun 18, 2021 at 02:00:35AM +0530, Ashish Mhetre wrote:
> Multiple iommu domains and iommu groups are getting created for the devices
> sharing same SID. It is expected for devices sharing same SID to be in same
> iommu group and same iommu domain.
> This is leading to context faults when one device is accessing IOVA from
> other device which shouldn't be the case for devices sharing same SID.
> Fix this by protecting iommu domain and iommu group creation with mutexes.

Robin -- any chance you could take a look at these, please? You had some
comments on the first version which convinced me that they are needed,
but I couldn't tell whether you wanted to solve this a different way or not.

Cheers,

Will

Re: [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation

[Robin Murphy]

On 2021-08-02 16:16, Will Deacon wrote:
> On Fri, Jun 18, 2021 at 02:00:35AM +0530, Ashish Mhetre wrote:
>> Multiple iommu domains and iommu groups are getting created for the devices
>> sharing same SID. It is expected for devices sharing same SID to be in same
>> iommu group and same iommu domain.
>> This is leading to context faults when one device is accessing IOVA from
>> other device which shouldn't be the case for devices sharing same SID.
>> Fix this by protecting iommu domain and iommu group creation with mutexes.
> 
> Robin -- any chance you could take a look at these, please? You had some
> comments on the first version which convinced me that they are needed,
> but I couldn't tell whether you wanted to solve this a different way or not.

Sorry, I was lamenting that this came to light due to the 
of_iommu_configure() flow being yucky, but that wasn't meant to imply 
that there aren't - or couldn't be in future - better reasons for 
iommu_probe_device() to be robust against concurrency anyway. I do think 
these are legitimate fixes to make in their own right, even if the 
current need might get swept back under the rug in future.

I would say, however, that the commit messages seem to focus too much on 
the wrong details and aren't overly useful, and patch #2 is missing 
Ashish's sign-off.

Thanks,
Robin.

Re: [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation

[Will Deacon]

On Mon, Aug 02, 2021 at 04:46:37PM +0100, Robin Murphy wrote:
> On 2021-08-02 16:16, Will Deacon wrote:
> > On Fri, Jun 18, 2021 at 02:00:35AM +0530, Ashish Mhetre wrote:
> > > Multiple iommu domains and iommu groups are getting created for the devices
> > > sharing same SID. It is expected for devices sharing same SID to be in same
> > > iommu group and same iommu domain.
> > > This is leading to context faults when one device is accessing IOVA from
> > > other device which shouldn't be the case for devices sharing same SID.
> > > Fix this by protecting iommu domain and iommu group creation with mutexes.
> > 
> > Robin -- any chance you could take a look at these, please? You had some
> > comments on the first version which convinced me that they are needed,
> > but I couldn't tell whether you wanted to solve this a different way or not.
> 
> Sorry, I was lamenting that this came to light due to the
> of_iommu_configure() flow being yucky, but that wasn't meant to imply that
> there aren't - or couldn't be in future - better reasons for
> iommu_probe_device() to be robust against concurrency anyway. I do think
> these are legitimate fixes to make in their own right, even if the current
> need might get swept back under the rug in future.
> 
> I would say, however, that the commit messages seem to focus too much on the
> wrong details and aren't overly useful, and patch #2 is missing Ashish's
> sign-off.

Ashish -- please can you send a v3 fixing these issues?

Will