[PATCH v2 0/5] bootconfig: Fixes to bootconfig memory management

[PATCH v2 0/5] bootconfig: Fixes to bootconfig memory management

[Masami Hiramatsu]

Hello,

Here are the series of patches to fix bootconfig memory management issues.
From v1, I added some more fixes and split a memory leak fix from freeing
unused memory patch.

[1/5] fixing kernel BUG issue on boot. Just rebased on the linus tree.
[2/5] Add new memblock leak fix in xbc_make_cmdline().
[3/5] Split from [4/5], fixing a memblock leak in setup_boot_config().
[4/5] Free unused memblock after boot. Use new memblock_free_ptr().
[5/5] Fix a tool build error caused by memblock_free_ptr().

And now I'm considering to move ownership of copied bootconfig data
to lib/bootconfig. That will make code simpler. But I also think fixing
bugs are urgent. So I send this series first.

Thank you,

---

Masami Hiramatsu (5):
      bootconfig: Fix to check the xbc_node is used before free it
      bootconfig: init: Fix memblock leak in xbc_make_cmdline()
      bootconfig: init: Fix memblock leak in setup_boot_config()
      bootconfig: Free copied bootconfig data after boot
      tools/bootconfig: Define memblock_free_ptr() to fix build error


 init/main.c                               |    9 +++++++++
 lib/bootconfig.c                          |    3 ++-
 tools/bootconfig/include/linux/memblock.h |    3 +--
 3 files changed, 12 insertions(+), 3 deletions(-)

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

[PATCH v2 1/5] bootconfig: Fix to check the xbc_node is used before free it

[Masami Hiramatsu]

Fix to check the xbc_node is used before calling memblock_free()
because passing NULL to phys_addr() will cause a panic.
This will happen if user doesn't pass any bootconfig to the
kernel, because kernel will call xbc_destroy_all() after
booting.

Fixes: 40caa127f3c7 ("init: bootconfig: Remove all bootconfig data when the init memory is removed")
Reported-by: kernel test robot <oliver.sang@intel.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v2:
   - Rebase on top of Linus tree.
---
 lib/bootconfig.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 5ae248b29373..bee691ea8213 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -792,7 +792,8 @@ void __init xbc_destroy_all(void)
 	xbc_data = NULL;
 	xbc_data_size = 0;
 	xbc_node_num = 0;
-	memblock_free_ptr(xbc_nodes, sizeof(struct xbc_node) * XBC_NODE_MAX);
+	if (xbc_nodes)
+		memblock_free_ptr(xbc_nodes, sizeof(struct xbc_node) * XBC_NODE_MAX);
 	xbc_nodes = NULL;
 	brace_index = 0;
 }

Re: [PATCH v2 1/5] bootconfig: Fix to check the xbc_node is used before free it

[Linus Torvalds]

On Tue, Sep 14, 2021 at 5:53 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Fix to check the xbc_node is used before calling memblock_free()
> because passing NULL to phys_addr() will cause a panic.

No.

That's the previous bad situation.

The whole point of memblock_free_ptr() is that it actually acts the
way a memory freeing function *should*, and has no problems with NULL
pointers.

>    - Rebase on top of Linus tree.

Please don't do a mindless rebase, take the actual changes into account.

         Linus

Re: [PATCH v2 1/5] bootconfig: Fix to check the xbc_node is used before free it

[Masami Hiramatsu]

On Tue, 14 Sep 2021 18:12:28 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, Sep 14, 2021 at 5:53 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Fix to check the xbc_node is used before calling memblock_free()
> > because passing NULL to phys_addr() will cause a panic.
> 
> No.
> 
> That's the previous bad situation.
> 
> The whole point of memblock_free_ptr() is that it actually acts the
> way a memory freeing function *should*, and has no problems with NULL
> pointers.

Oops, sorry. Please drop it.

> 
> >    - Rebase on top of Linus tree.
> 
> Please don't do a mindless rebase, take the actual changes into account.

Sorry about that. I missed the change.

Thank you,

> 
>          Linus


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH v2 2/5] 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 v2 3/5] 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 v2 4/5] bootconfig: Free copied bootconfig data after boot

