Message-ID: <d18d3026-a3a9-a92b-3735-f4045f96ff4e@linux.intel.com>
Date:   Mon, 6 Mar 2023 13:57:18 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Cc:     baolu.lu@linux.intel.com, iommu@lists.linux.dev,
        Baolin Wang <baolin.wang@linux.alibaba.com>,
        Orson Zhai <orsonzhai@gmail.com>,
        Chunyan Zhang <zhang.lyra@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2] iommu: sprd: fix a dma buffer leak issue
To:     Chunyan Zhang <chunyan.zhang@unisoc.com>,
        Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>
References: <20230306052613.411400-1-chunyan.zhang@unisoc.com>
Content-Language: en-US
From:   Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <20230306052613.411400-1-chunyan.zhang@unisoc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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