linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ashish Mhetre <amhetre@nvidia.com>
To: Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>, <vdumpa@nvidia.com>
Cc: <iommu@lists.linux-foundation.org>,
	<linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 1/2] iommu: Fix race condition during default domain allocation
Date: Thu, 17 Jun 2021 11:21:39 +0530	[thread overview]
Message-ID: <315fe1c5-2685-6ee3-2aa4-35a27233127b@nvidia.com> (raw)
In-Reply-To: <faf4504c-43f2-f68e-9a00-5e450dd7f352@arm.com>



On 6/11/2021 6:19 PM, Robin Murphy wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2021-06-11 11:45, Will Deacon wrote:
>> On Thu, Jun 10, 2021 at 09:46:53AM +0530, Ashish Mhetre wrote:
>>> 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, the group->default_domain and group->domain are ending
>>> up with different domains and leading to subsequent IOMMU faults.
>>> Fix this by protecting iommu_alloc_default_domain() call with 
>>> group->mutex.
>>
>> Can you provide some more information about exactly what the h/w
>> configuration is, and the callstack which exhibits the race, please?
> 
> It'll be basically the same as the issue reported long ago with PCI
> groups in the absence of ACS not being constructed correctly. Triggering
> the iommu_probe_device() replay in of_iommu_configure() off the back of
> driver probe is way too late and allows calls to happen in the wrong
> order, or indeed race in parallel as here. Fixing that is still on my
> radar, but will not be simple, and will probably go hand-in-hand with
> phasing out the bus ops (for the multiple-driver-coexistence problem).
> 
For iommu group creation, the stack flow during race is like:
Display device 1:
iommu_probe_device -> iommu_group_get_for_dev -> arm_smmu_device_group
Display device 2:
iommu_probe_device -> iommu_group_get_for_dev -> arm_smmu_device_group

And this way it ends up in creating 2 groups for 2 display devices 
sharing same SID.
Ideally for 2nd display device, iommu_group_get call from 
iommu_group_get_for_dev should return same group as 1st display device. 
But due to the race, it ends up with 2 groups.

For default domain, the stack flow during race is like:
Display device 1:
iommu_probe_device -> iommu_alloc_default_domain -> arm_smmu_domain_alloc
Display device 2:
iommu_probe_device -> iommu_alloc_default_domain -> arm_smmu_domain_alloc

Here also 2nd device should already have domain allocated and 
'if(group->default_domain)' condition from iommu_alloc_default_domain 
should be true for 2nd device.

Issue with this is IOVA accesses from 2nd device results in context faults.

>>> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
>>> ---
>>>   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);
>>
>> It feels wrong to serialise this for everybody just to cater for systems
>> with aliasing SIDs between devices.
> 
> If two or more devices are racing at this point then they're already
> going to be serialised by at least iommu_group_add_device(), so I doubt
> there would be much impact - only the first device through here will
> hold the mutex for any appreciable length of time. Every other path
> which modifies group->domain does so with the mutex held (note the
> "expected" default domain allocation flow in bus_iommu_probe() in
> particular), so not holding it here does seem like a straightforward
> oversight.
> 
> Robin.
Serialization will only happen for the devices sharing same group. Only 
the first device in group will hold this till domain is created. For 
rest of the devices it will just check for existing domain in 
iommu_alloc_default_domain and then return and release the mutex.



  reply	other threads:[~2021-06-17  5:52 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-10  4:16 [PATCH 0/2] iommu/arm-smmu: Fix races in iommu domain/group creation Ashish Mhetre
2021-06-10  4:16 ` [PATCH 1/2] iommu: Fix race condition during default domain allocation Ashish Mhetre
2021-06-11 10:45   ` Will Deacon
2021-06-11 12:49     ` Robin Murphy
2021-06-17  5:51       ` Ashish Mhetre [this message]
2021-06-17 17:49         ` Will Deacon
2021-06-11 18:30     ` Krishna Reddy
2021-06-10  4:16 ` [PATCH 2/2] iommu/arm-smmu: Fix race condition during iommu_group creation Ashish Mhetre

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=315fe1c5-2685-6ee3-2aa4-35a27233127b@nvidia.com \
    --to=amhetre@nvidia.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=vdumpa@nvidia.com \
    --cc=will@kernel.org \
    --subject='Re: [PATCH 1/2] iommu: Fix race condition during default domain allocation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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