Messages in this thread | | | From | Chunyan Zhang <> | Date | Mon, 6 Mar 2023 17:29:23 +0800 | Subject | Re: [PATCH V2] iommu: sprd: fix a dma buffer leak issue |
| |
On Mon, 6 Mar 2023 at 13:57, Baolu Lu <baolu.lu@linux.intel.com> wrote: > > 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():
I managed the wrong patch, sorry for the big mistake.
> > 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
Yes, it is the case for sprd iommu. I will add a comment in the next version.
Many thanks for the review, Chunyan
> 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
| |