[Masami Hiramatsu]

Free copied bootconfig data after booting kernel because that
data will not be used anymore.

commit 40caa127f3c7 ("init: bootconfig: Remove all bootconfig
data when the init memory is removed") freed the bootconfig
xbc_node array after booting kernel, but forgot to free the
bootconfig data itself. This fixes that to free the bootconfig
data too.

Fixes: 40caa127f3c7 ("init: bootconfig: Remove all bootconfig data when the init memory is removed")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v2:
  - Split memblock leak fix because it fixes another commit.
  - Use memblock_free_ptr()
---
 init/main.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/init/main.c b/init/main.c
index 4f059fde1df0..0148152652e9 100644
--- a/init/main.c
+++ b/init/main.c
@@ -319,6 +319,8 @@ static void * __init get_boot_config_from_initrd(u32 *_size, u32 *_csum)
 #ifdef CONFIG_BOOT_CONFIG
 
 static char xbc_namebuf[XBC_KEYLEN_MAX] __initdata;
+static void *init_xbc_data_copy __initdata;
+static phys_addr_t init_xbc_data_size __initdata;
 
 #define rest(dst, end) ((end) > (dst) ? (end) - (dst) : 0)
 
@@ -466,12 +468,17 @@ static void __init setup_boot_config(void)
 		extra_command_line = xbc_make_cmdline("kernel");
 		/* Also, "init." keys are init arguments */
 		extra_init_args = xbc_make_cmdline("init");
+		init_xbc_data_copy = copy;
+		init_xbc_data_size = size + 1;
 	}
 	return;
 }
 
 static void __init exit_boot_config(void)
 {
+	if (!init_xbc_data_copy)
+		return;
+	memblock_free_ptr(init_xbc_data_copy, init_xbc_data_size);
 	xbc_destroy_all();
 }
 

Re: [PATCH v2 4/5] bootconfig: Free copied bootconfig data after boot

[Linus Torvalds]

On Tue, Sep 14, 2021 at 5:53 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Free copied bootconfig data after booting kernel because that
> data will not be used anymore.

Don't do this.

You have already passed in that 'copy' to the xbc code, and
xbc_destroy_all() should just free it.

Don't add new pointless variables to keep track of state that somebody
else already keeps track of.

           Linus

Re: [PATCH v2 4/5] bootconfig: Free copied bootconfig data after boot

[Masami Hiramatsu]

On Tue, 14 Sep 2021 18:13:51 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, Sep 14, 2021 at 5:53 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Free copied bootconfig data after booting kernel because that
> > data will not be used anymore.
> 
> Don't do this.
> 
> You have already passed in that 'copy' to the xbc code, and
> xbc_destroy_all() should just free it.
> 
> Don't add new pointless variables to keep track of state that somebody
> else already keeps track of.

Ah, OK. So when I pass the copy, the ownership should be passed too.

Thank you,

> 
>            Linus


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH v2 5/5] tools/bootconfig: Define memblock_free_ptr() to fix build error

[Masami Hiramatsu]

