linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management
@ 2021-09-15 13:19 Masami Hiramatsu
  2021-09-15 13:19 ` [PATCH v3 1/3] bootconfig: init: Fix memblock leak in xbc_make_cmdline() Masami Hiramatsu
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Masami Hiramatsu @ 2021-09-15 13:19 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Linus Torvalds, Mike Rapoport, Andrew Morton, LKML, Ingo Molnar,
	Masami Hiramatsu, Linux-MM, Vlastimil Babka

Hello,

Here are the series of patches to fix bootconfig memory management issues.
In this version, I removed unneeded patches and add a patch to free xbc_data
in xbc_destroy_all().

But still I'm thinking how I can move lib/bootconfig.c and bootconfig.h
to share with tools, because those functions/data are __init and __initdata.
At least I have to add a dummy macro for those in userspace.
Thus the tools/bootconfig build error is still there. I'll fix that in
another series.

Thank you,

---

Masami Hiramatsu (3):
      bootconfig: init: Fix memblock leak in xbc_make_cmdline()
      bootconfig: init: Fix memblock leak in setup_boot_config()
      bootconfig: Free xbc_data in xbc_destroy_all()


 init/main.c      |    2 ++
 lib/bootconfig.c |    3 +++
 2 files changed, 5 insertions(+)

-- 
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/3] bootconfig: init: Fix memblock leak in xbc_make_cmdline()
  2021-09-15 13:19 [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management Masami Hiramatsu
@ 2021-09-15 13:19 ` Masami Hiramatsu
  2021-09-15 13:19 ` [PATCH v3 2/3] bootconfig: init: Fix memblock leak in setup_boot_config() Masami Hiramatsu
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Masami Hiramatsu @ 2021-09-15 13:19 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Linus Torvalds, Mike Rapoport, Andrew Morton, LKML, Ingo Molnar,
	Masami Hiramatsu, Linux-MM, Vlastimil Babka

Free unused memblock in a error case to fix memblock leak
in xbc_make_cmdline().

Fixes: 51887d03aca1 ("bootconfig: init: Allow admin to use bootconfig for kernel command line")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 init/main.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/init/main.c b/init/main.c
index 3f7216934441..0b054fff8e92 100644
--- a/init/main.c
+++ b/init/main.c
@@ -382,6 +382,7 @@ static char * __init xbc_make_cmdline(const char *key)
 	ret = xbc_snprint_cmdline(new_cmdline, len + 1, root);
 	if (ret < 0 || ret > len) {
 		pr_err("Failed to print extra kernel cmdline.\n");
+		memblock_free_ptr(new_cmdline, len + 1);
 		return NULL;
 	}
 


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/3] bootconfig: init: Fix memblock leak in setup_boot_config()
  2021-09-15 13:19 [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management Masami Hiramatsu
  2021-09-15 13:19 ` [PATCH v3 1/3] bootconfig: init: Fix memblock leak in xbc_make_cmdline() Masami Hiramatsu
