[PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management

[PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management

[Masami Hiramatsu]

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>

[PATCH v3 1/3] bootconfig: init: Fix memblock leak in xbc_make_cmdline()

[Masami Hiramatsu]

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

[PATCH v3 2/3] bootconfig: init: Fix memblock leak in setup_boot_config()

[Masami Hiramatsu]

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 */

[PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all()

[Masami Hiramatsu]

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

Re: [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all()

[Steven Rostedt]

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

Re: [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all()

[Masami Hiramatsu]

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>

Re: [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all()

[Steven Rostedt]

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

Re: [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management

[Linus Torvalds]

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

Re: [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management

[Linus Torvalds]

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

Re: [PATCH v3 0/3] bootconfig: Fixes to bootconfig memory management

[Masami Hiramatsu]

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>