Re: [PATCH RFC] iommu: fix wrong DMA ops for iommu device
From: Charan Teja Kalla
Date: Fri Jan 10 2025 - 08:30:13 EST
Thanks Will and Robin for the response!!
On 1/8/2025 6:07 PM, Robin Murphy wrote:
> On 07/01/2025 11:31 am, Will Deacon wrote:
>> On Mon, Jan 06, 2025 at 06:46:08PM +0530, Charan Teja Kalla wrote:
>>> On 1/3/2025 9:04 PM, Will Deacon wrote:
>>>> On Thu, Jan 02, 2025 at 04:09:46PM +0530, Charan Teja Kalla wrote:
>>>>> On 12/13/2024 8:34 PM, Charan Teja Kalla wrote:
>>>>>> Race between smmu device probe and iommu device probe is resulting
>>>>>> into
>>>>>> wrong DMA ops used for the device.
>>>>>>
>>>>>> bus_iommu_probe(P1) of_iommu_configure(P2)
>>>>>> (from iommu_device_register) (from insmod of a driver)
>>>>>> -------------------------- --------------------------
>>>>>> 1) probe_iommu_group()
>>>>>> (Fills just dev->iommu_group
>>>>>> under iommu_probe_device_lock)
>>>>>> 2) iommu_probe_device():
>>>>>> (acquires the iommu_probe_device_lock,
>>>>>> but since dev->iommu_group is already
>>>>>> set, it just returns without
>>>>>> allocating the domain.)
>>>>>>
>>>>>> 3) of_dma_configure_id()->
>>>>>> arch_setup_dma_ops() gets called for
>>>>>> this device, but returns with the
>>>>>> error message:
>>>>>> "Failed to set up IOMMU for device;
>>>>>> retaining platform DMA ops"
>>>>>>
>>>>>> 4) device probe is succeeded where it
>>>>>> can now setup iommu mappings using
>>>>>> wrong ->dma_ops
>>>>>>
>>>>>> 5) domain will be allocated later
>>>>>> and fills iommu_group->domain.
>>>>>>
>>>>>> Step 3) and 4) denies iommu device from using 'iommu_dma_ops'.
>>>>>>
>>>>>> This issue does exists on 6.6 because of commit f188056352bc ("iommu:
>>>>>> Avoid locking/unlocking for iommu_probe_device()") without which 2)
>>>>>> returns only after filling the domain under group->mutex, thus no
>>>>>> issue.
>>>>>>
>>>>>> With commit b5c58b2fdc42("dma-mapping: direct calls for dma-iommu"),
>>>>>> ->iommu_dma_ops is removed altogether and iommu api are directly
>>>>>> being
>>>>>> called under "use_dma_iommu(): return dev->dma_iommu". Again,
>>>>>> ->dma_iommu flag is set only after domain allocation. So, If
>>>>>> there is a sufficient delay between steps 4) and 5), issue is still
>>>>>> exists but that seems very narrow to me.
>>>>>>
>>>>>> I am thinking, setup of the domain before returning from step2) is
>>>>>> the
>>>>>> way to fix this issue. Can you please help if you have any
>>>>>> suggestions?
>>>>>>
>>>>>> Signed-off-by: Charan Teja Kalla <quic_charante@xxxxxxxxxxx>
>>>>>> ---
>>>>>> drivers/iommu/iommu.c | 26 +++++++++++++++++++++++++-
>>>>>> 1 file changed, 25 insertions(+), 1 deletion(-)
>>>>>
>>>>> Just a quick ask if you have any comments for the below. BTW, in
>>>>> internal tests, when we revert the commit f188056352bc ("iommu: Avoid
>>>>> locking/unlocking for iommu_probe_device()", it is helping.
>>>>>
>>>>> I can share more details If the below is not sufficient.
>>>>>
>>>>> Thanks in advance...
>>>>
>>>> Can you reproduce the issue on 6.13-rc5? We bodged some of the probe
>>>> code recently:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
>>>> commit/?id=229e6ee43d2a160a1592b83aad620d6027084aad
>>>>
>>>> and a better set of fixes are queued in -next.
>>>
>>> Yes, we did try a better patch from Robin[1] and the issue is still
>>> reproducible.
>>
>> Did you try the -EPROBE_DEFER hack that got merged?
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
>> commit/?id=229e6ee43d2a160a1592b83aad620d6027084aad
>>
@Will,
We just tested and the issue is still reported with just the mentioned
hack that got merged.
May be I will try to convince again with more details about why this is
a race and why the -EPROBE_DEFER hack you mentioned will not help.
S: SMMU device probe in bus_iommu_probe().
C: SMMU client device probe in of_iommu_configure().
1) S: In probe_iommu_group():
Fills the client device ->iommu_group under iommu_probe_device_lock.
But iommu_group->domain is not yet allocated because it is a 2 step
process from probe_iommu_group.
2) C: iommu_probe_device() --> __iommu_probe_device():
Acquires the iommu_probe_device_lock, and since dev->iommu_group is
already set, it just returns without allocating the domain.
static int __iommu_probe_device(struct device *dev, ...)
{
/* Device is probed already if in a group */
if (dev->iommu_group)
return 0; --> __driver call returns from here.__
ret = iommu_init_device(dev, ops);
if (ret)
return ret;
}
The change you mentioned will come into play iff it enters
iommu_init_device(), where it returns -EPROBE_DEFER till smmu driver
bound happens. __Here we didn't even enter into this function__.
3) C: Continues probing and calls
of_dma_configure_id()->arch_setup_dma_ops()->iommu_setup_dma_ops().
Since ->domain is not yet allocated, it just returns without setting
'iommu_dma_ops' and with a warn message of 'Failed to set up IOMMU for
device' for this client device. And also the client probe is
successfully completed.
void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit) {
if (!domain)
goto out_err;
if (iommu_is_dma_domain(domain)) {
.....
dev->dma_ops = &iommu_dma_ops;<-- This step will not be executed.
}
out_err:
pr_warn("Failed to set up IOMMU for device %s; retaining
platform DMA ops\n",
dev_name(dev));
}
4) S: domain will be allocated later in the second step and fills
iommu_group->domain.
Since, the SMMU client device probe is already successfully completed
with invalid dma_ops and when client uses the dma api can lead to issues.
>> Otherwise, you can try the stack queued for 6.14 in iommu/linux.git:
>>
>> 46b3df8eb9bd iommu: Manage driver probe deferral better
>> fcbd62156742 iommu/arm-smmu-v3: Clean up more on probe failure
>> 97cb1fa02726 iommu/arm-smmu: Retire probe deferral workaround
>> 7d835134d4e1 iommu/arm-smmu: Make instance lookup robust
>>
>>> IIUC, these patches are trying to get valid smmu device, during the
>>> probe of an smmu client device, by traversing proper 'klist_devices'
>>> happening from iommu_init_device(). Please CMIW.
>>
>> Well, they're fixing a race between the IOMMU probing and its client
>> devices attaching.
>>
>>> But this bug makes iommu_init_device() to completely gets skipped.
>>>
>>> static int __iommu_probe_device(struct device *dev, ...)
>>> {
>>> /* Device is probed already if in a group */
>>> if (dev->iommu_group)
>>> return 0; --> driver call returns from here.
>>>
>>> ret = iommu_init_device(dev, ops);
>>> if (ret)
>>> return ret;
>>>
>>>
>>> }
>>>
>>> Regarding the testing, we work on Android that is on LTS kernels, so, it
>>> may not be possible for us to try the thing on 6.13 and this seems a bug
>>> on LTS 6.6 kernel.
>>
>> We need to fix 6.13 before we fix 6.6 unless you can show that 6.13 is
>> unaffected (in which case, we can backport the fix(es)).
>
> Certainly the reasoning can't apply to mainline as given, since
> arch_setup_dma_ops() stopped touching IOMMU stuff at all back in 6.10
> (and indeed iommu_dma_ops itself no longer exists since 6.12).
>
@Robin
Agree that we don't have iommu_dma_ops but I can say that same race
still exists. Although dma_ops is not used, but the decision to call
into the iommu api's is determined by the 'dev->dma_iommu' flag, which
again, is set after domain is allocated for a device.
In the same race mentioned above,
1) S: Domain is not allocated but the dev->iommu_group.
2) C: Just returns as dev->iommu_group is filled.
3) C: Continues probing and succeeds.
4) C: Calls dma_alloc/map/.... But, it won't enter into iommu_ api's
because the 'dev->dma_iommu' is still 'false'.
5) S: Domain is allocated and sets the 'dev->dma_iommu' to 'true'.
4) above is the problematic step. Although issue exists but seems to me
that very narrow to get triggered. Please CMIW.
> Robin.
Thanks.