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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Mon, 6 Mar 2023 at 13:57, Baolu Lu <[email protected]> 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 <[email protected]>
> > ---
> > 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