Command line needs to be combined in both compressed and uncompressed
kernel from built-in and boot command line strings, which requires
non-trivial logic depending on CONFIG_CMDLINE_BOOL and
CONFIG_CMDLINE_OVERRIDE.
Add a helper function to avoid code duplication.
Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
---
arch/x86/include/asm/shared/cmdline.h | 35 +++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
create mode 100644 arch/x86/include/asm/shared/cmdline.h
diff --git a/arch/x86/include/asm/shared/cmdline.h b/arch/x86/include/asm/shared/cmdline.h
new file mode 100644
index 000000000000..01736d66028d
--- /dev/null
+++ b/arch/x86/include/asm/shared/cmdline.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _ASM_X86_SETUP_CMDLINE_H
+#define _ASM_X86_SETUP_CMDLINE_H
+
+#define _SETUP
+#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
+#undef _SETUP
+
+#include <linux/string.h>
+
+#ifdef CONFIG_CMDLINE_BOOL
+#define BUILTIN_COMMAND_LINE CONFIG_CMDLINE
+#else
+#define BUILTIN_COMMAND_LINE ""
+#endif
+
+static inline void cmdline_prepare(char *dst, const char *boot_command_line)
+{
+ if (!IS_ENABLED(CONFIG_CMDLINE_BOOL)) {
+ strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
+ } else {
+ strscpy(dst, BUILTIN_COMMAND_LINE, COMMAND_LINE_SIZE);
+ /*
+ * Append boot loader cmdline to builtin, if it exists
+ * and should not be overriden.
+ */
+ if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) && boot_command_line[0]) {
+ strlcat(dst, " ", COMMAND_LINE_SIZE);
+ strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
+ }
+ }
+}
+
+#endif /* _ASM_X86_SETUP_CMDLINE_H */
--
2.37.4On Thu, Nov 10, 2022 at 04:09:30PM +0300, Evgeniy Baskov wrote:
> Command line needs to be combined in both compressed and uncompressed
> kernel from built-in and boot command line strings, which requires
> non-trivial logic depending on CONFIG_CMDLINE_BOOL and
> CONFIG_CMDLINE_OVERRIDE.
>
> Add a helper function to avoid code duplication.
>
> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
> ---
> arch/x86/include/asm/shared/cmdline.h | 35 +++++++++++++++++++++++++++
> 1 file changed, 35 insertions(+)
> create mode 100644 arch/x86/include/asm/shared/cmdline.h
>
> diff --git a/arch/x86/include/asm/shared/cmdline.h b/arch/x86/include/asm/shared/cmdline.h
> new file mode 100644
> index 000000000000..01736d66028d
> --- /dev/null
> +++ b/arch/x86/include/asm/shared/cmdline.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _ASM_X86_SETUP_CMDLINE_H
> +#define _ASM_X86_SETUP_CMDLINE_H
> +
> +#define _SETUP
> +#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
> +#undef _SETUP
> +
> +#include <linux/string.h>
> +
> +#ifdef CONFIG_CMDLINE_BOOL
> +#define BUILTIN_COMMAND_LINE CONFIG_CMDLINE
> +#else
> +#define BUILTIN_COMMAND_LINE ""
> +#endif
> +
> +static inline void cmdline_prepare(char *dst, const char *boot_command_line)
> +{
> + if (!IS_ENABLED(CONFIG_CMDLINE_BOOL)) {
> + strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
> + } else {
> + strscpy(dst, BUILTIN_COMMAND_LINE, COMMAND_LINE_SIZE);
> + /*
> + * Append boot loader cmdline to builtin, if it exists
> + * and should not be overriden.
> + */
> + if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE) && boot_command_line[0]) {
> + strlcat(dst, " ", COMMAND_LINE_SIZE);
> + strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
> + }
You keep changing what I'm suggesting and the next patch has a strscpy()
outside of the function.
When I say it should be all concentrated in one function, I really mean
it.
So now it is my turn: I'll do it how I think it should be done and you
can review it.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Nov 14, 2022 at 03:28:55PM +0100, Borislav Petkov wrote:
> So now it is my turn: I'll do it how I think it should be done and you
> can review it.
Ok, here are two patches as a reply to this message.
I was able to test them as much as I can in a VM here but I'd need more
details/testing in your configuration with earlyprintk as a builtin
cmdline.
cmdline_prepare() has grown a bit hairy in the end but I've tried hard
to comment what happens there so that it is clear for the future. The
main goal being to concentrate all command line strings processing in
that function and not have it spread around the tree. And yes, there are
more cleanups possible.
In the compressed stage I'm using the cmdline which is in boot_params as
source and destination to basically add only the builtin cmdline.
In kernel proper the boot_command_line comes from generic code and that
is a whole another way of crazy in itself when I look at init/main.c
And as previously stated - the goal is to have everything in one place
and documented as good as possible so that trying to figure out how
command line parsing is done doesn't send you on grepping spree around
the tree.
Suggestions how to simplify this even more are always welcome, ofc.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
From: Borislav Petkov <bp@suse.de>
Date: Mon, 14 Nov 2022 15:31:59 +0100
Since both the compressed kernel and kernel proper need to deal with the
command line, add a common helper which abstracts away all the details.
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/include/asm/shared/cmdline.h | 34 +++++++++++++++++++++++++++
arch/x86/kernel/setup.c | 21 +++--------------
2 files changed, 37 insertions(+), 18 deletions(-)
create mode 100644 arch/x86/include/asm/shared/cmdline.h
diff --git a/arch/x86/include/asm/shared/cmdline.h b/arch/x86/include/asm/shared/cmdline.h
new file mode 100644
index 000000000000..e09c06338567
--- /dev/null
+++ b/arch/x86/include/asm/shared/cmdline.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_SHARED_CMDLINE_H
+#define _ASM_X86_SHARED_CMDLINE_H
+
+#ifdef CONFIG_CMDLINE_BOOL
+#define BUILTIN_CMDLINE CONFIG_CMDLINE
+#else
+#define BUILTIN_CMDLINE ""
+#endif
+
+static inline void cmdline_prepare(char *dst,
+ const char *builtin_cmdline,
+ char *boot_command_line)
+{
+ /* Depends on CONFIG_CMDLINE_BOOL, overwrite with builtin cmdline */
+ if (IS_ENABLED(CONFIG_CMDLINE_OVERRIDE))
+ strscpy(dst, builtin_cmdline, COMMAND_LINE_SIZE);
+ else if (IS_ENABLED(CONFIG_CMDLINE_BOOL)) {
+ if (builtin_cmdline[0]) {
+ /* Add builtin cmdline */
+ strlcat(dst, builtin_cmdline, COMMAND_LINE_SIZE);
+ strlcat(dst, " ", COMMAND_LINE_SIZE);
+ /* Add boot cmdline */
+ strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
+ }
+ } else {
+ strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
+ }
+
+ /* Copy back into boot command line, see setup_command_line() */
+ strscpy(boot_command_line, dst, COMMAND_LINE_SIZE);
+}
+
+#endif /* _ASM_X86_SHARED_CMDLINE_H */
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index aacaa96f0195..c506807142a7 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -25,6 +25,7 @@
#include <linux/static_call.h>
#include <linux/swiotlb.h>
#include <linux/random.h>
+#include <linux/vmalloc.h>
#include <uapi/linux/mount.h>
@@ -50,10 +51,10 @@
#include <asm/pci-direct.h>
#include <asm/prom.h>
#include <asm/proto.h>
+#include <asm/shared/cmdline.h>
#include <asm/thermal.h>
#include <asm/unwind.h>
#include <asm/vsyscall.h>
-#include <linux/vmalloc.h>
/*
* max_low_pfn_mapped: highest directly mapped pfn < 4 GB
@@ -168,9 +169,6 @@ unsigned long saved_video_mode;
#define RAMDISK_LOAD_FLAG 0x4000
static char __initdata command_line[COMMAND_LINE_SIZE];
-#ifdef CONFIG_CMDLINE_BOOL
-static char __initdata builtin_cmdline[COMMAND_LINE_SIZE] = CONFIG_CMDLINE;
-#endif
#if defined(CONFIG_EDD) || defined(CONFIG_EDD_MODULE)
struct edd edd;
@@ -970,20 +968,7 @@ void __init setup_arch(char **cmdline_p)
bss_resource.start = __pa_symbol(__bss_start);
bss_resource.end = __pa_symbol(__bss_stop)-1;
-#ifdef CONFIG_CMDLINE_BOOL
-#ifdef CONFIG_CMDLINE_OVERRIDE
- strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-#else
- if (builtin_cmdline[0]) {
- /* append boot loader cmdline to builtin */
- strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
- strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
- strscpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
- }
-#endif
-#endif
-
- strscpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
+ cmdline_prepare(command_line, BUILTIN_CMDLINE, boot_command_line);
*cmdline_p = command_line;
/*
--
2.35.1
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteFrom: Borislav Petkov <bp@suse.de>
Date: Tue, 15 Nov 2022 19:30:09 +0100
Use cmdline_prepare() in the compressed stage so that builtin
command line (CONFIG_CMDLINE_BOOL) and overridden command line
(CONFIG_CMDLINE_OVERRIDE) strings are visible in the compressed kernel
too.
Use case being, supplying earlyprintk via a compile-time option for
booting on systems with broken UEFI command line arguments via EFISTUB.
Reported-by: Evgeniy Baskov <baskov@ispras.ru>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
arch/x86/boot/compressed/misc.c | 7 +++++++
arch/x86/include/asm/shared/cmdline.h | 20 ++++++++++++++++----
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index cf690d8712f4..b1077b2fdba6 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -18,6 +18,9 @@
#include "../string.h"
#include "../voffset.h"
#include <asm/bootparam_utils.h>
+#include <asm/shared/cmdline.h>
+
+extern unsigned long get_cmd_line_ptr(void);
/*
* WARNING!!
@@ -355,6 +358,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
{
const unsigned long kernel_total_size = VO__end - VO__text;
unsigned long virt_addr = LOAD_PHYSICAL_ADDR;
+ char *cmdline = (char *)get_cmd_line_ptr();
unsigned long needed_size;
/* Retain x86 boot parameters pointer passed from startup_32/64. */
@@ -365,6 +369,9 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
sanitize_boot_params(boot_params);
+ /* Destination and boot command line are the same */
+ cmdline_prepare(cmdline, BUILTIN_CMDLINE, cmdline);
+
if (boot_params->screen_info.orig_video_mode == 7) {
vidmem = (char *) 0xb0000;
vidport = 0x3b4;
diff --git a/arch/x86/include/asm/shared/cmdline.h b/arch/x86/include/asm/shared/cmdline.h
index e09c06338567..8a7d8f579575 100644
--- a/arch/x86/include/asm/shared/cmdline.h
+++ b/arch/x86/include/asm/shared/cmdline.h
@@ -8,6 +8,15 @@
#define BUILTIN_CMDLINE ""
#endif
+#define _SETUP
+#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
+#undef _SETUP
+
+/*
+ * Add @boot_command_line to @dst only if it is not in @dst already. The compressed kernel
+ * has the command line pointer in setup_header.cmd_line_ptr which is set by the boot
+ * loader so @boot_command_line == @dst there, see the call in compressed/misc.c
+ */
static inline void cmdline_prepare(char *dst,
const char *builtin_cmdline,
char *boot_command_line)
@@ -20,15 +29,18 @@ static inline void cmdline_prepare(char *dst,
/* Add builtin cmdline */
strlcat(dst, builtin_cmdline, COMMAND_LINE_SIZE);
strlcat(dst, " ", COMMAND_LINE_SIZE);
- /* Add boot cmdline */
- strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
+
+ if (dst != boot_command_line)
+ strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
}
} else {
- strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
+ if (dst != boot_command_line)
+ strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
}
/* Copy back into boot command line, see setup_command_line() */
- strscpy(boot_command_line, dst, COMMAND_LINE_SIZE);
+ if (dst != boot_command_line)
+ strscpy(boot_command_line, dst, COMMAND_LINE_SIZE);
}
#endif /* _ASM_X86_SHARED_CMDLINE_H */
--
2.35.1
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteOn 2022-11-15 22:00, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> Date: Tue, 15 Nov 2022 19:30:09 +0100
>
> Use cmdline_prepare() in the compressed stage so that builtin
> command line (CONFIG_CMDLINE_BOOL) and overridden command line
> (CONFIG_CMDLINE_OVERRIDE) strings are visible in the compressed kernel
> too.
>
> Use case being, supplying earlyprintk via a compile-time option for
> booting on systems with broken UEFI command line arguments via EFISTUB.
>
> Reported-by: Evgeniy Baskov <baskov@ispras.ru>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
> arch/x86/boot/compressed/misc.c | 7 +++++++
> arch/x86/include/asm/shared/cmdline.h | 20 ++++++++++++++++----
> 2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/misc.c
> b/arch/x86/boot/compressed/misc.c
> index cf690d8712f4..b1077b2fdba6 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -18,6 +18,9 @@
> #include "../string.h"
> #include "../voffset.h"
> #include <asm/bootparam_utils.h>
> +#include <asm/shared/cmdline.h>
> +
> +extern unsigned long get_cmd_line_ptr(void);
>
> /*
> * WARNING!!
> @@ -355,6 +358,7 @@ asmlinkage __visible void *extract_kernel(void
> *rmode, memptr heap,
> {
> const unsigned long kernel_total_size = VO__end - VO__text;
> unsigned long virt_addr = LOAD_PHYSICAL_ADDR;
> + char *cmdline = (char *)get_cmd_line_ptr();
> unsigned long needed_size;
>
> /* Retain x86 boot parameters pointer passed from startup_32/64. */
> @@ -365,6 +369,9 @@ asmlinkage __visible void *extract_kernel(void
> *rmode, memptr heap,
>
> sanitize_boot_params(boot_params);
>
> + /* Destination and boot command line are the same */
> + cmdline_prepare(cmdline, BUILTIN_CMDLINE, cmdline);
> +
> if (boot_params->screen_info.orig_video_mode == 7) {
> vidmem = (char *) 0xb0000;
> vidport = 0x3b4;
> diff --git a/arch/x86/include/asm/shared/cmdline.h
> b/arch/x86/include/asm/shared/cmdline.h
> index e09c06338567..8a7d8f579575 100644
> --- a/arch/x86/include/asm/shared/cmdline.h
> +++ b/arch/x86/include/asm/shared/cmdline.h
> @@ -8,6 +8,15 @@
> #define BUILTIN_CMDLINE ""
> #endif
>
> +#define _SETUP
> +#include <asm/setup.h> /* For COMMAND_LINE_SIZE */
> +#undef _SETUP
> +
> +/*
> + * Add @boot_command_line to @dst only if it is not in @dst already.
> The compressed kernel
> + * has the command line pointer in setup_header.cmd_line_ptr which is
> set by the boot
> + * loader so @boot_command_line == @dst there, see the call in
> compressed/misc.c
> + */
> static inline void cmdline_prepare(char *dst,
> const char *builtin_cmdline,
> char *boot_command_line)
> @@ -20,15 +29,18 @@ static inline void cmdline_prepare(char *dst,
> /* Add builtin cmdline */
> strlcat(dst, builtin_cmdline, COMMAND_LINE_SIZE);
> strlcat(dst, " ", COMMAND_LINE_SIZE);
> - /* Add boot cmdline */
> - strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
> +
> + if (dst != boot_command_line)
> + strlcat(dst, boot_command_line, COMMAND_LINE_SIZE);
> }
> } else {
> - strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
> + if (dst != boot_command_line)
> + strscpy(dst, boot_command_line, COMMAND_LINE_SIZE);
> }
>
> /* Copy back into boot command line, see setup_command_line() */
> - strscpy(boot_command_line, dst, COMMAND_LINE_SIZE);
> + if (dst != boot_command_line)
> + strscpy(boot_command_line, dst, COMMAND_LINE_SIZE);
> }
>
> #endif /* _ASM_X86_SHARED_CMDLINE_H */
> --
> 2.35.1
Thanks for your time!
This patch has a few problems I was trying to avoid though:
* It has different behavior for dst == boot_command_line and dst !=
boot_command_line, since the order of parameters is different.
* It appends space at the end of command line, not as a separator.
* It also modifies boot_command_line in compressed kernel and it causes
builtin command line to be appended twice.
First two problems would be fixed if compressed kernel used separate
buffer for modified cmdline like setup.c does. This also would simplify
the helper a bit and is required to fix the third problem.
The third problem was the reason I did not include the last strscpy() in
the helper. I still don't think it should be a part of the command line
preparation logic... If the code needs a copy for parse_early_param(),
it is related more to the parse_early_param() call, than to
cmdline_prepare().
Should I maybe make another iteration by removing lazy cmdline
initialization in compressed kernel and adding more comments?
I don't see though how the last strscpy() could cleanly fit into
cmdline_prepare().
Thanks,
Evgeniy Baskov
© 2016 - 2026 Red Hat, Inc.