* [PATCH] ACPI / APEI: release resources if gen_pool_add fails
@ 2019-06-14 9:49 luanshi
2019-06-14 14:51 ` James Morse
0 siblings, 1 reply; 6+ messages in thread
From: luanshi @ 2019-06-14 9:49 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown
Cc: linux-acpi, James Morse, Tony Luck, Borislav Petkov
To avoid memory leaks, destroy ghes_estatus_pool and release memory
allocated via vmalloc() on errors in ghes_estatus_pool_init().
Signed-off-by: liguang.zlg <zhangliguang@linux.alibaba.com>
---
drivers/acpi/apei/ghes.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 993940d..8472c96 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -153,6 +153,7 @@ static void ghes_unmap(void __iomem *vaddr, enum fixed_addresses fixmap_idx)
int ghes_estatus_pool_init(int num_ghes)
{
unsigned long addr, len;
+ int rc = 0;
ghes_estatus_pool = gen_pool_create(GHES_ESTATUS_POOL_MIN_ALLOC_ORDER, -1);
if (!ghes_estatus_pool)
@@ -163,8 +164,10 @@ int ghes_estatus_pool_init(int num_ghes)
ghes_estatus_pool_size_request = PAGE_ALIGN(len);
addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
- if (!addr)
+ if (!addr) {
+ gen_pool_destroy(ghes_estatus_pool);
return -ENOMEM;
+ }
/*
* New allocation must be visible in all pgd before it can be found by
@@ -172,7 +175,12 @@ int ghes_estatus_pool_init(int num_ghes)
*/
vmalloc_sync_all();
- return gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
+ rc = gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
+ if (rc) {
+ vfree(addr);
+ gen_pool_destroy(ghes_estatus_pool);
+ }
+ return rc;
}
static int map_gen_v2(struct ghes *ghes)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI / APEI: release resources if gen_pool_add fails
2019-06-14 9:49 [PATCH] ACPI / APEI: release resources if gen_pool_add fails luanshi
@ 2019-06-14 14:51 ` James Morse
2019-06-15 0:55 ` 乱石
0 siblings, 1 reply; 6+ messages in thread
From: James Morse @ 2019-06-14 14:51 UTC (permalink / raw)
To: luanshi
Cc: Rafael J. Wysocki, Len Brown, linux-acpi, Tony Luck, Borislav Petkov
Hi Liguang,
On 14/06/2019 10:49, luanshi wrote:
> To avoid memory leaks, destroy ghes_estatus_pool and release memory
> allocated via vmalloc() on errors in ghes_estatus_pool_init().
>
> Signed-off-by: liguang.zlg <zhangliguang@linux.alibaba.com>
(I'm surprised your name has a '.' in it!)
Nit: This is v2. Please add a version number in the subject, e.g.:
| [PATCH v2] ACPI / APEI: release resources if gen_pool_add fails
This makes it easy for reviewers to know which is the latest. git format-patch will do
this for you if you add '-v 2' to its command-line.
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 993940d..8472c96 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -163,8 +164,10 @@ int ghes_estatus_pool_init(int num_ghes)
>
> ghes_estatus_pool_size_request = PAGE_ALIGN(len);
> addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
> - if (!addr)
> + if (!addr) {
> + gen_pool_destroy(ghes_estatus_pool);
> return -ENOMEM;
> + }
>
> /*
> * New allocation must be visible in all pgd before it can be found by
> @@ -172,7 +175,12 @@ int ghes_estatus_pool_init(int num_ghes)
> */
> vmalloc_sync_all();
>
> - return gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
> + rc = gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
> + if (rc) {
> + vfree(addr);
addr here is unsigned long, but vfree() wants a void *.
vfree() first leaves us with a pool containing memory we've vfree()d, which doesn't feel
like a good state to step through.
Can we vfree() after gen_pool_destroy()?
> + gen_pool_destroy(ghes_estatus_pool);
> + }
> + return rc;
> }
With that:
Reviewed-by: James Morse <james.morse@arm.com>
Tested-by: James Morse <james.morse@arm.com>
Thanks for cleaning this up!
James
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI / APEI: release resources if gen_pool_add fails
2019-06-14 14:51 ` James Morse
@ 2019-06-15 0:55 ` 乱石
0 siblings, 0 replies; 6+ messages in thread
From: 乱石 @ 2019-06-15 0:55 UTC (permalink / raw)
To: James Morse
Cc: Rafael J. Wysocki, Len Brown, linux-acpi, Tony Luck, Borislav Petkov
在 2019/6/14 22:51, James Morse 写道:
> Hi Liguang,
>
> On 14/06/2019 10:49, luanshi wrote:
>> To avoid memory leaks, destroy ghes_estatus_pool and release memory
>> allocated via vmalloc() on errors in ghes_estatus_pool_init().
>>
>> Signed-off-by: liguang.zlg <zhangliguang@linux.alibaba.com>
> (I'm surprised your name has a '.' in it!)
>
> Nit: This is v2. Please add a version number in the subject, e.g.:
> | [PATCH v2] ACPI / APEI: release resources if gen_pool_add fails
>
> This makes it easy for reviewers to know which is the latest. git format-patch will do
> this for you if you add '-v 2' to its command-line.
>
>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 993940d..8472c96 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -163,8 +164,10 @@ int ghes_estatus_pool_init(int num_ghes)
>>
>> ghes_estatus_pool_size_request = PAGE_ALIGN(len);
>> addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
>> - if (!addr)
>> + if (!addr) {
>> + gen_pool_destroy(ghes_estatus_pool);
>> return -ENOMEM;
>> + }
>>
>> /*
>> * New allocation must be visible in all pgd before it can be found by
>> @@ -172,7 +175,12 @@ int ghes_estatus_pool_init(int num_ghes)
>> */
>> vmalloc_sync_all();
>>
>> - return gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
>> + rc = gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
>> + if (rc) {
>> + vfree(addr);
> addr here is unsigned long, but vfree() wants a void *.
>
> vfree() first leaves us with a pool containing memory we've vfree()d, which doesn't feel
> like a good state to step through.
> Can we vfree() after gen_pool_destroy()?
Hi James,
I will modify this patch by your suggestion and send the patch v3 for
review.
Thanks,
Liguang
>
>
>> + gen_pool_destroy(ghes_estatus_pool);
>> + }
>> + return rc;
>> }
>
> With that:
> Reviewed-by: James Morse <james.morse@arm.com>
> Tested-by: James Morse <james.morse@arm.com>
>
>
> Thanks for cleaning this up!
>
> James
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI / APEI: release resources if gen_pool_add fails
2019-06-14 9:27 ` Rafael J. Wysocki
@ 2019-06-14 9:42 ` 乱石
0 siblings, 0 replies; 6+ messages in thread
From: 乱石 @ 2019-06-14 9:42 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Len Brown, ACPI Devel Maling List,
James Morse, Tony Luck, Borislav Petkov
在 2019/6/14 17:27, Rafael J. Wysocki 写道:
> On Fri, Jun 14, 2019 at 4:20 AM luanshi <zhangliguang@linux.alibaba.com> wrote:
>> If gen_pool_add fails, then ensure that ghes_estatus_pool and vmalloc
>> resources are released to avoid memory leak.
> The patch does more than this, it also destroys ghes_estatus_pool if
> vmalloc() fails.
>
> What about using this changelog:
>
> "To avoid memory leaks, destroy ghes_estatus_pool and release memory
> allocated via vmalloc() on errors in ghes_estatus_pool_init()."
Thanks,
I will modify the changelog by your suggestion, and resend this patch.
Thanks,
Liguang
>
>> Signed-off-by: liguang.zlg <zhangliguang@linux.alibaba.com>
>> ---
>> drivers/acpi/apei/ghes.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 993940d..8472c96 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -153,6 +153,7 @@ static void ghes_unmap(void __iomem *vaddr, enum fixed_addresses fixmap_idx)
>> int ghes_estatus_pool_init(int num_ghes)
>> {
>> unsigned long addr, len;
>> + int rc = 0;
>>
>> ghes_estatus_pool = gen_pool_create(GHES_ESTATUS_POOL_MIN_ALLOC_ORDER, -1);
>> if (!ghes_estatus_pool)
>> @@ -163,8 +164,10 @@ int ghes_estatus_pool_init(int num_ghes)
>>
>> ghes_estatus_pool_size_request = PAGE_ALIGN(len);
>> addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
>> - if (!addr)
>> + if (!addr) {
>> + gen_pool_destroy(ghes_estatus_pool);
>> return -ENOMEM;
>> + }
>>
>> /*
>> * New allocation must be visible in all pgd before it can be found by
>> @@ -172,7 +175,12 @@ int ghes_estatus_pool_init(int num_ghes)
>> */
>> vmalloc_sync_all();
>>
>> - return gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
>> + rc = gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
>> + if (rc) {
>> + vfree(addr);
>> + gen_pool_destroy(ghes_estatus_pool);
>> + }
>> + return rc;
>> }
>>
>> static int map_gen_v2(struct ghes *ghes)
>> --
>> 1.8.3.1
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ACPI / APEI: release resources if gen_pool_add fails
2019-06-14 2:20 luanshi
@ 2019-06-14 9:27 ` Rafael J. Wysocki
2019-06-14 9:42 ` 乱石
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-06-14 9:27 UTC (permalink / raw)
To: luanshi
Cc: Rafael J. Wysocki, Len Brown, ACPI Devel Maling List,
James Morse, Tony Luck, Borislav Petkov
On Fri, Jun 14, 2019 at 4:20 AM luanshi <zhangliguang@linux.alibaba.com> wrote:
>
> If gen_pool_add fails, then ensure that ghes_estatus_pool and vmalloc
> resources are released to avoid memory leak.
The patch does more than this, it also destroys ghes_estatus_pool if
vmalloc() fails.
What about using this changelog:
"To avoid memory leaks, destroy ghes_estatus_pool and release memory
allocated via vmalloc() on errors in ghes_estatus_pool_init()."
> Signed-off-by: liguang.zlg <zhangliguang@linux.alibaba.com>
> ---
> drivers/acpi/apei/ghes.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 993940d..8472c96 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -153,6 +153,7 @@ static void ghes_unmap(void __iomem *vaddr, enum fixed_addresses fixmap_idx)
> int ghes_estatus_pool_init(int num_ghes)
> {
> unsigned long addr, len;
> + int rc = 0;
>
> ghes_estatus_pool = gen_pool_create(GHES_ESTATUS_POOL_MIN_ALLOC_ORDER, -1);
> if (!ghes_estatus_pool)
> @@ -163,8 +164,10 @@ int ghes_estatus_pool_init(int num_ghes)
>
> ghes_estatus_pool_size_request = PAGE_ALIGN(len);
> addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
> - if (!addr)
> + if (!addr) {
> + gen_pool_destroy(ghes_estatus_pool);
> return -ENOMEM;
> + }
>
> /*
> * New allocation must be visible in all pgd before it can be found by
> @@ -172,7 +175,12 @@ int ghes_estatus_pool_init(int num_ghes)
> */
> vmalloc_sync_all();
>
> - return gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
> + rc = gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
> + if (rc) {
> + vfree(addr);
> + gen_pool_destroy(ghes_estatus_pool);
> + }
> + return rc;
> }
>
> static int map_gen_v2(struct ghes *ghes)
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ACPI / APEI: release resources if gen_pool_add fails
@ 2019-06-14 2:20 luanshi
2019-06-14 9:27 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: luanshi @ 2019-06-14 2:20 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown
Cc: linux-acpi, James Morse, Tony Luck, Borislav Petkov
If gen_pool_add fails, then ensure that ghes_estatus_pool and vmalloc
resources are released to avoid memory leak.
Signed-off-by: liguang.zlg <zhangliguang@linux.alibaba.com>
---
drivers/acpi/apei/ghes.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 993940d..8472c96 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -153,6 +153,7 @@ static void ghes_unmap(void __iomem *vaddr, enum fixed_addresses fixmap_idx)
int ghes_estatus_pool_init(int num_ghes)
{
unsigned long addr, len;
+ int rc = 0;
ghes_estatus_pool = gen_pool_create(GHES_ESTATUS_POOL_MIN_ALLOC_ORDER, -1);
if (!ghes_estatus_pool)
@@ -163,8 +164,10 @@ int ghes_estatus_pool_init(int num_ghes)
ghes_estatus_pool_size_request = PAGE_ALIGN(len);
addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
- if (!addr)
+ if (!addr) {
+ gen_pool_destroy(ghes_estatus_pool);
return -ENOMEM;
+ }
/*
* New allocation must be visible in all pgd before it can be found by
@@ -172,7 +175,12 @@ int ghes_estatus_pool_init(int num_ghes)
*/
vmalloc_sync_all();
- return gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
+ rc = gen_pool_add(ghes_estatus_pool, addr, PAGE_ALIGN(len), -1);
+ if (rc) {
+ vfree(addr);
+ gen_pool_destroy(ghes_estatus_pool);
+ }
+ return rc;
}
static int map_gen_v2(struct ghes *ghes)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-15 0:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-14 9:49 [PATCH] ACPI / APEI: release resources if gen_pool_add fails luanshi
2019-06-14 14:51 ` James Morse
2019-06-15 0:55 ` 乱石
-- strict thread matches above, loose matches on Subject: below --
2019-06-14 2:20 luanshi
2019-06-14 9:27 ` Rafael J. Wysocki
2019-06-14 9:42 ` 乱石
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).