[PATCH v4 0/4] bootconfig: Fixes to bootconfig memory management etc.

[PATCH v4 0/4] bootconfig: Fixes to bootconfig memory management etc.

[Masami Hiramatsu]

Hi Steve,

Here are the 4th version of the series to fix bootconfig memory 
management issues and API cleanup.

In this version, I added patches to move the memory allocation 
for the bootconfig in xbc_init() [2/4], add xbc_get_info()[3/4],
and rename xbc_destroy_all()[4/4].

Thank you,

---

Masami Hiramatsu (4):
      bootconfig: init: Fix memblock leak in xbc_make_cmdline()
      bootconfig: Allocate xbc_data inside xbc_init()
      bootconfig: Add xbc_get_info() for the node information
      bootconfig: Rename xbc_destroy_all() to xbc_fini()


 include/linux/bootconfig.h |    6 +++-
 init/main.c                |   17 ++++--------
 lib/bootconfig.c           |   60 +++++++++++++++++++++++++++++++++-----------
 tools/bootconfig/main.c    |    9 ++++---
 4 files changed, 59 insertions(+), 33 deletions(-)

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

[PATCH v4 1/4] 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 v4 2/4] bootconfig: Allocate xbc_data inside xbc_init()

[Masami Hiramatsu]

Allocate 'xbc_data' in the xbc_init() so that it does
not need to care about the ownership of the copied
data.

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 include/linux/bootconfig.h |    2 +-
 init/main.c                |   13 ++-----------
 lib/bootconfig.c           |   33 +++++++++++++++++++++------------
 tools/bootconfig/main.c    |    6 +++---
 4 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
index 537e1b991f11..62e09b788172 100644
--- a/include/linux/bootconfig.h
+++ b/include/linux/bootconfig.h
@@ -271,7 +271,7 @@ static inline int __init xbc_node_compose_key(struct xbc_node *node,
 }
 
 /* XBC node initializer */
-int __init xbc_init(char *buf, const char **emsg, int *epos);
+int __init xbc_init(const char *buf, size_t size, const char **emsg, int *epos);
 
 
 /* XBC cleanup data structures */
diff --git a/init/main.c b/init/main.c
index 0b054fff8e92..77c309ed9f17 100644
--- a/init/main.c
+++ b/init/main.c
@@ -410,7 +410,7 @@ static void __init setup_boot_config(void)
 	const char *msg;
 	int pos;
 	u32 size, csum;
-	char *data, *copy, *err;
+	char *data, *err;
 	int ret;
 
 	/* Cut out the bootconfig data even if we have no bootconfig option */
@@ -443,16 +443,7 @@ static void __init setup_boot_config(void)
 		return;
 	}
 
