[PATCH -next] acpi/hmat: fix memory leaks in hmat_init()

[PATCH -next] acpi/hmat: fix memory leaks in hmat_init()

[Qian Cai]

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(-)

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);
-- 
2.17.2 (Apple Git-113)

Re: [PATCH -next] acpi/hmat: fix memory leaks in hmat_init()

[Rafael J. Wysocki]

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

Re: [PATCH -next] acpi/hmat: fix memory leaks in hmat_init()

[Qian Cai]

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

Re: [PATCH -next] acpi/hmat: fix memory leaks in hmat_init()

[Rafael J. Wysocki]

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.

Re: [PATCH -next] acpi/hmat: fix memory leaks in hmat_init()

[Qian Cai]

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

Re: [PATCH -next] acpi/hmat: fix memory leaks in hmat_init()

[Rafael J. Wysocki]

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.