Received-SPF: pass (google.com: domain of linux-kernel+bounces-98077-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sun, 10 Mar 2024 10:25:34 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] bootconfig: do not put quotes on cmdline items
 unless necessary
Message-Id: <20240310102534.cf79e09037df23b4f51981b0@kernel.org>
In-Reply-To: <20240308124401.1702046-1-linux@rasmusvillemoes.dk>
References: <20240308124401.1702046-1-linux@rasmusvillemoes.dk>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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>