Re: [RFC PATCH 1/1] iommu: Fix one concurrency issue happens on non-iommu binding device

[Baolu Lu <baolu.lu@linux.intel.com>]

On 11/1/23 4:05 PM, Zhenhua Huang wrote:
> Thread (1)
> During SMMU probe:
> iommu_device_register
>          bus_iommu_probe
>                  probe_iommu_group for all devices in bus
>                          _probe_iommu_device
> (non-iommu binding device)	first allocate dev->iommu then free
> 
> Thread (2)
> client device probing:
> dma_configure
>          of_iommu_configure
>                  get dev->iommu->fwspec and call fwspec->ops
> 
> There may be a time window that dev->iommu allocated even for non iommu
> binding device in thread (1). It would be possible thread (2) meet Use-
> after-free errors at that window.
> 
> Fix it by closing the time window, set dev->iommu only once
> ops->probe_device successfully.
> Previous discussion details:
> https://lore.kernel.org/linux-arm-kernel/20231017163337.GE282036@ziepe.ca/T/#mee0d7bdc375541934a571ae69f43b9660f8e7312
> 
> Suggested-by: Jason Gunthorpe<jgg@ziepe.ca>
> Signed-off-by: Zhenhua Huang<quic_zhenhuah@quicinc.com>
> ---
> Hi Jason, Robin,
> Shall we first address non-iommu binding device crash with below patch? It's
> seen a lot especially when smmu probing and other driver probing at same time.
> Although it doesn't comprehensively fixes race with of_xlate VS probe, it
> indeed solves issue for non-iommu binding device.
> 
>   drivers/iommu/iommu.c | 42 +++++++++++++++++++++++++++++-------------
>   include/linux/iommu.h |  5 +----
>   2 files changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f17a111..88ce802 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -386,6 +386,17 @@ static u32 dev_iommu_get_max_pasids(struct device *dev)
>   	return min_t(u32, max_pasids, dev->iommu->iommu_dev->max_pasids);
>   }
>   
> +void dev_iommu_priv_set(struct device *dev, void *priv)
> +{
> +	struct dev_iommu *dev_iommu;
> +
> +	dev_iommu = dev_iommu_get(dev);
> +	if (WARN_ON(!dev_iommu))
> +		return;
> +
> +	dev->iommu->priv = priv;
> +}

The dev_iommu_priv_set() function is intended for iommu drivers to store
per-device iommu private data after successful device probing. Invoking
dev_iommu_get() within this helper is inappropriate as it may
inadvertently allocate dev->iommu, which is not the intended purpose of
dev_iommu_priv_set().

Best regards,
baolu