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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <05fb17c7-397b-5b33-fbfa-df3bf48b0de8@linux.intel.com>
Date:   Thu, 9 Mar 2023 10:50:31 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     Robin Murphy <robin.murphy@....com>,
        Chunyan Zhang <chunyan.zhang@...soc.com>,
        Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>
Cc:     baolu.lu@...ux.intel.com, iommu@...ts.linux.dev,
        Baolin Wang <baolin.wang@...ux.alibaba.com>,
        Orson Zhai <orsonzhai@...il.com>,
        Chunyan Zhang <zhang.lyra@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak

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);
?

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@...soc.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);
>