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

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

[Chunyan Zhang]

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.
+	 */
+	if (sdev) {
+		dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
+		dom->sdev = NULL;
+	}
 
 	kfree(dom);
 }
-- 
2.25.1

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

[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

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

[Chunyan Zhang]

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