linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation
@ 2021-06-17 20:30 Ashish Mhetre
  2021-06-17 20:30 ` [Patch V2 1/2] iommu: Fix race condition during default domain allocation Ashish Mhetre
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Ashish Mhetre @ 2021-06-17 20:30 UTC (permalink / raw)
  To: amhetre, robin.murphy, will, vdumpa; +Cc: iommu, linux-kernel, linux-arm-kernel

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Patch V2 1/2] iommu: Fix race condition during default domain allocation
  2021-06-17 20:30 [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation Ashish Mhetre
@ 2021-06-17 20:30 ` Ashish Mhetre
  2021-06-17 20:30 ` [Patch V2 2/2] iommu/arm-smmu: Fix race condition during iommu_group creation Ashish Mhetre
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Ashish Mhetre @ 2021-06-17 20:30 UTC (permalink / raw)
  To: amhetre, robin.murphy, will, vdumpa; +Cc: iommu, linux-kernel, linux-arm-kernel

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Patch V2 2/2] iommu/arm-smmu: Fix race condition during iommu_group creation
  2021-06-17 20:30 [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation Ashish Mhetre
  2021-06-17 20:30 ` [Patch V2 1/2] iommu: Fix race condition during default domain allocation Ashish Mhetre
@ 2021-06-17 20:30 ` Ashish Mhetre
  2021-07-15  4:44 ` [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation Ashish Mhetre
  2021-08-02 15:16 ` Will Deacon
  3 siblings, 0 replies; 7+ messages in thread
From: Ashish Mhetre @ 2021-06-17 20:30 UTC (permalink / raw)
  To: amhetre, robin.murphy, will, vdumpa; +Cc: iommu, linux-kernel, linux-arm-kernel

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation
  2021-06-17 20:30 [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation Ashish Mhetre
  2021-06-17 20:30 ` [Patch V2 1/2] iommu: Fix race condition during default domain allocation Ashish Mhetre
  2021-06-17 20:30 ` [Patch V2 2/2] iommu/arm-smmu: Fix race condition during iommu_group creation Ashish Mhetre
@ 2021-07-15  4:44 ` Ashish Mhetre
  2021-08-02 15:16 ` Will Deacon
  3 siblings, 0 replies; 7+ messages in thread
From: Ashish Mhetre @ 2021-07-15  4:44 UTC (permalink / raw)
  To: robin.murphy, will, vdumpa
  Cc: iommu, linux-kernel, linux-arm-kernel, linux-kernel



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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation
  2021-06-17 20:30 [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation Ashish Mhetre
                   ` (2 preceding siblings ...)
  2021-07-15  4:44 ` [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation Ashish Mhetre
@ 2021-08-02 15:16 ` Will Deacon
  2021-08-02 15:46   ` Robin Murphy
  3 siblings, 1 reply; 7+ messages in thread
From: Will Deacon @ 2021-08-02 15:16 UTC (permalink / raw)
  To: Ashish Mhetre; +Cc: robin.murphy, vdumpa, iommu, linux-kernel, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation
  2021-08-02 15:16 ` Will Deacon
@ 2021-08-02 15:46   ` Robin Murphy
  2021-08-09 14:54     ` Will Deacon
  0 siblings, 1 reply; 7+ messages in thread
From: Robin Murphy @ 2021-08-02 15:46 UTC (permalink / raw)
  To: Will Deacon, Ashish Mhetre; +Cc: vdumpa, iommu, linux-kernel, linux-arm-kernel

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation
  2021-08-02 15:46   ` Robin Murphy
@ 2021-08-09 14:54     ` Will Deacon
  0 siblings, 0 replies; 7+ messages in thread
From: Will Deacon @ 2021-08-09 14:54 UTC (permalink / raw)
  To: Robin Murphy; +Cc: Ashish Mhetre, vdumpa, iommu, linux-kernel, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-08-09 14:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-17 20:30 [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation Ashish Mhetre
2021-06-17 20:30 ` [Patch V2 1/2] iommu: Fix race condition during default domain allocation Ashish Mhetre
2021-06-17 20:30 ` [Patch V2 2/2] iommu/arm-smmu: Fix race condition during iommu_group creation Ashish Mhetre
2021-07-15  4:44 ` [Patch V2 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation Ashish Mhetre
2021-08-02 15:16 ` Will Deacon
2021-08-02 15:46   ` Robin Murphy
2021-08-09 14:54     ` Will Deacon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).