lkml.org 
[lkml]   [2023]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V2] iommu: sprd: fix a dma buffer leak issue
From
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

\
 
 \ /
  Last update: 2023-03-27 00:44    [W:0.057 / U:0.504 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site