[PATCH v2] bootconfig: do not put quotes on cmdline items unless necessary

[PATCH v2] bootconfig: do not put quotes on cmdline items unless necessary

[Rasmus Villemoes]

When trying to migrate to using bootconfig to embed the kernel's and
PID1's command line with the kernel image itself, and so allowing
changing that without modifying the bootloader, I noticed that
/proc/cmdline changed from e.g.

  console=ttymxc0,115200n8 cma=128M quiet -- --log-level=notice

to

  console="ttymxc0,115200n8" cma="128M" quiet -- --log-level="notice"

The kernel parameters are parsed just fine, and the quotes are indeed
stripped from the actual argv[] given to PID1. However, the quoting
doesn't really serve any purpose and looks excessive, and might
confuse some (naive) userspace tool trying to parse /proc/cmdline. So
do not quote the value unless it contains whitespace.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---

v2: use strpbrk(, " \t\r\n") instead of a loop doing isspace().
Technically not quite equivalent, but much more readable, and it's
quite unlikely anybody has \f or \v or 0xa0 bytes in kernel command
line arguments. Perhaps \r and \n, and maybe even \t, could also be
dropped from that list, but those at least have some chance of
appearing in the wild.

 init/main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/init/main.c b/init/main.c
index e24b0780fdff..3dd630132209 100644
--- a/init/main.c
+++ b/init/main.c
@@ -324,7 +324,7 @@ static int __init xbc_snprint_cmdline(char *buf, size_t size,
 {
 	struct xbc_node *knode, *vnode;
 	char *end = buf + size;
-	const char *val;
+	const char *val, *q;
 	int ret;
 
 	xbc_node_for_each_key_value(root, knode, val) {
@@ -342,8 +342,9 @@ static int __init xbc_snprint_cmdline(char *buf, size_t size,
 			continue;
 		}
 		xbc_array_for_each_value(vnode, val) {
-			ret = snprintf(buf, rest(buf, end), "%s=\"%s\" ",
-				       xbc_namebuf, val);
+			q = strpbrk(val, " \t\r\n") ? "\"" : "";
+			ret = snprintf(buf, rest(buf, end), "%s=%s%s%s ",
+				       xbc_namebuf, q, val, q);
 			if (ret < 0)
 				return ret;
 			buf += ret;
-- 
2.40.1.1.g1c60b9335d

Re: [PATCH v2] bootconfig: do not put quotes on cmdline items unless necessary

[Masami Hiramatsu]

On Fri,  8 Mar 2024 13:44:01 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> When trying to migrate to using bootconfig to embed the kernel's and
> PID1's command line with the kernel image itself, and so allowing
> changing that without modifying the bootloader, I noticed that
> /proc/cmdline changed from e.g.
> 
>   console=ttymxc0,115200n8 cma=128M quiet -- --log-level=notice
> 
> to
> 
>   console="ttymxc0,115200n8" cma="128M" quiet -- --log-level="notice"
> 
> The kernel parameters are parsed just fine, and the quotes are indeed
> stripped from the actual argv[] given to PID1. However, the quoting
> doesn't really serve any purpose and looks excessive, and might
> confuse some (naive) userspace tool trying to parse /proc/cmdline. So
> do not quote the value unless it contains whitespace.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> 
> v2: use strpbrk(, " \t\r\n") instead of a loop doing isspace().
> Technically not quite equivalent, but much more readable, and it's
> quite unlikely anybody has \f or \v or 0xa0 bytes in kernel command
> line arguments. Perhaps \r and \n, and maybe even \t, could also be
> dropped from that list, but those at least have some chance of
> appearing in the wild.
> 
>  init/main.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index e24b0780fdff..3dd630132209 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -324,7 +324,7 @@ static int __init xbc_snprint_cmdline(char *buf, size_t size,
>  {
>  	struct xbc_node *knode, *vnode;
>  	char *end = buf + size;
> -	const char *val;
> +	const char *val, *q;
>  	int ret;
>  
>  	xbc_node_for_each_key_value(root, knode, val) {
> @@ -342,8 +342,9 @@ static int __init xbc_snprint_cmdline(char *buf, size_t size,
>  			continue;
>  		}
>  		xbc_array_for_each_value(vnode, val) {
> -			ret = snprintf(buf, rest(buf, end), "%s=\"%s\" ",
> -				       xbc_namebuf, val);

Can you add a comment that why strpbrk(," \t\r\n") is used here?
Such comment will help when we change how to parse the cmdline options.

Thanks,

> +			q = strpbrk(val, " \t\r\n") ? "\"" : "";
> +			ret = snprintf(buf, rest(buf, end), "%s=%s%s%s ",
> +				       xbc_namebuf, q, val, q);
>  			if (ret < 0)
>  				return ret;
>  			buf += ret;
> -- 
> 2.40.1.1.g1c60b9335d
> 


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