Re: [PATCH V2] iommu: sprd: fix a dma buffer leak issue

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

On 2023/3/6 13:26, Chunyan Zhang wrote:
> The DMA buffer used to store the address mpping table is alloced when
> attaching device to a domain, and had been released in .detach_dev()
> callback which will never be called since this driver supports default
> domain, that will cause memory leak.
> 
> Move the dma buffer free before freeing sprd iommu domain to fix
> this issue.
> 
> Signed-off-by: Chunyan Zhang <chunyan.zhang@unisoc.com>
> ---
> V2: Modified commit message
> ---
>   drivers/iommu/sprd-iommu.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
> index ae94d74b73f4..4de2e79d2226 100644
> --- a/drivers/iommu/sprd-iommu.c
> +++ b/drivers/iommu/sprd-iommu.c
> @@ -154,6 +154,17 @@ static struct iommu_domain *sprd_iommu_domain_alloc(unsigned int domain_type)
>   static void sprd_iommu_domain_free(struct iommu_domain *domain)
>   {
>   	struct sprd_iommu_domain *dom = to_sprd_domain(domain);
> +	size_t pgt_size = sprd_iommu_pgt_size(domain);
> +	struct sprd_iommu_device *sdev = dom->sdev;
> +
> +	/*
> +	 * If the domain has been attached to a device already,
> +	 * free the dma buffer first.

Is it possible that a domain is attached to multiple devices or attached
to a device multiple times? If so, perhaps the memory is also leaked in
sprd_iommu_attach_device():

233 static int sprd_iommu_attach_device(struct iommu_domain *domain,
234                                     struct device *dev)
235 {
236         struct sprd_iommu_device *sdev = dev_iommu_priv_get(dev);
237         struct sprd_iommu_domain *dom = to_sprd_domain(domain);
238         size_t pgt_size = sprd_iommu_pgt_size(domain);
239
240         if (dom->sdev)
241                 return -EINVAL;
242
243         dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, 
&dom->pgt_pa, GFP_KERNEL);
244         if (!dom->pgt_va)
245                 return -ENOMEM;

as dom->pgt_va is not checked before setting a new pointer.

If sprd iommu driver only supports one-time use of domain， perhaps put
a comment around above code so that people can take care of it when the
assumption changes.

> +	 */
> +	if (sdev) {
> +		dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
> +		dom->sdev = NULL;
> +	}
>   
>   	kfree(dom);
>   }

Best regards,
baolu