Re: [PATCH v3] bootconfig: Only load bootconfig if "bootconfig" is on the kernel cmdline

[Masami Hiramatsu <mhiramat@kernel.org>]

On Tue, 4 Feb 2020 11:04:46 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> As the bootconfig is appended to the initrd it is not as easy to modify as
> the kernel command line. If there's some issue with the kernel, and the
> developer wants to boot a pristine kernel, it should not be needed to modify
> the initrd to remove the bootconfig for a single boot.
> 
> As bootconfig is silently added (if the admin does not know where to look
> they may not know it's being loaded). It should be explicitly added to the
> kernel cmdline. The loading of the bootconfig is only done if "bootconfig"
> is on the kernel command line. This will let admins know that the kernel
> command line is extended.
> 
> Note, after adding printk()s for when the size is too great or the checksum
> is wrong, exposed that the current method always looked for the boot config,
> and if this size and checksum matched, it would parse it (as if either is
> wrong a printk has been added to show this). It's better to only check this
> if the boot config is asked to be looked for.

Thanks for adding this feature :) This looks good to me.
Please add my ack when you fix the error message of "config=bootconfig".

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you!

> 
> Link: https://lore.kernel.org/r/CAHk-=wjfjO+h6bQzrTf=YCZA53Y3EDyAs3Z4gEsT7icA3u_Psw@mail.gmail.com
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> Changes from v2:
>   - Use "bootconfig" instead of "config=bootconifg"
> 
>  Documentation/admin-guide/bootconfig.rst      |  2 ++
>  .../admin-guide/kernel-parameters.txt         |  6 ++++
>  init/main.c                                   | 28 ++++++++++++++-----
>  3 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> index 4d617693c0c8..b342a6796392 100644
> --- a/Documentation/admin-guide/bootconfig.rst
> +++ b/Documentation/admin-guide/bootconfig.rst
> @@ -123,6 +123,8 @@ To remove the config from the image, you can use -d option as below::
>  
>   # tools/bootconfig/bootconfig -d /boot/initrd.img-X.Y.Z
>  
> +Then add "bootconfig" on the normal kernel command line to tell the
> +kernel to look for the bootconfig at the end of the initrd file.
>  
>  Config File Limitation
>  ======================
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index ade4e6ec23e0..b48c70ba9841 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -437,6 +437,12 @@
>  			no delay (0).
>  			Format: integer
>  
> +	bootconfig	[KNL]
> +			Extended command line options can be added to an initrd
> +			and this will cause the kernel to look for it.
> +
> +			See Documentation/admin-guide/bootconfig.rst
> +
>  	bert_disable	[ACPI]
>  			Disable BERT OS support on buggy BIOSes.
>  
> diff --git a/init/main.c b/init/main.c
> index dd7da62d99a5..1465fb2d7277 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -336,28 +336,39 @@ u32 boot_config_checksum(unsigned char *p, u32 size)
>  	return ret;
>  }
>  
> -static void __init setup_boot_config(void)
> +static void __init setup_boot_config(const char *cmdline)
>  {
>  	u32 size, csum;
>  	char *data, *copy;
> +	const char *p;
>  	u32 *hdr;
>  
> -	if (!initrd_end)
> +	p = strstr(cmdline, "bootconfig");
> +	if (!p || (p != cmdline && !isspace(*(p-1))) ||
> +	    (p[10] && !isspace(p[10])))
>  		return;
>  
> +	if (!initrd_end)
> +		goto not_found;
> +
>  	hdr = (u32 *)(initrd_end - 8);
>  	size = hdr[0];
>  	csum = hdr[1];
>  
> -	if (size >= XBC_DATA_MAX)
> +	if (size >= XBC_DATA_MAX) {
> +		pr_err("bootconfig size %d greater than max size %d\n",
> +			size, XBC_DATA_MAX);
>  		return;
> +	}
>  
>  	data = ((void *)hdr) - size;
>  	if ((unsigned long)data < initrd_start)
> -		return;
> +		goto not_found;
>  
> -	if (boot_config_checksum((unsigned char *)data, size) != csum)
> +	if (boot_config_checksum((unsigned char *)data, size) != csum) {
> +		pr_err("bootconfig checksum failed\n");
>  		return;
> +	}
>  
>  	copy = memblock_alloc(size + 1, SMP_CACHE_BYTES);
>  	if (!copy) {
> @@ -377,9 +388,12 @@ static void __init setup_boot_config(void)
>  		/* Also, "init." keys are init arguments */
>  		extra_init_args = xbc_make_cmdline("init");
>  	}
> +	return;
> +not_found:
> +	pr_err("config=bootconfig on command line, but no bootconfig found\n");
>  }
>  #else
> -#define setup_boot_config()	do { } while (0)
> +#define setup_boot_config(cmdline)	do { } while (0)
>  #endif
>  
>  /* Change NUL term back to "=", to make "param" the whole string. */
> @@ -760,7 +774,7 @@ asmlinkage __visible void __init start_kernel(void)
>  	pr_notice("%s", linux_banner);
>  	early_security_init();
>  	setup_arch(&command_line);
> -	setup_boot_config();
> +	setup_boot_config(command_line);
>  	setup_command_line(command_line);
>  	setup_nr_cpu_ids();
>  	setup_per_cpu_areas();
> -- 
> 2.20.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>