Re: [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak

[Robin Murphy]

On 2023-03-09 02:50, Baolu Lu wrote:
> On 3/8/23 8:37 PM, Robin Murphy wrote:
> > On 2023-03-08 03:41, Chunyan Zhang wrote:
> > > Release page table DMA buffer when the IOMMU domain is not used:
> > >
> > > - Domain freed.
> > >
> > > - IOMMU is attaching to a new domain.
> > >    Since one sprd IOMMU servers only one client device, if the IOMMU has
> > >    been attached to other domain, it has to be detached first, that's
> > >    saying the DMA buffer should be released, otherwise that would
> > >    cause memory leak issue.
> >
> > This is clearly wrong; domain resources should only be freed when the 
> > domain is freed. 
>
> Agreed. Perhaps, in the attach path:
>
>      if (!dom->pgt_va)
>          dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size,
>                  &dom->pgt_pa, GFP_KERNEL);
> ?

Why? dom->pgt_va will always be NULL if dom->sdev is NULL, and once 
dom->sdev is set then that allocation is not reachable (of course this 
means the driver also has a separate bug where reattaching to a 
previously-used domain will erroneously fail, but that's not a memory 
leak as such). The only thing to recognise here is that the allocation 
is logically not part of the .attach_dev operation, but really a one-off 
deferred part of .domain_alloc, and thus it should be balanced in .free, 
not anywhere else.

Thanks,
Robin.

> Best regards,
> baolu
>
> > Just because a caller has detached from a domain doesn't mean that 
> > they can't reattach to it later and expect the previous mappings to 
> > still be in place - it has nothing to do with how many devices or 
> > domains can be active at once.
> >
> > Thanks,
> > Robin.
> >
> > > Signed-off-by: Chunyan Zhang <chunyan.zhang@xxxxxxxxxx>
> > > ---
> > > V2:
> > > * Added some comment in sprd_iommu_attach_device() for the reason
> > >    of calling sprd_iommu_cleanup().
> > >
> > > V1: https://lkml.org/lkml/2023/2/10/198
> > > ---
> > >   drivers/iommu/sprd-iommu.c | 46 ++++++++++++++++++++++++++++++++------
> > >   1 file changed, 39 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
> > > index ae94d74b73f4..fb2f96df3bca 100644
> > > --- a/drivers/iommu/sprd-iommu.c
> > > +++ b/drivers/iommu/sprd-iommu.c
> > > @@ -62,6 +62,7 @@ enum sprd_iommu_version {
> > >    * @eb: gate clock which controls IOMMU access
> > >    */
> > >   struct sprd_iommu_device {
> > > +    struct sprd_iommu_domain    *dom;
> > >       enum sprd_iommu_version    ver;
> > >       u32            *prot_page_va;
> > >       dma_addr_t        prot_page_pa;
> > > @@ -151,13 +152,6 @@ static struct iommu_domain 
> > > *sprd_iommu_domain_alloc(unsigned int domain_type)
> > >       return &dom->domain;
> > >   }
> > > -static void sprd_iommu_domain_free(struct iommu_domain *domain)
> > > -{
> > > -    struct sprd_iommu_domain *dom = to_sprd_domain(domain);
> > > -
> > > -    kfree(dom);
> > > -}
> > > -
> > >   static void sprd_iommu_first_vpn(struct sprd_iommu_domain *dom)
> > >   {
> > >       struct sprd_iommu_device *sdev = dom->sdev;
> > > @@ -230,6 +224,29 @@ static void sprd_iommu_hw_en(struct 
> > > sprd_iommu_device *sdev, bool en)
> > >       sprd_iommu_update_bits(sdev, reg_cfg, mask, 0, val);
> > >   }
> > > +static void sprd_iommu_cleanup(struct sprd_iommu_device *sdev)
> > > +{
> > > +    struct sprd_iommu_domain *dom = sdev->dom;
> > > +    size_t pgt_size = sprd_iommu_pgt_size(&dom->domain);
> > > +
> > > +    dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
> > > +    dom->sdev = NULL;
> > > +    sdev->dom = NULL;
> > > +    sprd_iommu_hw_en(sdev, false);
> > > +}
> > > +
> > > +static void sprd_iommu_domain_free(struct iommu_domain *domain)
> > > +{
> > > +    struct sprd_iommu_domain *dom = to_sprd_domain(domain);
> > > +    struct sprd_iommu_device *sdev = dom->sdev;
> > > +
> > > +    /* Free DMA buffer first if the domain has been attached */
> > > +    if (sdev)
> > > +        sprd_iommu_cleanup(sdev);
> > > +
> > > +    kfree(dom);
> > > +}
> > > +
> > >   static int sprd_iommu_attach_device(struct iommu_domain *domain,
> > >                       struct device *dev)
> > >   {
> > > @@ -237,14 +254,29 @@ static int sprd_iommu_attach_device(struct 
> > > iommu_domain *domain,
> > >       struct sprd_iommu_domain *dom = to_sprd_domain(domain);
> > >       size_t pgt_size = sprd_iommu_pgt_size(domain);
> > > +    /* Return directly if the domain attached to IOMMU already */
> > >       if (dom->sdev)
> > >           return -EINVAL;
> > > +    /* The IOMMU already attached to a domain */
> > > +    if (sdev->dom) {
> > > +        if (sdev->dom == dom)
> > > +            return 0;
> > > +
> > > +        /*
> > > +         * Clean up the previous domain, one sprd IOMMU servers only
> > > +         * one client device, if the IOMMU has been attached to other
> > > +         * domain, it has to be detached first.
> > > +         */
> > > +        sprd_iommu_cleanup(sdev);
> > > +    }
> > > +
> > >       dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, 
> > > &dom->pgt_pa, GFP_KERNEL);
> > >       if (!dom->pgt_va)
> > >           return -ENOMEM;
> > >       dom->sdev = sdev;
> > > +    sdev->dom = dom;
> > >       sprd_iommu_first_ppn(dom);
> > >       sprd_iommu_first_vpn(dom);