Re: [PATCH v3 6.6] iommu: Handle race with default domain setup

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

On 29/04/2025 2:00 pm, Greg KH wrote:
> On Tue, Apr 29, 2025 at 11:47:40AM +0100, Robin Murphy wrote:
>> [ Upstream commit b46064a18810bad3aea089a79993ca5ea7a3d2b2 ]
>>
>> 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.
>>
>> [ Backport: The above is true for mainline, but here we still have
>> arch_setup_dma_ops() to worry about, which is not replayed if the
>> default domain happens to be allocated *between* that call and
>> subsequently reaching iommu_device_use_default_domain(), so we need an
>> additional earlier check to cover that case. Also we're now back before
>> the nominal commit 98ac73f99bc4 so we need to tweak the logic to depend
>> on IOMMU_DMA as well, to avoid falsely deferring on architectures not
>> using default domains. This then serves us back as far as f188056352bc,
>> where this specific form of the problem first arises. ]
>>
>> Reported-by: Charan Teja Kalla <quic_charante@quicinc.com>
>> Fixes: 98ac73f99bc4 ("iommu: Require a default_domain for all iommu drivers")
>> Fixes: f188056352bc ("iommu: Avoid locking/unlocking for iommu_probe_device()")
>> Link: https://lore.kernel.org/r/e88b94c9b575034a2c98a48b3d383654cbda7902.1740753261.git.robin.murphy@arm.com
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> Resending as a new version with a new Message-Id so as not to confuse
>> the tools... 6.12.y should simply have a straight cherry-pick of the
>> mainline commit - 98ac73f99bc4 was in 6.7 so I'm not sure why autosel
>> hasn't picked that already?
> 
> autosel is "maybe we get it", NEVER rely on it for an actual backport to
> happen.
> 
> If you want this in 6.12.y, and it applies cleanly, just ask!  But I
> can't take this 6.6.y patch before that happens for obvious reasons.

Understood; I shall try harder to remember to include explicit stable 
tags in future.

Could you please pick b46064a18810 for 6.12? I checked and there are 
indeed no conflicts :)

Thanks,
Robin.