* [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak
@ 2023-03-08 3:41 Chunyan Zhang
2023-03-08 12:37 ` Robin Murphy
2023-03-09 2:08 ` Baolu Lu
0 siblings, 2 replies; 6+ messages in thread
From: Chunyan Zhang @ 2023-03-08 3:41 UTC (permalink / raw)
To: Joerg Roedel, Will Deacon, Robin Murphy
Cc: iommu, Baolu Lu, Baolin Wang, Orson Zhai, Chunyan Zhang,
Chunyan Zhang, LKML
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.
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);
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak
2023-03-08 3:41 [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak Chunyan Zhang
@ 2023-03-08 12:37 ` Robin Murphy
2023-03-09 2:50 ` Baolu Lu
2023-03-09 2:08 ` Baolu Lu
1 sibling, 1 reply; 6+ messages in thread
From: Robin Murphy @ 2023-03-08 12:37 UTC (permalink / raw)
To: Chunyan Zhang, Joerg Roedel, Will Deacon
Cc: iommu, Baolu Lu, Baolin Wang, Orson Zhai, Chunyan Zhang, LKML
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak
2023-03-08 3:41 [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak Chunyan Zhang
2023-03-08 12:37 ` Robin Murphy
@ 2023-03-09 2:08 ` Baolu Lu
1 sibling, 0 replies; 6+ messages in thread
From: Baolu Lu @ 2023-03-09 2:08 UTC (permalink / raw)
To: Chunyan Zhang, Joerg Roedel, Will Deacon, Robin Murphy
Cc: baolu.lu, iommu, Baolin Wang, Orson Zhai, Chunyan Zhang, LKML
On 3/8/23 11:41 AM, 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.
>
> Signed-off-by: Chunyan Zhang<chunyan.zhang@unisoc.com>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Best regards,
baolu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak
2023-03-08 12:37 ` Robin Murphy
@ 2023-03-09 2:50 ` Baolu Lu
2023-03-09 11:23 ` Robin Murphy
0 siblings, 1 reply; 6+ messages in thread
From: Baolu Lu @ 2023-03-09 2:50 UTC (permalink / raw)
To: Robin Murphy, Chunyan Zhang, Joerg Roedel, Will Deacon
Cc: baolu.lu, iommu, Baolin Wang, Orson Zhai, Chunyan Zhang, LKML
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@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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak
2023-03-09 2:50 ` Baolu Lu
@ 2023-03-09 11:23 ` Robin Murphy
2023-03-10 2:11 ` Chunyan Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Robin Murphy @ 2023-03-09 11:23 UTC (permalink / raw)
To: Baolu Lu, Chunyan Zhang, Joerg Roedel, Will Deacon
Cc: iommu, Baolin Wang, Orson Zhai, Chunyan Zhang, LKML
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@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);
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak
2023-03-09 11:23 ` Robin Murphy
@ 2023-03-10 2:11 ` Chunyan Zhang
0 siblings, 0 replies; 6+ messages in thread
From: Chunyan Zhang @ 2023-03-10 2:11 UTC (permalink / raw)
To: Robin Murphy
Cc: Baolu Lu, Chunyan Zhang, Joerg Roedel, Will Deacon, iommu,
Baolin Wang, Orson Zhai, LKML
On Thu, 9 Mar 2023 at 19:23, Robin Murphy <robin.murphy@arm.com> wrote:
>
> 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
Yes, I also found this. I'm going to remove the check of dom->sdev
that will make reattching being allowed, like you mentioned below, in
the next version.
Thanks for your comments,
Chunyan
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-10 2:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-08 3:41 [PATCH V2] iommu: sprd: release dma buffer to avoid memory leak Chunyan Zhang
2023-03-08 12:37 ` Robin Murphy
2023-03-09 2:50 ` Baolu Lu
2023-03-09 11:23 ` Robin Murphy
2023-03-10 2:11 ` Chunyan Zhang
2023-03-09 2:08 ` Baolu Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox