| Message ID | 20190406181704.33574-1-cai@lca.pw (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [-next] acpi/hmat: fix memory leaks in hmat_init() | expand |
On Sat, Apr 6, 2019 at 8:18 PM Qian Cai <cai@lca.pw> wrote: > > The commit 665ac7e92757 ("acpi/hmat: Register processor domain to its > memory") introduced some memory leaks below due to it fails to release > the heap memory in an error path, and then the stack __initdata memory > which reference them get freed during boot renders those heap memory as > leaks. > > unreferenced object 0xc8ff8008349e9400 (size 128): > comm "swapper/0", pid 1, jiffies 4294709236 (age 48121.476s) > hex dump (first 32 bytes): > 00 d0 9e 34 08 80 ff 84 d8 00 43 11 00 10 ff ff ...4......C..... > 00 00 00 00 ff ff ff ff 00 00 00 00 00 00 00 00 ................ > backtrace: > [<00000000869d4503>] __kmalloc+0x568/0x600 > [<0000000070fd6afb>] alloc_memory_target+0x50/0xd8 > [<00000000efa2081e>] srat_parse_mem_affinity+0x58/0x5c > [<000000008bfaef74>] acpi_parse_entries_array+0x1c8/0x2c0 > [<0000000022804877>] acpi_table_parse_entries_array+0x11c/0x138 > [<00000000ffe9cd34>] acpi_table_parse_entries+0x7c/0xac > [<00000000a7023afd>] hmat_init+0x90/0x174 > [<00000000694a86c1>] do_one_initcall+0x2d8/0x5f8 > [<0000000024889da9>] do_initcall_level+0x37c/0x3fc > [<000000009be02908>] do_basic_setup+0x38/0x50 > [<0000000037b3ac0a>] kernel_init_freeable+0x194/0x258 > [<00000000f5741184>] kernel_init+0x18/0x334 > [<000000007b30f423>] ret_from_fork+0x10/0x18 > [<000000006c7147a8>] 0xffffffffffffffff > > Signed-off-by: Qian Cai <cai@lca.pw> Well, the catch is good, but the additional label is sort of excessive IMO. > --- > drivers/acpi/hmat/hmat.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/hmat/hmat.c b/drivers/acpi/hmat/hmat.c > index b7824a0309f7..c9b8abcf012c 100644 > --- a/drivers/acpi/hmat/hmat.c > +++ b/drivers/acpi/hmat/hmat.c > @@ -637,7 +637,7 @@ static __init int hmat_init(void) > > status = acpi_get_table(ACPI_SIG_HMAT, 0, &tbl); > if (ACPI_FAILURE(status)) I would just add a hmat_free_structures() call here or rework the _put_table() logic which is not really straightforward. > - return 0; > + goto out_free; > > hmat_revision = tbl->revision; > switch (hmat_revision) { > @@ -659,8 +659,9 @@ static __init int hmat_init(void) > } > hmat_register_targets(); > out_put: > - hmat_free_structures(); > acpi_put_table(tbl); > +out_free: > + hmat_free_structures(); > return 0; > } > subsys_initcall(hmat_init); > --
On Tue, 2019-04-09 at 10:25 +0200, Rafael J. Wysocki wrote: > On Sat, Apr 6, 2019 at 8:18 PM Qian Cai <cai@lca.pw> wrote: > > > > The commit 665ac7e92757 ("acpi/hmat: Register processor domain to its > > memory") introduced some memory leaks below due to it fails to release > > the heap memory in an error path, and then the stack __initdata memory > > which reference them get freed during boot renders those heap memory as > > leaks. > > > > unreferenced object 0xc8ff8008349e9400 (size 128): > > comm "swapper/0", pid 1, jiffies 4294709236 (age 48121.476s) > > hex dump (first 32 bytes): > > 00 d0 9e 34 08 80 ff 84 d8 00 43 11 00 10 ff ff ...4......C..... > > 00 00 00 00 ff ff ff ff 00 00 00 00 00 00 00 00 ................ > > backtrace: > > [<00000000869d4503>] __kmalloc+0x568/0x600 > > [<0000000070fd6afb>] alloc_memory_target+0x50/0xd8 > > [<00000000efa2081e>] srat_parse_mem_affinity+0x58/0x5c > > [<000000008bfaef74>] acpi_parse_entries_array+0x1c8/0x2c0 > > [<0000000022804877>] acpi_table_parse_entries_array+0x11c/0x138 > > [<00000000ffe9cd34>] acpi_table_parse_entries+0x7c/0xac > > [<00000000a7023afd>] hmat_init+0x90/0x174 > > [<00000000694a86c1>] do_one_initcall+0x2d8/0x5f8 > > [<0000000024889da9>] do_initcall_level+0x37c/0x3fc > > [<000000009be02908>] do_basic_setup+0x38/0x50 > > [<0000000037b3ac0a>] kernel_init_freeable+0x194/0x258 > > [<00000000f5741184>] kernel_init+0x18/0x334 > > [<000000007b30f423>] ret_from_fork+0x10/0x18 > > [<000000006c7147a8>] 0xffffffffffffffff > > > > Signed-off-by: Qian Cai <cai@lca.pw> > > Well, the catch is good, but the additional label is sort of excessive IMO. > > > --- > > drivers/acpi/hmat/hmat.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/hmat/hmat.c b/drivers/acpi/hmat/hmat.c > > index b7824a0309f7..c9b8abcf012c 100644 > > --- a/drivers/acpi/hmat/hmat.c > > +++ b/drivers/acpi/hmat/hmat.c > > @@ -637,7 +637,7 @@ static __init int hmat_init(void) > > > > status = acpi_get_table(ACPI_SIG_HMAT, 0, &tbl); > > if (ACPI_FAILURE(status)) > > I would just add a hmat_free_structures() call here or rework the > _put_table() logic which is not really straightforward. I am not sure how adding a hmat_free_structures() call there would be better. If the name has bee changed to something else in the future, it ends up needing to change 2 places. It also not save the adding line-count either. I don't see how to rework the acpi_put_table() logic to be better than adding a label here either. > > > - return 0; > > + goto out_free; > > > > hmat_revision = tbl->revision; > > switch (hmat_revision) { > > @@ -659,8 +659,9 @@ static __init int hmat_init(void) > > } > > hmat_register_targets(); > > out_put: > > - hmat_free_structures(); > > acpi_put_table(tbl); > > +out_free: > > + hmat_free_structures(); > > return 0; > > } > > subsys_initcall(hmat_init); > > --
On Tue, Apr 9, 2019 at 3:25 PM Qian Cai <cai@lca.pw> wrote: > > On Tue, 2019-04-09 at 10:25 +0200, Rafael J. Wysocki wrote: > > On Sat, Apr 6, 2019 at 8:18 PM Qian Cai <cai@lca.pw> wrote: > > > > > > The commit 665ac7e92757 ("acpi/hmat: Register processor domain to its > > > memory") introduced some memory leaks below due to it fails to release > > > the heap memory in an error path, and then the stack __initdata memory > > > which reference them get freed during boot renders those heap memory as > > > leaks. > > > > > > unreferenced object 0xc8ff8008349e9400 (size 128): > > > comm "swapper/0", pid 1, jiffies 4294709236 (age 48121.476s) > > > hex dump (first 32 bytes): > > > 00 d0 9e 34 08 80 ff 84 d8 00 43 11 00 10 ff ff ...4......C..... > > > 00 00 00 00 ff ff ff ff 00 00 00 00 00 00 00 00 ................ > > > backtrace: > > > [<00000000869d4503>] __kmalloc+0x568/0x600 > > > [<0000000070fd6afb>] alloc_memory_target+0x50/0xd8 > > > [<00000000efa2081e>] srat_parse_mem_affinity+0x58/0x5c > > > [<000000008bfaef74>] acpi_parse_entries_array+0x1c8/0x2c0 > > > [<0000000022804877>] acpi_table_parse_entries_array+0x11c/0x138 > > > [<00000000ffe9cd34>] acpi_table_parse_entries+0x7c/0xac > > > [<00000000a7023afd>] hmat_init+0x90/0x174 > > > [<00000000694a86c1>] do_one_initcall+0x2d8/0x5f8 > > > [<0000000024889da9>] do_initcall_level+0x37c/0x3fc > > > [<000000009be02908>] do_basic_setup+0x38/0x50 > > > [<0000000037b3ac0a>] kernel_init_freeable+0x194/0x258 > > > [<00000000f5741184>] kernel_init+0x18/0x334 > > > [<000000007b30f423>] ret_from_fork+0x10/0x18 > > > [<000000006c7147a8>] 0xffffffffffffffff > > > > > > Signed-off-by: Qian Cai <cai@lca.pw> > > > > Well, the catch is good, but the additional label is sort of excessive IMO. > > > > > --- > > > drivers/acpi/hmat/hmat.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/acpi/hmat/hmat.c b/drivers/acpi/hmat/hmat.c > > > index b7824a0309f7..c9b8abcf012c 100644 > > > --- a/drivers/acpi/hmat/hmat.c > > > +++ b/drivers/acpi/hmat/hmat.c > > > @@ -637,7 +637,7 @@ static __init int hmat_init(void) > > > > > > status = acpi_get_table(ACPI_SIG_HMAT, 0, &tbl); > > > if (ACPI_FAILURE(status)) > > > > I would just add a hmat_free_structures() call here or rework the > > _put_table() logic which is not really straightforward. > > I am not sure how adding a hmat_free_structures() call there would be better. If > the name has bee changed to something else in the future, it ends up needing to > change 2 places. It also not save the adding line-count either. I don't see how > to rework the acpi_put_table() logic to be better than adding a label here > either. Fewer jumps are easier to follow in general, so avoiding ones that can be avoided is helpful. I'm not buying the argument about more code line changes needed if the function name changes. It's meaningless. And if you check the return value of acpi_get_table() for SRAT after calling acpi_put_table(tbl), you will only need the out_free label, if I'm not mistaken.
On Tue, 2019-04-09 at 16:54 +0200, Rafael J. Wysocki wrote: > Fewer jumps are easier to follow in general, so avoiding ones that can > be avoided is helpful. > > I'm not buying the argument about more code line changes needed if the > function name changes. It's meaningless. > > And if you check the return value of acpi_get_table() for SRAT after > calling acpi_put_table(tbl), you will only need the out_free label, if > I'm not mistaken. I don't really understand this. status = acpi_get_table(ACPI_SIG_SRAT acpi_put_table(tbl); status = acpi_get_table(ACPI_SIG_HMAT If acpi_get_table(ACPI_SIG_SRAT failed, there is no point calling acpi_put_table(), so what is the point checking return value of acpi_get_table() for SRAT after acpi_put_table() ?
On Tue, Apr 9, 2019 at 5:33 PM Qian Cai <cai@lca.pw> wrote: > > On Tue, 2019-04-09 at 16:54 +0200, Rafael J. Wysocki wrote: > > Fewer jumps are easier to follow in general, so avoiding ones that can > > be avoided is helpful. > > > > I'm not buying the argument about more code line changes needed if the > > function name changes. It's meaningless. > > > > And if you check the return value of acpi_get_table() for SRAT after > > calling acpi_put_table(tbl), you will only need the out_free label, if > > I'm not mistaken. > > I don't really understand this. Sorry, I didn't have the original code in front of me when I was writing my reply. I was thinking about checking the return value of acpi_table_parse_entries() after calling acpi_put_table(tbl). Then, if it returns an error, you only need to call hmat_free_structures(). Similarly, if acpi_get_table(ACPI_SIG_HMAT, 0, &tbl) returns an error, you only need to call hmat_free_structures(). But I didn't remember the other jumps to out_put. Still, since it is valid to pass NULL to acpi_put_table(), you can jump to out_put if acpi_get_table(ACPI_SIG_HMAT, 0, &tbl) returns an error too.
diff --git a/drivers/acpi/hmat/hmat.c b/drivers/acpi/hmat/hmat.c index b7824a0309f7..c9b8abcf012c 100644 --- a/drivers/acpi/hmat/hmat.c +++ b/drivers/acpi/hmat/hmat.c @@ -637,7 +637,7 @@ static __init int hmat_init(void) status = acpi_get_table(ACPI_SIG_HMAT, 0, &tbl); if (ACPI_FAILURE(status)) - return 0; + goto out_free; hmat_revision = tbl->revision; switch (hmat_revision) { @@ -659,8 +659,9 @@ static __init int hmat_init(void) } hmat_register_targets(); out_put: - hmat_free_structures(); acpi_put_table(tbl); +out_free: + hmat_free_structures(); return 0; } subsys_initcall(hmat_init);
The commit 665ac7e92757 ("acpi/hmat: Register processor domain to its memory") introduced some memory leaks below due to it fails to release the heap memory in an error path, and then the stack __initdata memory which reference them get freed during boot renders those heap memory as leaks. unreferenced object 0xc8ff8008349e9400 (size 128): comm "swapper/0", pid 1, jiffies 4294709236 (age 48121.476s) hex dump (first 32 bytes): 00 d0 9e 34 08 80 ff 84 d8 00 43 11 00 10 ff ff ...4......C..... 00 00 00 00 ff ff ff ff 00 00 00 00 00 00 00 00 ................ backtrace: [<00000000869d4503>] __kmalloc+0x568/0x600 [<0000000070fd6afb>] alloc_memory_target+0x50/0xd8 [<00000000efa2081e>] srat_parse_mem_affinity+0x58/0x5c [<000000008bfaef74>] acpi_parse_entries_array+0x1c8/0x2c0 [<0000000022804877>] acpi_table_parse_entries_array+0x11c/0x138 [<00000000ffe9cd34>] acpi_table_parse_entries+0x7c/0xac [<00000000a7023afd>] hmat_init+0x90/0x174 [<00000000694a86c1>] do_one_initcall+0x2d8/0x5f8 [<0000000024889da9>] do_initcall_level+0x37c/0x3fc [<000000009be02908>] do_basic_setup+0x38/0x50 [<0000000037b3ac0a>] kernel_init_freeable+0x194/0x258 [<00000000f5741184>] kernel_init+0x18/0x334 [<000000007b30f423>] ret_from_fork+0x10/0x18 [<000000006c7147a8>] 0xffffffffffffffff Signed-off-by: Qian Cai <cai@lca.pw> --- drivers/acpi/hmat/hmat.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)