Messages in this thread | | | Date | Mon, 6 Mar 2023 13:57:18 +0800 | Subject | Re: [PATCH V2] iommu: sprd: fix a dma buffer leak issue | From | Baolu Lu <> |
| |
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
| |