lkml.org 
[lkml]   [2023]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak
From
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. 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@unisoc.com>
> ---
> 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);

\
 
 \ /
  Last update: 2023-03-27 00:49    [W:0.060 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site