bootconfig: Fixes to bootconfig memory management etc.
Subsystems (3)
Masami Hiramatsu
maintainerv4 cover bootconfig: Fixes to bootconfig memory management etc.
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(-)
Signature · 1 line
Masami Hiramatsu (Linaro) <[email protected]>
Masami Hiramatsu
maintainerPATCH v4 1/4 bootconfig: init: Fix memblock leak in xbc_make_cmdline()
Free unused memblock in a error case to fix memblock leak in xbc_make_cmdline().
--- init/main.c | 1 + 1 file changed, 1 insertion(+)
init/main.c +1 -0
@@ -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 ; }
Re: [PATCH v4 1/4] bootconfig: init: Fix memblock leak in xbc_make_cmdline()
Quoted 14 lines · On Thu, 16 Sep 2021 15:23:12 +0900 Masami Hiramatsu <[email protected]> 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 <[email protected]>--- init/main.c | 1 + 1 file changed, 1 insertion(+)init/main.c +0
-0
init/main.c static char * __init xbc_make_cmdline(const char *key) · line 388
Quoted 7 lines
init/main.c +1
-0@@ -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
Masami Hiramatsu
maintainerRe: [PATCH v4 1/4] bootconfig: init: Fix memblock leak in xbc_make_cmdline()
Quoted 32 lines · On Wed, 6 Oct 2021 21:02:16 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 14 lines · On Thu, 16 Sep 2021 15:23:12 +0900 Masami Hiramatsu <[email protected]> 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 <[email protected]>--- init/main.c | 1 + 1 file changed, 1 insertion(+)init/main.c +0
-0init/main.c static char * __init xbc_make_cmdline(const char *key) · line 388
Quoted 7 lines
init/main.c +1
-0@@ -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.
Quoted 1 line
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];
Quoted 2 lines
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/[email protected]/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,
Quoted 4 lines
Thanks!
-- Steve
Signature · 1 line
Masami Hiramatsu <[email protected]>
Re: [PATCH v4 1/4] bootconfig: init: Fix memblock leak in xbc_make_cmdline()
Quoted 37 lines · On Thu, 7 Oct 2021 10:43:57 +0900 Masami Hiramatsu <[email protected]> wrote:
Quoted 32 lines · On Wed, 6 Oct 2021 21:02:16 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 14 lines · On Thu, 16 Sep 2021 15:23:12 +0900 Masami Hiramatsu <[email protected]> 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 <[email protected]>--- init/main.c | 1 + 1 file changed, 1 insertion(+)init/main.c +0
-0init/main.c static char * __init xbc_make_cmdline(const char *key) · line 388
Quoted 7 lines
init/main.c +1
-0@@ -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.Quoted 1 line
Masami, I just want to confirm, that this patch is still relevant, right?
Quoted 7 lines
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];
Quoted 2 lines
I'm going to leave this patch out, and just review and accept the first three patches in the series.
Bah, doing all my rebases with Linus's "nack" probably caused me to accidentally drop this one.
Quoted 6 lines
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.
Quoted 2 lines
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
Masami Hiramatsu
maintainerPATCH v4 2/4 bootconfig: Allocate xbc_data inside xbc_init()
In reply to Masami HiramatsuAllocate 'xbc_data' in the xbc_init() so that it does not need to care about the ownership of the copied data.
--- 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(-)
include/linux/bootconfig.h +1 -1
@@ -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 */
init/main.c +2 -11
@@ -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);
lib/bootconfig.c +21 -12
@@ -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) {
tools/bootconfig/main.c +3 -3
@@ -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);
Masami Hiramatsu
maintainerPATCH v4 3/4 bootconfig: Add xbc_get_info() for the node information
In reply to Masami HiramatsuAdd 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.
--- include/linux/bootconfig.h | 2 ++ init/main.c | 1 + lib/bootconfig.c | 21 +++++++++++++++++++++ tools/bootconfig/main.c | 1 + 4 files changed, 25 insertions(+)
include/linux/bootconfig.h +2 -0
@@ -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 );
init/main.c +1 -0
@@ -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" );
lib/bootconfig.c +21 -0
@@ -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;
tools/bootconfig/main.c +1 -0
@@ -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);
Masami Hiramatsu
maintainerPATCH v4 4/4 bootconfig: Rename xbc_destroy_all() to xbc_fini()
In reply to Masami HiramatsuAvoid using this noisy name and use more calm one. This is just a name change. No functional change.
--- 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(-)
include/linux/bootconfig.h +1 -1
@@ -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 );
init/main.c +1 -1
@@ -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 */
lib/bootconfig.c +4 -4
@@ -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;
tools/bootconfig/main.c +1 -1
@@ -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()
Quoted 29 lines · On Thu, 16 Sep 2021 15:23:36 +0900 Masami Hiramatsu <[email protected]> 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 <[email protected]>--- 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(-)include/linux/bootconfig.h +1
-1@@ -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 );init/main.c +0
-0
init/main.c static void __init setup_boot_config(void) · line 466
Quoted 5 lines
init/main.c +1
-1@@ -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
Quoted 1 line
init/main.c +0
-0}
Re: [PATCH v4 4/4] bootconfig: Rename xbc_destroy_all() to xbc_fini()
Quoted 5 lines · Steven Rostedt
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()
Quoted 3 lines · On Thu, 16 Sep 2021 13:16:59 -0700 Linus Torvalds…
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
Masami Hiramatsu
maintainerRe: [PATCH v4 4/4] bootconfig: Rename xbc_destroy_all() to xbc_fini()
Quoted 7 lines · On Thu, 16 Sep 2021 16:48:05 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 3 lines · On Thu, 16 Sep 2021 13:16:59 -0700 Linus Torvalds…
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.
Quoted 8 lines
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,
Quoted 2 lines
-- Steve
Signature · 1 line
Masami Hiramatsu <[email protected]>