-	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);
+	ret = xbc_init(data, size, &msg, &pos);
 	if (ret < 0) {
 		if (pos < 0)
 			pr_err("Failed to init bootconfig: %s.\n", msg);
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 5ae248b29373..66b02fddfea8 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;
@@ -799,19 +800,20 @@ void __init xbc_destroy_all(void)
 
 /**
  * xbc_init() - Parse given XBC file and build XBC internal tree
- * @buf: boot config text
+ * @data: The boot config text original data
+ * @size: The size of @data
  * @emsg: A pointer of const char * to store the error message
  * @epos: A pointer of int to store the error position
  *
- * This parses the boot config text in @buf. @buf must be a
- * null terminated string and smaller than XBC_DATA_MAX.
+ * This parses the boot config text in @data. @size must be smaller
+ * than XBC_DATA_MAX.
  * Return the number of stored nodes (>0) if succeeded, or -errno
  * if there is any error.
  * 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.
  */
-int __init xbc_init(char *buf, const char **emsg, int *epos)
+int __init xbc_init(const char *data, size_t size, const char **emsg, int *epos)
 {
 	char *p, *q;
 	int ret, c;
@@ -824,28 +826,35 @@ int __init xbc_init(char *buf, const char **emsg, int *epos)
 			*emsg = "Bootconfig is already initialized";
 		return -EBUSY;
 	}
-
-	ret = strlen(buf);
-	if (ret > XBC_DATA_MAX - 1 || ret == 0) {
+	if (size > XBC_DATA_MAX || size == 0) {
 		if (emsg)
-			*emsg = ret ? "Config data is too big" :
+			*emsg = size ? "Config data is too big" :
 				"Config data is empty";
 		return -ERANGE;
 	}
 
+	xbc_data = memblock_alloc(size + 1, SMP_CACHE_BYTES);
+	if (!xbc_data) {
+		if (emsg)
+			*emsg = "Failed to allocate bootconfig data";
+		return -ENOMEM;
+	}
+	memcpy(xbc_data, data, size);
+	xbc_data[size] = '\0';
+	xbc_data_size = size + 1;
+
 	xbc_nodes = memblock_alloc(sizeof(struct xbc_node) * XBC_NODE_MAX,
 				   SMP_CACHE_BYTES);
 	if (!xbc_nodes) {
 		if (emsg)
 			*emsg = "Failed to allocate bootconfig nodes";
+		xbc_destroy_all();
 		return -ENOMEM;
 	}
 	memset(xbc_nodes, 0, sizeof(struct xbc_node) * XBC_NODE_MAX);
-	xbc_data = buf;
-	xbc_data_size = ret + 1;
-	last_parent = NULL;
 
-	p = buf;
+	last_parent = NULL;
+	p = xbc_data;
 	do {
 		q = strpbrk(p, "{}=+;:\n#");
 		if (!q) {
diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index fd67496a947f..7269c9e35335 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -229,7 +229,7 @@ static int load_xbc_from_initrd(int fd, char **buf)
 		return -EINVAL;
 	}
 
-	ret = xbc_init(*buf, &msg, NULL);
+	ret = xbc_init(*buf, size, &msg, NULL);
 	/* Wrong data */
 	if (ret < 0) {
 		pr_err("parse error: %s.\n", msg);
@@ -269,7 +269,7 @@ static int init_xbc_with_error(char *buf, int len)
 	if (!copy)
 		return -ENOMEM;
 
-	ret = xbc_init(buf, &msg, &pos);
+	ret = xbc_init(buf, len, &msg, &pos);
 	if (ret < 0)
 		show_xbc_error(copy, msg, pos);
 	free(copy);
@@ -382,7 +382,7 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	memcpy(data, buf, size);
 
 	/* Check the data format */
-	ret = xbc_init(buf, &msg, &pos);
+	ret = xbc_init(buf, size, &msg, &pos);
 	if (ret < 0) {
 		show_xbc_error(data, msg, pos);
 		free(data);

[PATCH v4 3/4] bootconfig: Add xbc_get_info() for the node information

[Masami Hiramatsu]

Add xbc_get_info() API which allows user to get the
number of used xbc_nodes and the size of bootconfig
data. This is also useful for checking the bootconfig
is initialized or not.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 include/linux/bootconfig.h |    2 ++
 init/main.c                |    1 +
 lib/bootconfig.c           |   21 +++++++++++++++++++++
 tools/bootconfig/main.c    |    1 +
 4 files changed, 25 insertions(+)

diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
index 62e09b788172..f955bb7eabbb 100644
--- a/include/linux/bootconfig.h
+++ b/include/linux/bootconfig.h
@@ -273,6 +273,8 @@ static inline int __init xbc_node_compose_key(struct xbc_node *node,
 /* XBC node initializer */
 int __init xbc_init(const char *buf, size_t size, const char **emsg, int *epos);
 
+/* XBC node and size information */
+int __init xbc_get_info(int *node_size, size_t *data_size);
 
 /* XBC cleanup data structures */
 void __init xbc_destroy_all(void);
diff --git a/init/main.c b/init/main.c
index 77c309ed9f17..747b4fd38a1a 100644
--- a/init/main.c
+++ b/init/main.c
@@ -451,6 +451,7 @@ static void __init setup_boot_config(void)
 			pr_err("Failed to parse bootconfig: %s at %d.\n",
 				msg, pos);
 	} else {
+		xbc_get_info(&ret, NULL);
 		pr_info("Load bootconfig: %d bytes %d nodes\n", size, ret);
 		/* keys starting with "kernel." are passed via cmdline */
 		extra_command_line = xbc_make_cmdline("kernel");
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 66b02fddfea8..b088fe5c0001 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -34,6 +34,27 @@ static int xbc_err_pos __initdata;
 static int open_brace[XBC_DEPTH_MAX] __initdata;
 static int brace_index __initdata;
 
+/**
+ * xbc_get_info() - Get the information of loaded boot config
+ * node_size: A pointer to store the number of nodes.
+ * data_size: A pointer to store the size of bootconfig data.
+ *
+ * Get the number of used nodes in @node_size if it is not NULL,
+ * and the size of bootconfig data in @data_size if it is not NULL.
+ * Return 0 if the boot config is initialized, or return -ENODEV.
+ */
+int __init xbc_get_info(int *node_size, size_t *data_size)
+{
+	if (!xbc_data)
+		return -ENODEV;
+
+	if (node_size)
+		*node_size = xbc_node_num;
+	if (data_size)
+		*data_size = xbc_data_size;
+	return 0;
+}
+
 static int __init xbc_parse_error(const char *msg, const char *p)
 {
 	xbc_err_msg = msg;
diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 7269c9e35335..4f2a8d884745 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -391,6 +391,7 @@ static int apply_xbc(const char *path, const char *xbc_path)
 		return ret;
 	}
 	printf("Apply %s to %s\n", xbc_path, path);
+	xbc_get_info(&ret, NULL);
 	printf("\tNumber of nodes: %d\n", ret);
 	printf("\tSize: %u bytes\n", (unsigned int)size);
 	printf("\tChecksum: %d\n", (unsigned int)csum);

[PATCH v4 4/4] bootconfig: Rename xbc_destroy_all() to xbc_fini()

[Masami Hiramatsu]

Avoid using this noisy name and use more calm one.
This is just a name change. No functional change.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 include/linux/bootconfig.h |    2 +-
 init/main.c                |    2 +-
 lib/bootconfig.c           |    8 ++++----
 tools/bootconfig/main.c    |    2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
index f955bb7eabbb..ba40194a339c 100644
--- a/include/linux/bootconfig.h
+++ b/include/linux/bootconfig.h
@@ -277,7 +277,7 @@ int __init xbc_init(const char *buf, size_t size, const char **emsg, int *epos);
 int __init xbc_get_info(int *node_size, size_t *data_size);
 
 /* XBC cleanup data structures */
-void __init xbc_destroy_all(void);
+void __init xbc_fini(void);
 
 /* Debug dump functions */
 void __init xbc_debug_dump(void);
diff --git a/init/main.c b/init/main.c
index 747b4fd38a1a..99a23324d4a1 100644
--- a/init/main.c
+++ b/init/main.c
@@ -463,7 +463,7 @@ static void __init setup_boot_config(void)
 
 static void __init exit_boot_config(void)
 {
-	xbc_destroy_all();
+	xbc_fini();
 }
 
 #else	/* !CONFIG_BOOT_CONFIG */
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index b088fe5c0001..43a402b02748 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -802,13 +802,13 @@ static int __init xbc_verify_tree(void)
 }
 
 /**
- * xbc_destroy_all() - Clean up all parsed bootconfig
+ * xbc_fini() - Clean up all parsed bootconfig
  *
  * This clears all data structures of parsed bootconfig on memory.
  * If you need to reuse xbc_init() with new boot config, you can
  * use this.
  */
-void __init xbc_destroy_all(void)
+void __init xbc_fini(void)
 {
 	memblock_free_ptr(xbc_data, xbc_data_size);
 	xbc_data = NULL;
@@ -869,7 +869,7 @@ int __init xbc_init(const char *data, size_t size, const char **emsg, int *epos)
 	if (!xbc_nodes) {
 		if (emsg)
 			*emsg = "Failed to allocate bootconfig nodes";
-		xbc_destroy_all();
+		xbc_fini();
 		return -ENOMEM;
 	}
 	memset(xbc_nodes, 0, sizeof(struct xbc_node) * XBC_NODE_MAX);
@@ -925,7 +925,7 @@ int __init xbc_init(const char *data, size_t size, const char **emsg, int *epos)
 			*epos = xbc_err_pos;
 		if (emsg)
 			*emsg = xbc_err_msg;
-		xbc_destroy_all();
+		xbc_fini();
 	} else
 		ret = xbc_node_num;
 
diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 4f2a8d884745..84808a1871f0 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -397,7 +397,7 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	printf("\tChecksum: %d\n", (unsigned int)csum);
 
 	/* TODO: Check the options by schema */
-	xbc_destroy_all();
+	xbc_fini();
 	free(buf);
 
 	/* Remove old boot config if exists */

Re: [PATCH v4 4/4] bootconfig: Rename xbc_destroy_all() to xbc_fini()

[Steven Rostedt]

On Thu, 16 Sep 2021 15:23:36 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Avoid using this noisy name and use more calm one.
> This is just a name change. No functional change.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  include/linux/bootconfig.h |    2 +-
>  init/main.c                |    2 +-
>  lib/bootconfig.c           |    8 ++++----
>  tools/bootconfig/main.c    |    2 +-
>  4 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
> index f955bb7eabbb..ba40194a339c 100644
> --- a/include/linux/bootconfig.h
> +++ b/include/linux/bootconfig.h
> @@ -277,7 +277,7 @@ int __init xbc_init(const char *buf, size_t size, const char **emsg, int *epos);
>  int __init xbc_get_info(int *node_size, size_t *data_size);
>  
>  /* XBC cleanup data structures */
> -void __init xbc_destroy_all(void);
> +void __init xbc_fini(void);
>  
>  /* Debug dump functions */
>  void __init xbc_debug_dump(void);
> diff --git a/init/main.c b/init/main.c
> index 747b4fd38a1a..99a23324d4a1 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -463,7 +463,7 @@ static void __init setup_boot_config(void)
>  
>  static void __init exit_boot_config(void)
>  {
> -	xbc_destroy_all();
> +	xbc_fini();

I didn't know this was a thing. But looking for other use cases with
"*_fini(", there seems to be plenty of precedence in the kernel for this
change.

-- Steve


>  }

Re: [PATCH v4 4/4] bootconfig: Rename xbc_destroy_all() to xbc_fini()

[Linus Torvalds]

On Thu, Sep 16, 2021 at 6:26 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
> I didn't know this was a thing. But looking for other use cases with
> "*_fini(", there seems to be plenty of precedence in the kernel for this
> change.

I wouldn't encourage it.

It's an odd compiler thing, where initializers and destructors are in
'init' and 'fini' segments respectively.

It makes absolutely no sense in any other context, and the fact that
it has bled into kernel usage is not a good thing imnsho.

Honestly, "exit" is the normal prefix/postfix, and is actually a real
word. As is "destroy", used elsewhere.

So I'm not going to NAK 'fini', but it's a completely stupid and
pointless thing to use and there are better character sequences that
aren't any more typing and are real words.

          Linus

Re: [PATCH v4 4/4] bootconfig: Rename xbc_destroy_all() to xbc_fini()

[Linus Torvalds]

On Thu, Sep 16, 2021 at 6:26 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
> I didn't know this was a thing. But looking for other use cases with
> "*_fini(", there seems to be plenty of precedence in the kernel for this
> change.

I wouldn't encourage it.

It's an odd compiler thing, where initializers and destructors are in
'init' and 'fini' segments respectively.

It makes absolutely no sense in any other context, and the fact that
it has bled into kernel usage is not a good thing imnsho.

Honestly, "exit" is the normal prefix/postfix, and is actually a real
word. As is "destroy", used elsewhere.

So I'm not going to NAK 'fini', but it's a completely stupid and
pointless thing to use and there are better character sequences that
aren't any more typing and are real words.

          Linus

Re: [PATCH v4 4/4] bootconfig: Rename xbc_destroy_all() to xbc_fini()

[Steven Rostedt]

On Thu, 16 Sep 2021 13:16:59 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> So I'm not going to NAK 'fini', but it's a completely stupid and
> pointless thing to use and there are better character sequences that
> aren't any more typing and are real words.

I didn't like it when I first saw it, but only was going to take it because
it's used elsewhere in the kernel.

Because of your response, and my initial feeling about the change, I'm going
to leave this patch out, and just review and accept the first three patches
in the series.

Masami, are you OK with that?

-- Steve

Re: [PATCH v4 4/4] bootconfig: Rename xbc_destroy_all() to xbc_fini()

[Masami Hiramatsu]

On Thu, 16 Sep 2021 16:48:05 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 16 Sep 2021 13:16:59 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > So I'm not going to NAK 'fini', but it's a completely stupid and
> > pointless thing to use and there are better character sequences that
> > aren't any more typing and are real words.
> 
> I didn't like it when I first saw it, but only was going to take it because
> it's used elsewhere in the kernel.
> 
> Because of your response, and my initial feeling about the change, I'm going
> to leave this patch out, and just review and accept the first three patches
> in the series.
> 
> Masami, are you OK with that?

Yes, I'm OK. And I will update it to use "xbc_exit()" then.

Anyway, it is good to know your opinion about this. :-)
I also noticed this "_fini" recently when reviewing patches.

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v4 1/4] bootconfig: init: Fix memblock leak in xbc_make_cmdline()

[Steven Rostedt]

On Thu, 16 Sep 2021 15:23:12 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

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

Hmm, looking at my patch queue, I noticed that this did not get
applied. I'm thinking I may have been confused with the other memory
freeing that was put into the xbc_destroy(), thinking this was part of
that. But now that I look at this patch in the context of the code, it
looks like this patch is required, as "new_cmdline" never gets exposed
on this error.

Masami, I just want to confirm, that this patch is still relevant, right?

Thanks!

-- Steve

Re: [PATCH v4 1/4] bootconfig: init: Fix memblock leak in xbc_make_cmdline()

[Masami Hiramatsu]

On Wed, 6 Oct 2021 21:02:16 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 16 Sep 2021 15:23:12 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > 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;
> >  	}
> >  
> 
> Hmm, looking at my patch queue, I noticed that this did not get
> applied. I'm thinking I may have been confused with the other memory
> freeing that was put into the xbc_destroy(), thinking this was part of
> that. But now that I look at this patch in the context of the code, it
> looks like this patch is required, as "new_cmdline" never gets exposed
> on this error.
> 
> Masami, I just want to confirm, that this patch is still relevant, right?

Yes, with other 2 patches in this series ([1/4]-[3/4]), I thought you already
queued it in your tree as you said in [1];

> I'm going to leave this patch out, and just review and accept the first three patches
> in the series.

[1] https://lore.kernel.org/all/20210916164805.32592423@gandalf.local.home/T/#u

So, my next cleanup series [2] (including xbc_destroy_all() -> xbc_exit()) was
based on the [1]'s first 3 patches.

[2] https://lore.kernel.org/all/163187294400.2366983.7393164788107844569.stgit@devnote2/T/#u

If it helps, I can make these series to one series and rebase on top of your
for-next (or ftrace/core) branch.


Thank you,

> 
> Thanks!
> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v4 1/4] bootconfig: init: Fix memblock leak in xbc_make_cmdline()

[Steven Rostedt]

On Thu, 7 Oct 2021 10:43:57 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Wed, 6 Oct 2021 21:02:16 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu, 16 Sep 2021 15:23:12 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >   
> > > 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;
> > >  	}
> > >    
> > 
> > Hmm, looking at my patch queue, I noticed that this did not get
> > applied. I'm thinking I may have been confused with the other memory
> > freeing that was put into the xbc_destroy(), thinking this was part of
> > that. But now that I look at this patch in the context of the code, it
> > looks like this patch is required, as "new_cmdline" never gets exposed
> > on this error.
> > 
> > Masami, I just want to confirm, that this patch is still relevant, right?  
> 
> Yes, with other 2 patches in this series ([1/4]-[3/4]), I thought you already
> queued it in your tree as you said in [1];
> 
> > I'm going to leave this patch out, and just review and accept the first three patches
> > in the series.  
> 
> [1] https://lore.kernel.org/all/20210916164805.32592423@gandalf.local.home/T/#u

Bah, doing all my rebases with Linus's "nack" probably caused me to
accidentally drop this one.

> 
> So, my next cleanup series [2] (including xbc_destroy_all() -> xbc_exit()) was
> based on the [1]'s first 3 patches.
> 
> [2] https://lore.kernel.org/all/163187294400.2366983.7393164788107844569.stgit@devnote2/T/#u
> 

I just went through my queue from as far back as August, to pick up
everything that I left behind to worry about Linux Plumbers and Open
Source Summit, and found this set in that queue. I'll be processing all
these patches in the next few days.


> If it helps, I can make these series to one series and rebase on top of your
> for-next (or ftrace/core) branch.

No, this patch should be added to my urgent tree and pushed onto Linus
(and stable).

I'm working on those patches first, then will move on to my for-next
tree.

Thanks,

-- Steve