Re: [PATCH RFC] iommu: fix wrong DMA ops for iommu device

[Robin Murphy <robin.murphy@arm.com>]

On 2025-01-22 5:39 am, Charan Teja Kalla wrote:
> 
> 
> On 1/20/2025 5:45 PM, Charan Teja Kalla wrote:
>>> ----->8-----
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index d1af0547f553..8d90d196e38d 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -3120,6 +3120,11 @@ int iommu_device_use_default_domain(struct device
>>> *dev)
>>>           return 0;
>>>   
>>>       mutex_lock(&group->mutex);
>>> +    /* We may race against bus_iommu_probe() finalising groups here */
>>> +    if (!group->default_domain) {
>>> +        ret = -EPROBE_DEFER;
>>> +        goto unlock_out;
>>> +    }
>> Please lmk If I can submit this as V2 patch, with your name as Suggested-by?
> I missed to mention that this patch is fixing the problem we are
> seeing..Thanks.

Ah, that's good. I did spend most of an afternoon digging through git
history to write up this patch the other day, and was planning to send
it properly soon (probably after the merge window now) - as noted, it
also turns out not to be quite right for 6.6LTS as-is, so I figured I'd
save worrying about backports until it lands.

Thanks,
Robin.

----->8-----
Subject: [PATCH] iommu: Handle race with default domain setup

It turns out that deferred default domain creation leaves a subtle
race window during iommu_device_register() wherein a client driver may
asynchronously probe in parallel and get as far as performing DMA API
operations with dma-direct, only to be switched to iommu-dma underfoot
once the default domain attachment finally happens, with obviously
disastrous consequences. Even the wonky of_iommu_configure() path is at
risk, since iommu_fwspec_init() will no longer defer client probe as the
instance ops are (necessarily) already registered, and the "replay"
iommu_probe_device() call can see dev->iommu_group already set and so
think there's nothing to do either.

Fortunately we already have the right tool in the right place in the
form of iommu_device_use_default_domain(), which just needs to ensure
that said default domain is actually ready to *be* used. Deferring the
client probe shouldn't have too much impact, given that this only
happens while the IOMMU driver is probing, and thus due to kick the
deferred probe list again once it finishes.

Reported-by: Charan Teja Kalla <quic_charante@quicinc.com>
Fixes: 98ac73f99bc4 ("iommu: Require a default_domain for all iommu drivers")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>

---

Note this fixes tag is rather nuanced - historically there was a more
general issue before deac0b3bed26 ("iommu: Split off default domain
allocation from group assignment") set the basis for the current
conditions; 1ea2a07a532b ("iommu: Add DMA ownership management
interfaces") is then the point at which it becomes logical to fix the
current race this way; however only from 98ac73f99bc4 can we rely on all
drivers supporting default domains and so avoid false negatives, thus
even though this might apply to older kernels without conflict it would
not be functionally correct. Furthermore given the other locking and API
flow changes which also happened over that time, it's not clear how long
this race has actually been exposed. LTS fixes are going to be fiddly...
---
  drivers/iommu/iommu.c | 5 +++++
  1 file changed, 5 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 851fd5aeccf5..9969531ad4ed 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3097,6 +3097,11 @@ int iommu_device_use_default_domain(struct device *dev)
  		return 0;
  
  	mutex_lock(&group->mutex);
+	/* We may race against bus_iommu_probe() finalising groups here */
+	if (!group->default_domain) {
+		ret = -EPROBE_DEFER;
+		goto unlock_out;
+	}
  	if (group->owner_cnt) {
  		if (group->domain != group->default_domain || group->owner ||
  		    !xa_empty(&group->pasid_array)) {
-- 
2.39.2.101.g768bb238c484.dirty