bootconfig: Fixes to bootconfig memory management

lib/bootconfig.c void __init xbc_destroy_all(void) · line 818

Quoted 9 lines · On Wed, 15 Sep 2021 22:19:52 +0900 Masami Hiramatsu <[email protected]> wrote:

lib/bootconfig.c +2 -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)
 {

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

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.

But still I'm thinking how I can move lib/bootconfig.c and bootconfig.h

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.

Quoted 4 lines · Linus Torvalds

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