MIME-Version: 1.0
References: <20230306052613.411400-1-chunyan.zhang@unisoc.com> <d18d3026-a3a9-a92b-3735-f4045f96ff4e@linux.intel.com>
In-Reply-To: <d18d3026-a3a9-a92b-3735-f4045f96ff4e@linux.intel.com>
From:   Chunyan Zhang <zhang.lyra@gmail.com>
Date:   Mon, 6 Mar 2023 17:29:23 +0800
Message-ID: <CAAfSe-uXH1nAM4Er4UxQfVgVf24j1kFvi+fF3XRG+ucVwtS61A@mail.gmail.com>
Subject: Re: [PATCH V2] iommu: sprd: fix a dma buffer leak issue
To:     Baolu Lu <baolu.lu@linux.intel.com>
Cc:     Chunyan Zhang <chunyan.zhang@unisoc.com>,
        Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>, iommu@lists.linux.dev,
        Baolin Wang <baolin.wang@linux.alibaba.com>,
        Orson Zhai <orsonzhai@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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_allo=
c(unsigned int domain_type)
> >   static void sprd_iommu_domain_free(struct iommu_domain *domain)
> >   {
> >       struct sprd_iommu_domain *dom =3D to_sprd_domain(domain);
> > +     size_t pgt_size =3D sprd_iommu_pgt_size(domain);
> > +     struct sprd_iommu_device *sdev =3D 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 =3D dev_iommu_priv_get(dev);
> 237         struct sprd_iommu_domain *dom =3D to_sprd_domain(domain);
> 238         size_t pgt_size =3D sprd_iommu_pgt_size(domain);
> 239
> 240         if (dom->sdev)
> 241                 return -EINVAL;
> 242
> 243         dom->pgt_va =3D 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=EF=BC=8C perhap=
s put

Yes, it is the case for sprd iommu. I will add a comment in the next versio=
n.

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 =3D NULL;
> > +     }
> >
> >       kfree(dom);
> >   }
>
> Best regards,
> baolu