Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 29 Jul 2023 23:29:29 +0900
From:   Masami Hiramatsu (Google) <mhiramat@kernel.org>
To:     "Paul E. McKenney" <paulmck@kernel.org>
Cc:     akpm@linux-foundation.org, adobriyan@gmail.com, arnd@kernel.org,
        ndesaulniers@google.com, sfr@canb.auug.org.au,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel-team@meta.com
Subject: Re: [PATCH RFC bootconfig] 1/2] fs/proc: Add /proc/cmdline_load for
 boot loader arguments
Message-Id: <20230729232929.a3e962f46c16973031bb466c@kernel.org>
In-Reply-To: <20230728033701.817094-1-paulmck@kernel.org>
References: <197cba95-3989-4d2f-a9f1-8b192ad08c49@paulmck-laptop>
        <20230728033701.817094-1-paulmck@kernel.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Paul,

On Thu, 27 Jul 2023 20:37:00 -0700
"Paul E. McKenney" <paulmck@kernel.org> wrote:

> In kernels built with CONFIG_BOOT_CONFIG_FORCE=y, /proc/cmdline will
> show all kernel boot parameters, both those supplied by the boot loader
> and those embedded in the kernel image.  This works well for those who
> just want to see all of the kernel boot parameters, but is not helpful to
> those who need to see only those parameters supplied by the boot loader.
> This is especially important when these parameters are presented to the
> boot loader by automation that might gather them from diverse sources.
> 
> Therefore, provide a /proc/cmdline_load file that shows only those kernel
> boot parameters supplied by the boot loader.

If I understand correctly, /proc/cmdline_load is something like
/proc/cmdline_load - `/proc/bootconfig | grep ^kernel\\.`.

BTW, what about CONFIG_CMDLINE? We already have that Kconfig and it is also
merged with the command line specified by boot loader. Should we also
expose that? (when CONFIG_CMDLINE_OVERRIDE=y, we don't need it because
cmdline is always overridden by the CONFIG_CMDLINE) Unfortunatelly, this
option is implemented in each arch init, so we have to change all of them...

Thank you,

> 
> Why put this in /proc?  Because it is quite similar to /proc/cmdline, so
> it makes sense to put it in the same place that /proc/cmdline is located.
> 
> [ sfr: Apply kernel test robot feedback. ]
> 
> Co-developed-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: <linux-fsdevel@vger.kernel.org>
> ---
>  fs/proc/cmdline.c    | 13 +++++++++++++
>  include/linux/init.h |  3 ++-
>  init/main.c          |  2 +-
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/proc/cmdline.c b/fs/proc/cmdline.c
> index a6f76121955f..1d0ef9d2949d 100644
> --- a/fs/proc/cmdline.c
> +++ b/fs/proc/cmdline.c
> @@ -3,6 +3,7 @@
>  #include <linux/init.h>
>  #include <linux/proc_fs.h>
>  #include <linux/seq_file.h>
> +#include <asm/setup.h>
>  #include "internal.h"
>  
>  static int cmdline_proc_show(struct seq_file *m, void *v)
> @@ -12,6 +13,13 @@ static int cmdline_proc_show(struct seq_file *m, void *v)
>  	return 0;
>  }
>  
> +static int cmdline_load_proc_show(struct seq_file *m, void *v)
> +{
> +	seq_puts(m, boot_command_line);
> +	seq_putc(m, '\n');
> +	return 0;
> +}
> +
>  static int __init proc_cmdline_init(void)
>  {
>  	struct proc_dir_entry *pde;
> @@ -19,6 +27,11 @@ static int __init proc_cmdline_init(void)
>  	pde = proc_create_single("cmdline", 0, NULL, cmdline_proc_show);
>  	pde_make_permanent(pde);
>  	pde->size = saved_command_line_len + 1;
> +	if (IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE)) {
> +		pde = proc_create_single("cmdline_load", 0, NULL, cmdline_load_proc_show);
> +		pde_make_permanent(pde);
> +		pde->size = strnlen(boot_command_line, COMMAND_LINE_SIZE) + 1;
> +	}
>  	return 0;
>  }
>  fs_initcall(proc_cmdline_init);
> diff --git a/include/linux/init.h b/include/linux/init.h
> index 266c3e1640d4..29e75bbe7984 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
> @@ -112,6 +112,7 @@
>  #define __REFCONST       .section       ".ref.rodata", "a"
>  
>  #ifndef __ASSEMBLY__
> +
>  /*
>   * Used for initialization calls..
>   */
> @@ -143,7 +144,7 @@ struct file_system_type;
>  
>  /* Defined in init/main.c */
>  extern int do_one_initcall(initcall_t fn);
> -extern char __initdata boot_command_line[];
> +extern char boot_command_line[];

FYI, boot_command_line[] is mixture of built-in cmdline string with
bootloader cmdline string.

>  extern char *saved_command_line;
>  extern unsigned int saved_command_line_len;
>  extern unsigned int reset_devices;
> diff --git a/init/main.c b/init/main.c
> index ad920fac325c..2121685c479a 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -135,7 +135,7 @@ EXPORT_SYMBOL(system_state);
>  void (*__initdata late_time_init)(void);
>  
>  /* Untouched command line saved by arch-specific code. */
> -char __initdata boot_command_line[COMMAND_LINE_SIZE];
> +char boot_command_line[COMMAND_LINE_SIZE] __ro_after_init;
>  /* Untouched saved command line (eg. for /proc) */
>  char *saved_command_line __ro_after_init;
>  unsigned int saved_command_line_len __ro_after_init;
> -- 
> 2.40.1
> 


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