@ 2021-09-15 13:19 ` Masami Hiramatsu
  2021-09-15 13:19 ` [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all() Masami Hiramatsu
  2021-09-15 16:47 ` [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management Linus Torvalds
  3 siblings, 0 replies; 10+ messages in thread
From: Masami Hiramatsu @ 2021-09-15 13:19 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Linus Torvalds, Mike Rapoport, Andrew Morton, LKML, Ingo Molnar,
	Masami Hiramatsu, Linux-MM, Vlastimil Babka

Free unused memblock in a error case to fix memblock leak
in setup_boot_config().

Fixes: 7684b8582c24 ("bootconfig: Load boot config from the tail of initrd")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 init/main.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/init/main.c b/init/main.c
index 0b054fff8e92..4f059fde1df0 100644
--- a/init/main.c
+++ b/init/main.c
@@ -459,6 +459,7 @@ static void __init setup_boot_config(void)
 		else
 			pr_err("Failed to parse bootconfig: %s at %d.\n",
 				msg, pos);
+		memblock_free_ptr(copy, size + 1);
 	} else {
 		pr_info("Load bootconfig: %d bytes %d nodes\n", size, ret);
 		/* keys starting with "kernel." are passed via cmdline */


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all()
  2021-09-15 13:19 [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management Masami Hiramatsu
  2021-09-15 13:19 ` [PATCH v3 1/3] bootconfig: init: Fix memblock leak in xbc_make_cmdline() Masami Hiramatsu
  2021-09-15 13:19 ` [PATCH v3 2/3] bootconfig: init: Fix memblock leak in setup_boot_config() Masami Hiramatsu
@ 2021-09-15 13:19 ` Masami Hiramatsu
  2021-09-15 14:23   ` Steven Rostedt
  2021-09-15 16:47 ` [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management Linus Torvalds
  3 siblings, 1 reply; 10+ messages in thread
From: Masami Hiramatsu @ 2021-09-15 13:19 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Linus Torvalds, Mike Rapoport, Andrew Morton, LKML, Ingo Molnar,
	Masami Hiramatsu, Linux-MM, Vlastimil Babka

Free xbc_data memory in xbc_destroy_all() with other data.
Note that this changes the ownership of the passed bootconfig
data.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 lib/bootconfig.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 5ae248b29373..2ba7d945adc9 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -789,6 +789,7 @@ static int __init xbc_verify_tree(void)
  */
 void __init xbc_destroy_all(void)
 {
+	memblock_free_ptr(xbc_data, xbc_data_size);
 	xbc_data = NULL;
 	xbc_data_size = 0;
 	xbc_node_num = 0;
@@ -810,6 +811,8 @@ void __init xbc_destroy_all(void)
  * In error cases, @emsg will be updated with an error message and
  * @epos will be updated with the error position which is the byte offset
  * of @buf. If the error is not a parser error, @epos will be -1.
+ * Note that the @buf ownership is transferred, so it will be freed
+ * in xbc_destroy_all().
  */
 int __init xbc_init(char *buf, const char **emsg, int *epos)
 {


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all()
  2021-09-15 13:19 ` [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all() Masami Hiramatsu
@ 2021-09-15 14:23   ` Steven Rostedt
  2021-09-16  0:05     ` Masami Hiramatsu
  0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2021-09-15 14:23 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Linus Torvalds, Mike Rapoport, Andrew Morton, LKML, Ingo Molnar,
	Linux-MM, Vlastimil Babka

On Wed, 15 Sep 2021 22:19:52 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> @@ -810,6 +811,8 @@ void __init xbc_destroy_all(void)
>   * In error cases, @emsg will be updated with an error message and
>   * @epos will be updated with the error position which is the byte offset
>   * of @buf. If the error is not a parser error, @epos will be -1.
> + * Note that the @buf ownership is transferred, so it will be freed
> + * in xbc_destroy_all().
>   */
>  int __init xbc_init(char *buf, const char **emsg, int *epos)
>  {

I hate this "ownership transfer". Looking at the use case here:

init/main.c:

	copy = memblock_alloc(size + 1, SMP_CACHE_BYTES);
	if (!copy) {
		pr_err("Failed to allocate memory for bootconfig\n");
		return;
	}

	memcpy(copy, data, size);
	copy[size] = '\0';

	ret = xbc_init(copy, &msg, &pos);
	if (ret < 0) {

Instead of having xbc_init() return the node count on success, how about
having it allocate the buffer to use and then return it?

That is, move the:

	copy = memblock_alloc(size + 1, SMP_CACHE_BYTES);
	if (!copy) {
		pr_err("Failed to allocate memory for bootconfig\n");
		return;
	}

	memcpy(copy, data, size);
	copy[size] = '\0';

into xbc_init(), and have data, and size be passed to it.

Then, have it return the pointer of "copy" or NULL on error?

This will keep the semantics of xbc_* owning the buffer that gets
freed by the destroy.

The xbc_init() could also do the pr_info() that prints the bytes and
node count. There's no other reason to pass that node count to the
caller, is there?

-- Steve

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management
  2021-09-15 13:19 [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management Masami Hiramatsu
                   ` (2 preceding siblings ...)
  2021-09-15 13:19 ` [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all() Masami Hiramatsu
@ 2021-09-15 16:47 ` Linus Torvalds
  2021-09-15 16:51   ` Linus Torvalds
  3 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2021-09-15 16:47 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Steven Rostedt, Mike Rapoport, Andrew Morton, LKML, Ingo Molnar,
	Linux-MM, Vlastimil Babka

On Wed, Sep 15, 2021 at 6:19 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> But still I'm thinking how I can move lib/bootconfig.c and bootconfig.h

So having slept on it, I think the solution for now is "it's been a
problem only this once, let's ignore it for now"

IOW, I'll apply your trivial fixup for the bootconfig copy of
memblock.h, and forget about it, and then if we end up having more
problems with it later, we can look at a bigger fix.

          Linus

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management
  2021-09-15 16:47 ` [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management Linus Torvalds
@ 2021-09-15 16:51   ` Linus Torvalds
  2021-09-16  0:07     ` Masami Hiramatsu
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2021-09-15 16:51 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Steven Rostedt, Mike Rapoport, Andrew Morton, LKML, Ingo Molnar,
	Linux-MM, Vlastimil Babka

On Wed, Sep 15, 2021 at 9:47 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> IOW, I'll apply your trivial fixup for the bootconfig copy of
> memblock.h, and forget about it, and then if we end up having more
> problems with it later, we can look at a bigger fix.

Just to clarify - I applied that build fix directly to my tree, but
this series of leak fixes I'll leave for the tracing tree.

             Linus

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all()
  2021-09-15 14:23   ` Steven Rostedt
@ 2021-09-16  0:05     ` Masami Hiramatsu
  2021-09-16  0:17       ` Steven Rostedt
  0 siblings, 1 reply; 10+ messages in thread
From: Masami Hiramatsu @ 2021-09-16  0:05 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Linus Torvalds, Mike Rapoport, Andrew Morton, LKML, Ingo Molnar,
	Linux-MM, Vlastimil Babka

On Wed, 15 Sep 2021 10:23:54 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 15 Sep 2021 22:19:52 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > @@ -810,6 +811,8 @@ void __init xbc_destroy_all(void)
> >   * In error cases, @emsg will be updated with an error message and
> >   * @epos will be updated with the error position which is the byte offset
> >   * of @buf. If the error is not a parser error, @epos will be -1.
> > + * Note that the @buf ownership is transferred, so it will be freed
> > + * in xbc_destroy_all().
> >   */
> >  int __init xbc_init(char *buf, const char **emsg, int *epos)
> >  {
> 
> I hate this "ownership transfer". Looking at the use case here:
> 
> init/main.c:
> 
> 	copy = memblock_alloc(size + 1, SMP_CACHE_BYTES);
> 	if (!copy) {
> 		pr_err("Failed to allocate memory for bootconfig\n");
> 		return;
> 	}
> 
> 	memcpy(copy, data, size);
> 	copy[size] = '\0';
> 
> 	ret = xbc_init(copy, &msg, &pos);
> 	if (ret < 0) {
> 
> Instead of having xbc_init() return the node count on success, how about
> having it allocate the buffer to use and then return it?
> 
> That is, move the:
> 
> 	copy = memblock_alloc(size + 1, SMP_CACHE_BYTES);
> 	if (!copy) {
> 		pr_err("Failed to allocate memory for bootconfig\n");
> 		return;
> 	}
> 
> 	memcpy(copy, data, size);
> 	copy[size] = '\0';
> 
> into xbc_init(), and have data, and size be passed to it.
> 
> Then, have it return the pointer of "copy" or NULL on error?

Thanks for pointing it out, that is also good to me.
Let me update it.

> 
> This will keep the semantics of xbc_* owning the buffer that gets
> freed by the destroy.
> 
> The xbc_init() could also do the pr_info() that prints the bytes and
> node count. There's no other reason to pass that node count to the
> caller, is there?

Ah, it is my policy that the error or information message is shown
by caller (since caller can also ignore that, e.g. passing the
testing data), not from the library code.
I learned that from perf-probe and ftrace, sometimes the library
code reused in unexpected way. So I decided to decouple the
generating error message and showing it.

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management
  2021-09-15 16:51   ` Linus Torvalds
@ 2021-09-16  0:07     ` Masami Hiramatsu
  0 siblings, 0 replies; 10+ messages in thread
From: Masami Hiramatsu @ 2021-09-16  0:07 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Steven Rostedt, Mike Rapoport, Andrew Morton, LKML, Ingo Molnar,
	Linux-MM, Vlastimil Babka

On Wed, 15 Sep 2021 09:51:15 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Sep 15, 2021 at 9:47 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > IOW, I'll apply your trivial fixup for the bootconfig copy of
> > memblock.h, and forget about it, and then if we end up having more
> > problems with it later, we can look at a bigger fix.

Thanks, and OK, I'll try to solve this issue.

> 
> Just to clarify - I applied that build fix directly to my tree, but
> this series of leak fixes I'll leave for the tracing tree.

OK, I got it.


Thank you,

> 
>              Linus


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all()
  2021-09-16  0:05     ` Masami Hiramatsu
@ 2021-09-16  0:17       ` Steven Rostedt
  0 siblings, 0 replies; 10+ messages in thread
From: Steven Rostedt @ 2021-09-16  0:17 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Linus Torvalds, Mike Rapoport, Andrew Morton, LKML, Ingo Molnar,
	Linux-MM, Vlastimil Babka

On Thu, 16 Sep 2021 09:05:03 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Ah, it is my policy that the error or information message is shown
> by caller (since caller can also ignore that, e.g. passing the
> testing data), not from the library code.
> I learned that from perf-probe and ftrace, sometimes the library
> code reused in unexpected way. So I decided to decouple the
> generating error message and showing it.

OK, then we can just pass the number of nodes allocated via a pointer
to an integer.

Thanks!

-- Steve

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-09-16  0:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-15 13:19 [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management Masami Hiramatsu
2021-09-15 13:19 ` [PATCH v3 1/3] bootconfig: init: Fix memblock leak in xbc_make_cmdline() Masami Hiramatsu
2021-09-15 13:19 ` [PATCH v3 2/3] bootconfig: init: Fix memblock leak in setup_boot_config() Masami Hiramatsu
2021-09-15 13:19 ` [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all() Masami Hiramatsu
2021-09-15 14:23   ` Steven Rostedt
2021-09-16  0:05     ` Masami Hiramatsu
2021-09-16  0:17       ` Steven Rostedt
2021-09-15 16:47 ` [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management Linus Torvalds
2021-09-15 16:51   ` Linus Torvalds
2021-09-16  0:07     ` Masami Hiramatsu

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