bootconfig: Fixes to bootconfig memory management

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) <[email protected]>

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.

---
Changes in v2:
- Rebase on top of Linus tree.

---
 lib/bootconfig.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 5ae248b29373..bee691ea8213 100644
@@ -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;
 }

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.

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

Linus

Oops, sorry. Please drop it.

Sorry about that. I missed the change.

Thank you,

Masami Hiramatsu <[email protected]>

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

---
 init/main.c |    1 +
 1 file changed, 1 insertion(+)

index 3f7216934441..0b054fff8e92 100644
@@ -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;
 	}
 

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

---
 init/main.c |    1 +
 1 file changed, 1 insertion(+)

index 0b054fff8e92..4f059fde1df0 100644
@@ -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 */

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.

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

index 4f059fde1df0..0148152652e9 100644
@@ -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();
 }
 

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

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

Thank you,

Masami Hiramatsu <[email protected]>

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

---
 tools/bootconfig/include/linux/memblock.h |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 7862f217d85d..f2e506f7d57f 100644
@@ -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

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

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

Thank you,

Masami Hiramatsu <[email protected]>

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

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

OK.

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

Thank you,

Masami Hiramatsu <[email protected]>