Since commit 77e02cf57b6c ("memblock: introduce saner
'memblock_free_ptr()' interface") introduced memblock_free_ptr()
to lib/bootconfig.c, bootconfig tool also has to define
memblock_free_ptr() wrapper, and remove unused __pa() and
memblock_free().

Fixes: 77e02cf57b6c ("memblock: introduce saner 'memblock_free_ptr()' interface")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/bootconfig/include/linux/memblock.h |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/bootconfig/include/linux/memblock.h b/tools/bootconfig/include/linux/memblock.h
index 7862f217d85d..f2e506f7d57f 100644
--- a/tools/bootconfig/include/linux/memblock.h
+++ b/tools/bootconfig/include/linux/memblock.h
@@ -4,9 +4,8 @@
 
 #include <stdlib.h>
 
-#define __pa(addr)	(addr)
 #define SMP_CACHE_BYTES	0
 #define memblock_alloc(size, align)	malloc(size)
-#define memblock_free(paddr, size)	free(paddr)
+#define memblock_free_ptr(paddr, size)	free(paddr)
 
 #endif

Re: [PATCH v2 5/5] tools/bootconfig: Define memblock_free_ptr() to fix build error

[Linus Torvalds]

On Tue, Sep 14, 2021 at 5:53 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Since commit 77e02cf57b6c ("memblock: introduce saner
> 'memblock_free_ptr()' interface") introduced memblock_free_ptr()
> to lib/bootconfig.c, bootconfig tool also has to define
> memblock_free_ptr() wrapper, and remove unused __pa() and
> memblock_free().

Christ.

I grepped for this, and couldn't find any use of that memblock_free
function in the tools directory, so I ignored it.

It seems like the code in lib/bootconfig.c is compiled both into the
kernel and into that tool. This is a nightmare. We've explicitly tried
to avoid this for the tooling headers exactly because of issues like
this.

           Linus

Re: [PATCH v2 5/5] tools/bootconfig: Define memblock_free_ptr() to fix build error

[Masami Hiramatsu]

On Tue, 14 Sep 2021 18:21:09 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, Sep 14, 2021 at 5:53 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Since commit 77e02cf57b6c ("memblock: introduce saner
> > 'memblock_free_ptr()' interface") introduced memblock_free_ptr()
> > to lib/bootconfig.c, bootconfig tool also has to define
> > memblock_free_ptr() wrapper, and remove unused __pa() and
> > memblock_free().
> 
> Christ.
> 
> I grepped for this, and couldn't find any use of that memblock_free
> function in the tools directory, so I ignored it.
> 
> It seems like the code in lib/bootconfig.c is compiled both into the
> kernel and into that tool. This is a nightmare. We've explicitly tried
> to avoid this for the tooling headers exactly because of issues like
> this.

Hmm, OK. Let me copy lib/bootconfig.c itself into tools/bootconfig
as a user-space code.

Thank you,

> 
>            Linus


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 5/5] tools/bootconfig: Define memblock_free_ptr() to fix build error

[Linus Torvalds]

On Tue, Sep 14, 2021 at 6:47 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hmm, OK. Let me copy lib/bootconfig.c itself into tools/bootconfig
> as a user-space code.

Well, or we need to have some really good way to mark these shared files.

Normally I don't think we share any *.c files with tooling, and
tooling copies over the *.h files it needs. Is this the only one?

So yes, copying the *.c file in this case would match what we do for
the header files, but particularly if there are others, maybe we could
have something like the "uapi" directory that allows people to
explicialy share files with the tools.

But it would need to be very explicit in the pathname, so that people
would have that big warning sign of "hey, now you're editing a file
that is shared with tooling".

That has worked at least _somewhat_ with include/uapi/ and arch/*/include/uapi/.

         Linus

Re: [PATCH v2 5/5] tools/bootconfig: Define memblock_free_ptr() to fix build error

[Masami Hiramatsu]

On Tue, 14 Sep 2021 18:57:34 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, Sep 14, 2021 at 6:47 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Hmm, OK. Let me copy lib/bootconfig.c itself into tools/bootconfig
> > as a user-space code.
> 
> Well, or we need to have some really good way to mark these shared files.
> 
> Normally I don't think we share any *.c files with tooling, and
> tooling copies over the *.h files it needs. Is this the only one?

What I need to share is lib/bootconfig.c and include/linux/bootconfig.h.
Those provides bootconfig APIs and parser.

But since bootconfig.c uses some kernel APIs, I made wrapper
header files. If I can add #ifdefs to split the parser and APIs
from those part (as you can see 90% of code doesn't need kernel
APIs), I think I don't need any wrappers to include the file
(instead of copying bootconfig.c).

> So yes, copying the *.c file in this case would match what we do for
> the header files, but particularly if there are others, maybe we could
> have something like the "uapi" directory that allows people to
> explicialy share files with the tools.

OK.

> But it would need to be very explicit in the pathname, so that people
> would have that big warning sign of "hey, now you're editing a file
> that is shared with tooling".
> 
> That has worked at least _somewhat_ with include/uapi/ and arch/*/include/uapi/.

Hmm, what about lib/uapi/bootconfig.c ?

Thank you,

> 
>          Linus


-- 
Masami Hiramatsu <mhiramat@kernel.org>