Re: [PATCH RFC bootconfig] 1/2] fs/proc: Add /proc/cmdline_load for boot loader arguments

From: Paul E. McKenney
Date: Sun Jul 30 2023 - 14:17:50 EST

Next message: Svyatoslav Ryhel: "Re: [PATCH v3 0/2] GPIO-based hotplug i2c bus"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v2 1/3] dt-bindings: power: supply: maxim,max17040: update properties"
In reply to: Google: "Re: [PATCH RFC bootconfig] 1/2] fs/proc: Add /proc/cmdline_load for boot loader arguments"
Next in thread: Randy Dunlap: "Re: [PATCH RFC bootconfig 0/2] Distinguish bootloader and embedded kernel parameters"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Jul 30, 2023 at 10:58:44AM +0900, Masami Hiramatsu wrote:
> On Sat, 29 Jul 2023 09:16:56 -0700
> "Paul E. McKenney" <paulmck@xxxxxxxxxx> wrote:
>
> > On Sat, Jul 29, 2023 at 11:29:29PM +0900, Masami Hiramatsu wrote:
> > > Hi Paul,
> > >
> > > On Thu, 27 Jul 2023 20:37:00 -0700
> > > "Paul E. McKenney" <paulmck@xxxxxxxxxx> 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\\.`.
>
> ^^^ /proc/cmdline - `/proc/bootconfig | grep ^kernel\\.`

Heh! My mind autocorrected without me noticing. ;-)

> > Yes, very much something like that.
> >
> > For one use case, suppose you have a kernel that gets some boot parameters
> > from the boot loader and some from bootconfig. If you want to kexec()
> > into a new kernel, you must tell kexec() what the kernel boot parameters
> > are. However, you must *not* tell kexec() about any of the current
> > kernel's parameters that came from bootconfig, because those should
> > instead be supplied by the new kernel being kexec()ed into.
> >
> > So you must pass in only those parameters that came from the boot loader,
> > hence my proposed /proc/cmdline_load.
>
> Ah, I got it. Indeed, for kexec, we need to drop the options from
> the bootconfig.
>
> > > 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...
> >
> > The use case is embedded systems, right? I have no idea whether they
> > have a use case requiring this. Do those sorts of embedded systems
> > use kexec()? (I don't know of any that do, but then again, I haven't
> > been looking.)
>
> Not sure, I guess it is possible to use kexec() for kdump or warm reboot,
> but it should be rare and we can expand this if someone need it.

Works for me!

> > This arch init is in setup_arch(), correct? If so, one option is to
> > make start_kernel() or something that it invokes make a copy of the
> > command line just before invoking setup_arch(). Full disclosure: I
> > have not yet looked at all the ins and outs of CONFIG_CMDLINE, so this
> > suggestion should be viewed with appropriate skepticism.
>
> Yeah, maybe it is the best way to do.
> Anyway, I understand the reason why we need this interface.
>
> Acked-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>

Thank you! I will apply your Acked-by on my next rebase.

Thanx, Paul

> Thank you!
>
> >
> > > 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@xxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx>
> > > > Co-developed-by: Arnd Bergmann <arnd@xxxxxxxxxx>
> > > > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxxxx>
> > > > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > > > Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> > > > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > > > Cc: Alexey Dobriyan <adobriyan@xxxxxxxxx>
> > > > Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> > > > Cc: <linux-fsdevel@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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.
> >
> > So if we also need to separate out the CONFIG_CMDLINE arguments, then
> > /proc/cmdline_load will need to come from some string saved off before
> > the CONFIG_CMDLINE processing, correct? I would expect that to be a
> > separate patch series, but if it is needed, I would be happy to look
> > into setting it up, as long as I am in the area.
> >
> > My tests indicate that boot_command_line[] doesn't contain any bootconfig
> > (and opposed to CONFIG_CMDLINE) arguments, but I could easily have missed
> > some other corner-case configuration.
> >
> > And thank you for looking this over!
> >
> > Thanx, Paul
> >
> > > > 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@xxxxxxxxxx>
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>

Next message: Svyatoslav Ryhel: "Re: [PATCH v3 0/2] GPIO-based hotplug i2c bus"
Previous message: Krzysztof Kozlowski: "Re: [PATCH v2 1/3] dt-bindings: power: supply: maxim,max17040: update properties"
In reply to: Google: "Re: [PATCH RFC bootconfig] 1/2] fs/proc: Add /proc/cmdline_load for boot loader arguments"
Next in thread: Randy Dunlap: "Re: [PATCH RFC bootconfig 0/2] Distinguish bootloader and embedded kernel parameters"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]