[PATCH 0/8] bootconfig: Update for the recent reports

[PATCH 0/8] bootconfig: Update for the recent reports

[Masami Hiramatsu]

Hello,

Here are some patches to update bootconfig. There are
several implementation changes and syntax fix/updates.

As we discussed in the previous thread, modifies bootconfig.

 - [1/8] Make CONFIG_BOOT_CONFIG=n by default
 - [2/8] Add magic word for bootconfig
 - [8/8] Print array as multiple commands for
         legacy command line

With the magic word, we can identify bootconfig without
checksum, also if we find a wrong bootconfig, there is no
reason to suppress the error message anymore.

 - [3/8] Remove unneeded error message silencer
 - [4/8] Remove unneeded checksum

In addition, I found some issues on bootconfig syntax, so
fix it. (these also include testcase update)

 - [5/8] Reject if subkey and value has same parent
 - [6/8] Overwrite value on same key by default
 - [7/8] Add append value operator ("+=")

To update synchronously, each patch includes code, test
and documentation. If it is better to split (especially
documentation). please let me know.

TODO: support kernel builtin bootconfig. Should we reuse
firmware builtin loader? But it seems we just need a Makefile.

Thank you,

---

Masami Hiramatsu (8):
      bootconfig: Set CONFIG_BOOT_CONFIG=n by default
      bootconfig: Add bootconfig magic word for indicating bootconfig explicitly
      tools/bootconfig: Remove unneeded error message silencer
      bootconfig: Remove unneeded checksum
      bootconfig: Reject subkey and value on same parent key
      bootconfig: Overwrite value on same key by default
      bootconfig: Add append value operator support
      bootconfig: Print array as multiple commands for legacy command line


 Documentation/admin-guide/bootconfig.rst     |   37 +++++++++++-
 include/linux/bootconfig.h                   |    3 +
 init/Kconfig                                 |    2 -
 init/main.c                                  |   54 +++++++-----------
 kernel/trace/Kconfig                         |    3 +
 lib/bootconfig.c                             |   44 +++++++++++----
 tools/bootconfig/include/linux/printk.h      |    5 --
 tools/bootconfig/main.c                      |   77 +++++++++++---------------
 tools/bootconfig/samples/bad-mixed-kv1.bconf |    3 +
 tools/bootconfig/samples/bad-mixed-kv2.bconf |    3 +
 tools/bootconfig/test-bootconfig.sh          |   31 +++++++++-
 11 files changed, 164 insertions(+), 98 deletions(-)
 create mode 100644 tools/bootconfig/samples/bad-mixed-kv1.bconf
 create mode 100644 tools/bootconfig/samples/bad-mixed-kv2.bconf

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

[PATCH 1/8] bootconfig: Set CONFIG_BOOT_CONFIG=n by default

[Masami Hiramatsu]

Set CONFIG_BOOT_CONFIG=n by default. This also warns
user if CONFIG_BOOT_CONFIG=n but "bootconfig" is given
in the kernel command line.

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 init/Kconfig         |    2 +-
 init/main.c          |    8 ++++++++
 kernel/trace/Kconfig |    3 ++-
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 4a672c6629d0..824f3763260d 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1218,7 +1218,7 @@ endif
 config BOOT_CONFIG
 	bool "Boot config support"
 	depends on BLK_DEV_INITRD
-	default y
+	default n
 	help
 	  Extra boot config allows system admin to pass a config file as
 	  complemental extension of kernel cmdline when booting.
diff --git a/init/main.c b/init/main.c
index 59248717c925..680ff7123705 100644
--- a/init/main.c
+++ b/init/main.c
@@ -418,6 +418,14 @@ static void __init setup_boot_config(const char *cmdline)
 }
 #else
 #define setup_boot_config(cmdline)	do { } while (0)
+
+static int __init warn_bootconfig(char *str)
+{
+	pr_err("WARNING: 'bootconfig' found on the kernel command line but CONFIG_BOOTCONFIG is not set.\n");
+	return 0;
+}
+early_param("bootconfig", warn_bootconfig);
+
 #endif
 
 /* Change NUL term back to "=", to make "param" the whole string. */
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 91e885194dbc..795c3e02d3f1 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -143,7 +143,8 @@ if FTRACE
 
 config BOOTTIME_TRACING
 	bool "Boot-time Tracing support"
-	depends on BOOT_CONFIG && TRACING
+	depends on TRACING
+	select BOOT_CONFIG
 	default y
 	help
 	  Enable developer to setup ftrace subsystem via supplemental

[PATCH 2/8] bootconfig: Add bootconfig magic word for indicating bootconfig explicitly

[Masami Hiramatsu]

Add bootconfig magic word to the end of bootconfig on initrd
image for indicating explicitly the bootconfig is there.
Also tools/bootconfig treats wrong size or wrong checksum or
parse error as an error, because if there is a bootconfig magic
word, there must be a bootconfig.

The bootconfig magic word is "#BOOTCONFIG\n", 12 bytes word.
Thus the block image of the initrd file with bootconfig is
as follows.

[Initrd][bootconfig][size][csum][#BOOTCONFIG\n]

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Documentation/admin-guide/bootconfig.rst |   10 +++++--
 include/linux/bootconfig.h               |    3 ++
 init/main.c                              |    6 +++-
 tools/bootconfig/main.c                  |   43 ++++++++++++++++++++++--------
 tools/bootconfig/test-bootconfig.sh      |    2 +
 5 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
index b342a6796392..5e7609936507 100644
--- a/Documentation/admin-guide/bootconfig.rst
+++ b/Documentation/admin-guide/bootconfig.rst
@@ -102,9 +102,13 @@ Boot Kernel With a Boot Config
 ==============================
 
 Since the boot configuration file is loaded with initrd, it will be added
-to the end of the initrd (initramfs) image file. The Linux kernel decodes
-the last part of the initrd image in memory to get the boot configuration
-data.
+to the end of the initrd (initramfs) image file with size, checksum and
+12-byte magic word as below.
+
+[initrd][bootconfig][size(u32)][checksum(u32)][#BOOTCONFIG\n]
+
+The Linux kernel decodes the last part of the initrd image in memory to
+get the boot configuration data.
 Because of this "piggyback" method, there is no need to change or
 update the boot loader and the kernel image itself.
 
diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
index 7e18c939663e..d11e183fcb54 100644
--- a/include/linux/bootconfig.h
+++ b/include/linux/bootconfig.h
@@ -10,6 +10,9 @@
 #include <linux/kernel.h>
 #include <linux/types.h>
 
+#define BOOTCONFIG_MAGIC	"#BOOTCONFIG\n"
+#define BOOTCONFIG_MAGIC_LEN	12
+
 /* XBC tree node */
 struct xbc_node {
 	u16 next;
diff --git a/init/main.c b/init/main.c
index 680ff7123705..671dd355fe9b 100644
--- a/init/main.c
+++ b/init/main.c
@@ -374,7 +374,11 @@ static void __init setup_boot_config(const char *cmdline)
 	if (!initrd_end)
 		goto not_found;
 
-	hdr = (u32 *)(initrd_end - 8);
+	data = (char *)initrd_end - BOOTCONFIG_MAGIC_LEN;
+	if (memcmp(data, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN))
+		goto not_found;
+
+	hdr = (u32 *)(data - 8);
 	size = hdr[0];
 	csum = hdr[1];
 
diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index e18eeb070562..742271f019a9 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -131,15 +131,26 @@ int load_xbc_from_initrd(int fd, char **buf)
 	struct stat stat;
 	int ret;
 	u32 size = 0, csum = 0, rcsum;
+	char magic[BOOTCONFIG_MAGIC_LEN];
 
 	ret = fstat(fd, &stat);
 	if (ret < 0)
 		return -errno;
 
-	if (stat.st_size < 8)
+	if (stat.st_size < 8 + BOOTCONFIG_MAGIC_LEN)
 		return 0;
 
-	if (lseek(fd, -8, SEEK_END) < 0) {
+	if (lseek(fd, -BOOTCONFIG_MAGIC_LEN, SEEK_END) < 0) {
+		pr_err("Failed to lseek: %d\n", -errno);
+		return -errno;
+	}
+	if (read(fd, magic, BOOTCONFIG_MAGIC_LEN) < 0)
+		return -errno;
+	/* Check the bootconfig magic bytes */
+	if (memcmp(magic, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN) != 0)
+		return 0;
+
+	if (lseek(fd, -(8 + BOOTCONFIG_MAGIC_LEN), SEEK_END) < 0) {
 		pr_err("Failed to lseek: %d\n", -errno);
 		return -errno;
 	}
@@ -150,11 +161,14 @@ int load_xbc_from_initrd(int fd, char **buf)
 	if (read(fd, &csum, sizeof(u32)) < 0)
 		return -errno;
 
-	/* Wrong size, maybe no boot config here */
-	if (stat.st_size < size + 8)
-		return 0;
+	/* Wrong size error  */
+	if (stat.st_size < size + 8 + BOOTCONFIG_MAGIC_LEN) {
+		pr_err("bootconfig size is too big\n");
+		return -E2BIG;
+	}
 
-	if (lseek(fd, stat.st_size - 8 - size, SEEK_SET) < 0) {
+	if (lseek(fd, stat.st_size - (size + 8 + BOOTCONFIG_MAGIC_LEN),
+		  SEEK_SET) < 0) {
 		pr_err("Failed to lseek: %d\n", -errno);
 		return -errno;
 	}
@@ -163,17 +177,17 @@ int load_xbc_from_initrd(int fd, char **buf)
 	if (ret < 0)
 		return ret;
 
-	/* Wrong Checksum, maybe no boot config here */
+	/* Wrong Checksum */
 	rcsum = checksum((unsigned char *)*buf, size);
 	if (csum != rcsum) {
 		pr_err("checksum error: %d != %d\n", csum, rcsum);
-		return 0;
+		return -EINVAL;
 	}
 
 	ret = xbc_init(*buf);
-	/* Wrong data, maybe no boot config here */
+	/* Wrong data */
 	if (ret < 0)
-		return 0;
+		return ret;
 
 	return size;
 }
@@ -226,7 +240,8 @@ int delete_xbc(const char *path)
 	} else if (size > 0) {
 		ret = fstat(fd, &stat);
 		if (!ret)
-			ret = ftruncate(fd, stat.st_size - size - 8);
+			ret = ftruncate(fd, stat.st_size
+					- size - 8 - BOOTCONFIG_MAGIC_LEN);
 		if (ret)
 			ret = -errno;
 	} /* Ignore if there is no boot config in initrd */
@@ -295,6 +310,12 @@ int apply_xbc(const char *path, const char *xbc_path)
 		pr_err("Failed to apply a boot config: %d\n", ret);
 		return ret;
 	}
+	/* Write a magic word of the bootconfig */
+	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
+	if (ret < 0) {
+		pr_err("Failed to apply a boot config magic: %d\n", ret);
+		return ret;
+	}
 	close(fd);
 	free(data);
 
diff --git a/tools/bootconfig/test-bootconfig.sh b/tools/bootconfig/test-bootconfig.sh
index 1de06de328e2..adafb7c50940 100755
--- a/tools/bootconfig/test-bootconfig.sh
+++ b/tools/bootconfig/test-bootconfig.sh
@@ -49,7 +49,7 @@ xpass $BOOTCONF -a $TEMPCONF $INITRD
 new_size=$(stat -c %s $INITRD)
 
 echo "File size check"
-xpass test $new_size -eq $(expr $bconf_size + $initrd_size + 9)
+xpass test $new_size -eq $(expr $bconf_size + $initrd_size + 9 + 12)
 
 echo "Apply command repeat test"
 xpass $BOOTCONF -a $TEMPCONF $INITRD

[PATCH 3/8] tools/bootconfig: Remove unneeded error message silencer

[Masami Hiramatsu]

Remove error message silent knob, we don't need it anymore
because we can check if there is a bootconfig by checking
the magic word.
If there is a magic word, but failed to load a bootconfig
from initrd, there is a real problem.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/bootconfig/include/linux/printk.h |    5 +----
 tools/bootconfig/main.c                 |    8 --------
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/tools/bootconfig/include/linux/printk.h b/tools/bootconfig/include/linux/printk.h
index e978a63d3222..036e667596eb 100644
--- a/tools/bootconfig/include/linux/printk.h
+++ b/tools/bootconfig/include/linux/printk.h
@@ -4,10 +4,7 @@
 
 #include <stdio.h>
 
-/* controllable printf */
-extern int pr_output;
-#define printk(fmt, ...)	\
-	(pr_output ? printf(fmt, ##__VA_ARGS__) : 0)
+#define printk(fmt, ...) printf(fmt, ##__VA_ARGS__)
 
 #define pr_err printk
 #define pr_warn	printk
diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 742271f019a9..a9b97814d1a9 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -14,8 +14,6 @@
 #include <linux/kernel.h>
 #include <linux/bootconfig.h>
 
-int pr_output = 1;
-
 static int xbc_show_array(struct xbc_node *node)
 {
 	const char *val;
@@ -227,13 +225,7 @@ int delete_xbc(const char *path)
 		return -errno;
 	}
 
-	/*
-	 * Suppress error messages in xbc_init() because it can be just a
-	 * data which concidentally matches the size and checksum footer.
-	 */
-	pr_output = 0;
 	size = load_xbc_from_initrd(fd, &buf);
-	pr_output = 1;
 	if (size < 0) {
 		ret = size;
 		pr_err("Failed to load a boot config from initrd: %d\n", ret);

[PATCH 4/8] bootconfig: Remove unneeded checksum

[Masami Hiramatsu]

Remove checksum of bootconfig because we already use a magic
word to identify bootconfig. This checksum was used for
checking whether there is a bootconfig at the end of initrd
or not. Since we have a bootconfig magic word to identify
the bootconfig data, we do not this checksum anymore.

Thus the block image of the initrd file with bootconfig is
as follows.

[initrd][bootconfig][size(u32)][#BOOTCONFIG\n]

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Documentation/admin-guide/bootconfig.rst |    6 ++--
 init/main.c                              |   20 +-------------
 tools/bootconfig/main.c                  |   42 ++++++------------------------
 tools/bootconfig/test-bootconfig.sh      |    2 +
 4 files changed, 15 insertions(+), 55 deletions(-)

diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
index 5e7609936507..48675052c963 100644
--- a/Documentation/admin-guide/bootconfig.rst
+++ b/Documentation/admin-guide/bootconfig.rst
@@ -102,10 +102,10 @@ Boot Kernel With a Boot Config
 ==============================
 
 Since the boot configuration file is loaded with initrd, it will be added
-to the end of the initrd (initramfs) image file with size, checksum and
-12-byte magic word as below.
+to the end of the initrd (initramfs) image file with 4-byte size and 12-byte
+magic word as below.
 
-[initrd][bootconfig][size(u32)][checksum(u32)][#BOOTCONFIG\n]
+[initrd][bootconfig][size][#BOOTCONFIG\n]
 
 The Linux kernel decodes the last part of the initrd image in memory to
 get the boot configuration data.
diff --git a/init/main.c b/init/main.c
index 671dd355fe9b..7c4ca805562a 100644
--- a/init/main.c
+++ b/init/main.c
@@ -335,16 +335,6 @@ static char * __init xbc_make_cmdline(const char *key)
 	return new_cmdline;
 }
 
-u32 boot_config_checksum(unsigned char *p, u32 size)
-{
-	u32 ret = 0;
-
-	while (size--)
-		ret += *p++;
-
-	return ret;
-}
-
 static int __init bootconfig_params(char *param, char *val,
 				    const char *unused, void *arg)
 {
@@ -359,7 +349,7 @@ static int __init bootconfig_params(char *param, char *val,
 static void __init setup_boot_config(const char *cmdline)
 {
 	static char tmp_cmdline[COMMAND_LINE_SIZE] __initdata;
-	u32 size, csum;
+	u32 size;
 	char *data, *copy;
 	u32 *hdr;
 	int ret;
@@ -378,9 +368,8 @@ static void __init setup_boot_config(const char *cmdline)
 	if (memcmp(data, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN))
 		goto not_found;
 
-	hdr = (u32 *)(data - 8);
+	hdr = (u32 *)(data - sizeof(u32));
 	size = hdr[0];
-	csum = hdr[1];
 
 	if (size >= XBC_DATA_MAX) {
 		pr_err("bootconfig size %d greater than max size %d\n",
@@ -392,11 +381,6 @@ static void __init setup_boot_config(const char *cmdline)
 	if ((unsigned long)data < initrd_start)
 		goto not_found;
 
-	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) {
 		pr_err("Failed to allocate memory for bootconfig\n");
diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index a9b97814d1a9..01acb86c6273 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -75,17 +75,6 @@ static void xbc_show_compact_tree(void)
 	}
 }
 
-/* Simple real checksum */
-int checksum(unsigned char *buf, int len)
-{
-	int i, sum = 0;
-
-	for (i = 0; i < len; i++)
-		sum += buf[i];
-
-	return sum;
-}
-
 #define PAGE_SIZE	4096
 
 int load_xbc_fd(int fd, char **buf, int size)
@@ -128,14 +117,14 @@ int load_xbc_from_initrd(int fd, char **buf)
 {
 	struct stat stat;
 	int ret;
-	u32 size = 0, csum = 0, rcsum;
+	u32 size = 0;
 	char magic[BOOTCONFIG_MAGIC_LEN];
 
 	ret = fstat(fd, &stat);
 	if (ret < 0)
 		return -errno;
 
-	if (stat.st_size < 8 + BOOTCONFIG_MAGIC_LEN)
+	if (stat.st_size < 4 + BOOTCONFIG_MAGIC_LEN)
 		return 0;
 
 	if (lseek(fd, -BOOTCONFIG_MAGIC_LEN, SEEK_END) < 0) {
@@ -148,7 +137,7 @@ int load_xbc_from_initrd(int fd, char **buf)
 	if (memcmp(magic, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN) != 0)
 		return 0;
 
-	if (lseek(fd, -(8 + BOOTCONFIG_MAGIC_LEN), SEEK_END) < 0) {
+	if (lseek(fd, -(4 + BOOTCONFIG_MAGIC_LEN), SEEK_END) < 0) {
 		pr_err("Failed to lseek: %d\n", -errno);
 		return -errno;
 	}
@@ -156,16 +145,13 @@ int load_xbc_from_initrd(int fd, char **buf)
 	if (read(fd, &size, sizeof(u32)) < 0)
 		return -errno;
 
-	if (read(fd, &csum, sizeof(u32)) < 0)
-		return -errno;
-
 	/* Wrong size error  */
-	if (stat.st_size < size + 8 + BOOTCONFIG_MAGIC_LEN) {
+	if (stat.st_size < size + 4 + BOOTCONFIG_MAGIC_LEN) {
 		pr_err("bootconfig size is too big\n");
 		return -E2BIG;
 	}
 
-	if (lseek(fd, stat.st_size - (size + 8 + BOOTCONFIG_MAGIC_LEN),
+	if (lseek(fd, stat.st_size - (size + 4 + BOOTCONFIG_MAGIC_LEN),
 		  SEEK_SET) < 0) {
 		pr_err("Failed to lseek: %d\n", -errno);
 		return -errno;
@@ -175,13 +161,6 @@ int load_xbc_from_initrd(int fd, char **buf)
 	if (ret < 0)
 		return ret;
 
-	/* Wrong Checksum */
-	rcsum = checksum((unsigned char *)*buf, size);
-	if (csum != rcsum) {
-		pr_err("checksum error: %d != %d\n", csum, rcsum);
-		return -EINVAL;
-	}
-
 	ret = xbc_init(*buf);
 	/* Wrong data */
 	if (ret < 0)
@@ -233,7 +212,7 @@ int delete_xbc(const char *path)
 		ret = fstat(fd, &stat);
 		if (!ret)
 			ret = ftruncate(fd, stat.st_size
-					- size - 8 - BOOTCONFIG_MAGIC_LEN);
+					- size - 4 - BOOTCONFIG_MAGIC_LEN);
 		if (ret)
 			ret = -errno;
 	} /* Ignore if there is no boot config in initrd */
@@ -246,7 +225,7 @@ int delete_xbc(const char *path)
 
 int apply_xbc(const char *path, const char *xbc_path)
 {
-	u32 size, csum;
+	u32 size;
 	char *buf, *data;
 	int ret, fd;
 
@@ -256,15 +235,13 @@ int apply_xbc(const char *path, const char *xbc_path)
 		return ret;
 	}
 	size = strlen(buf) + 1;
-	csum = checksum((unsigned char *)buf, size);
 
 	/* Prepare xbc_path data */
-	data = malloc(size + 8);
+	data = malloc(size + 4);
 	if (!data)
 		return -ENOMEM;
 	strcpy(data, buf);
 	*(u32 *)(data + size) = size;
-	*(u32 *)(data + size + 4) = csum;
 
 	/* Check the data format */
 	ret = xbc_init(buf);
@@ -277,7 +254,6 @@ int apply_xbc(const char *path, const char *xbc_path)
 	printf("Apply %s to %s\n", xbc_path, path);
 	printf("\tNumber of nodes: %d\n", ret);
 	printf("\tSize: %u bytes\n", (unsigned int)size);
-	printf("\tChecksum: %d\n", (unsigned int)csum);
 
 	/* TODO: Check the options by schema */
 	xbc_destroy_all();
@@ -297,7 +273,7 @@ int apply_xbc(const char *path, const char *xbc_path)
 		return fd;
 	}
 	/* TODO: Ensure the @path is initramfs/initrd image */
-	ret = write(fd, data, size + 8);
+	ret = write(fd, data, size + 4);
 	if (ret < 0) {
 		pr_err("Failed to apply a boot config: %d\n", ret);
 		return ret;
diff --git a/tools/bootconfig/test-bootconfig.sh b/tools/bootconfig/test-bootconfig.sh
index adafb7c50940..c5965eff62c5 100755
--- a/tools/bootconfig/test-bootconfig.sh
+++ b/tools/bootconfig/test-bootconfig.sh
@@ -49,7 +49,7 @@ xpass $BOOTCONF -a $TEMPCONF $INITRD
 new_size=$(stat -c %s $INITRD)
 
 echo "File size check"
-xpass test $new_size -eq $(expr $bconf_size + $initrd_size + 9 + 12)
+xpass test $new_size -eq $(expr $bconf_size + $initrd_size + 5 + 12)
 
 echo "Apply command repeat test"
 xpass $BOOTCONF -a $TEMPCONF $INITRD

[PATCH 5/8] bootconfig: Reject subkey and value on same parent key

[Masami Hiramatsu]

Reject if a value node is mixed with subkey node on same
parent key node.

A value node can not co-exist with subkey node under some key
node, e.g.

key = value
key.subkey = another-value

This is not be allowed because bootconfig API is not designed
to handle such case.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Documentation/admin-guide/bootconfig.rst     |    7 +++++++
 lib/bootconfig.c                             |   16 ++++++++++++----
 tools/bootconfig/samples/bad-mixed-kv1.bconf |    3 +++
 tools/bootconfig/samples/bad-mixed-kv2.bconf |    3 +++
 4 files changed, 25 insertions(+), 4 deletions(-)
 create mode 100644 tools/bootconfig/samples/bad-mixed-kv1.bconf
 create mode 100644 tools/bootconfig/samples/bad-mixed-kv2.bconf

diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
index 48675052c963..4a106584eb21 100644
--- a/Documentation/admin-guide/bootconfig.rst
+++ b/Documentation/admin-guide/bootconfig.rst
@@ -62,6 +62,13 @@ Or more shorter, written as following::
 In both styles, same key words are automatically merged when parsing it
 at boot time. So you can append similar trees or key-values.
 
+Note that a sub-key and a value can not co-exist under a parent key.
+For example, following config is NOT allowed.::
+
+ foo = value1
+ foo.bar = value2 # !ERROR! subkey "bar" and value "value1" can NOT co-exist
+
+
 Comments
 --------
 
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 3ea601a2eba5..54ac623ca781 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -533,7 +533,7 @@ struct xbc_node *find_match_node(struct xbc_node *node, char *k)
 
 static int __init __xbc_add_key(char *k)
 {
-	struct xbc_node *node;
+	struct xbc_node *node, *child;
 
 	if (!xbc_valid_keyword(k))
 		return xbc_parse_error("Invalid keyword", k);
@@ -543,8 +543,12 @@ static int __init __xbc_add_key(char *k)
 
 	if (!last_parent)	/* the first level */
 		node = find_match_node(xbc_nodes, k);
-	else
-		node = find_match_node(xbc_node_get_child(last_parent), k);
+	else {
+		child = xbc_node_get_child(last_parent);
+		if (child && xbc_node_is_value(child))
+			return xbc_parse_error("Subkey is mixed with value", k);
+		node = find_match_node(child, k);
+	}
 
 	if (node)
 		last_parent = node;
@@ -577,7 +581,7 @@ static int __init __xbc_parse_keys(char *k)
 static int __init xbc_parse_kv(char **k, char *v)
 {
 	struct xbc_node *prev_parent = last_parent;
-	struct xbc_node *node;
+	struct xbc_node *node, *child;
 	char *next;
 	int c, ret;
 
@@ -585,6 +589,10 @@ static int __init xbc_parse_kv(char **k, char *v)
 	if (ret)
 		return ret;
 
+	child = xbc_node_get_child(last_parent);
+	if (child && xbc_node_is_key(child))
+		return xbc_parse_error("Value is mixed with subkey", v);
+
 	c = __xbc_parse_value(&v, &next);
 	if (c < 0)
 		return c;
diff --git a/tools/bootconfig/samples/bad-mixed-kv1.bconf b/tools/bootconfig/samples/bad-mixed-kv1.bconf
new file mode 100644
index 000000000000..1761547dd05c
--- /dev/null
+++ b/tools/bootconfig/samples/bad-mixed-kv1.bconf
@@ -0,0 +1,3 @@
+# value -> subkey pattern
+key = value
+key.subkey = another-value
diff --git a/tools/bootconfig/samples/bad-mixed-kv2.bconf b/tools/bootconfig/samples/bad-mixed-kv2.bconf
new file mode 100644
index 000000000000..6b32e0c3878c
--- /dev/null
+++ b/tools/bootconfig/samples/bad-mixed-kv2.bconf
@@ -0,0 +1,3 @@
+# subkey -> value pattern
+key.subkey = value
+key = another-value

[PATCH 6/8] bootconfig: Overwrite value on same key by default

[Masami Hiramatsu]

Currently, bootconfig does not overwrite existing value
on same key, but add new value to the tail of an array.
But this looks a bit confusing because similar syntax
configuration always overwrite the value by default.

This changes the behavior to overwrite value on same key.

For example, if there are 2 entries

  key = value
  ...
  key = value2

Then, the key is updated as below.

  key = value2

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Documentation/admin-guide/bootconfig.rst |   12 ++++++++++++
 lib/bootconfig.c                         |   18 ++++++++++++++----
 tools/bootconfig/test-bootconfig.sh      |   16 ++++++++++++++--
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
index 4a106584eb21..0c18e3f540e6 100644
--- a/Documentation/admin-guide/bootconfig.rst
+++ b/Documentation/admin-guide/bootconfig.rst
@@ -62,6 +62,18 @@ Or more shorter, written as following::
 In both styles, same key words are automatically merged when parsing it
 at boot time. So you can append similar trees or key-values.
 
+Same-key Values
+---------------
+
+If two or more values or arraies share a same-key, the latter one remains.
+For example,::
+
+ foo = bar, baz
+ foo = qux
+
+In this case ``bar`` and ``baz`` is overwritten by ``qux``, and ``foo = qux``
+remains.
+
 Note that a sub-key and a value can not co-exist under a parent key.
 For example, following config is NOT allowed.::
 
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 54ac623ca781..9a094162ea3e 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -578,10 +578,18 @@ static int __init __xbc_parse_keys(char *k)
 	return __xbc_add_key(k);
 }
 
+static void xbc_node_overwrite(struct xbc_node *node, char *v)
+{
+	unsigned long offset = v - xbc_data;
+
+	node->data = (u16)offset | XBC_VALUE;
+	node->next = 0;
+}
+
 static int __init xbc_parse_kv(char **k, char *v)
 {
 	struct xbc_node *prev_parent = last_parent;
-	struct xbc_node *node, *child;
+	struct xbc_node *child;
 	char *next;
 	int c, ret;
 
@@ -597,9 +605,11 @@ static int __init xbc_parse_kv(char **k, char *v)
 	if (c < 0)
 		return c;
 
-	node = xbc_add_sibling(v, XBC_VALUE);
-	if (!node)
-		return -ENOMEM;
+	if (!child) {
+		if (!xbc_add_sibling(v, XBC_VALUE))
+			return -ENOMEM;
+	} else
+		xbc_node_overwrite(child, v);
 
 	if (c == ',') {	/* Array */
 		c = xbc_parse_array(&next);
diff --git a/tools/bootconfig/test-bootconfig.sh b/tools/bootconfig/test-bootconfig.sh
index c5965eff62c5..f6948790c693 100755
--- a/tools/bootconfig/test-bootconfig.sh
+++ b/tools/bootconfig/test-bootconfig.sh
@@ -9,7 +9,7 @@ TEMPCONF=`mktemp temp-XXXX.bconf`
 NG=0
 
 cleanup() {
-  rm -f $INITRD $TEMPCONF
+  rm -f $INITRD $TEMPCONF $OUTFILE
   exit $NG
 }
 
@@ -71,7 +71,6 @@ printf " \0\0\0 \0\0\0" >> $INITRD
 $BOOTCONF -a $TEMPCONF $INITRD > $OUTFILE 2>&1
 xfail grep -i "failed" $OUTFILE
 xfail grep -i "error" $OUTFILE
-rm $OUTFILE
 
 echo "Max node number check"
 
@@ -96,6 +95,19 @@ truncate -s 32764 $TEMPCONF
 echo "\"" >> $TEMPCONF	# add 2 bytes + terminal ('\"\n\0')
 xpass $BOOTCONF -a $TEMPCONF $INITRD
 
+echo "Overwrite same-key values"
+cat > $TEMPCONF << EOF
+key = bar, baz
+key = qux
+EOF
+echo > $INITRD
+
+xpass $BOOTCONF -a $TEMPCONF $INITRD
+$BOOTCONF $INITRD > $OUTFILE
+xfail grep -q "bar" $OUTFILE
+xfail grep -q "baz" $OUTFILE
+xpass grep -q "qux" $OUTFILE
+
 echo "=== expected failure cases ==="
 for i in samples/bad-* ; do
   xfail $BOOTCONF -a $i $INITRD

[PATCH 7/8] bootconfig: Add append value operator support

[Masami Hiramatsu]

Add append value operator "+=" support to bootconfig syntax.
With this operator, user can add new value to the key as
an entry of array instead of overwriting.
For example,

  foo = bar
  ...
  foo += baz

Then the key "foo" has "bar" and "baz" values as an array.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Documentation/admin-guide/bootconfig.rst |    8 ++++++++
 lib/bootconfig.c                         |   14 ++++++++++----
 tools/bootconfig/test-bootconfig.sh      |   13 +++++++++++++
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
index 0c18e3f540e6..abe2432b8eec 100644
--- a/Documentation/admin-guide/bootconfig.rst
+++ b/Documentation/admin-guide/bootconfig.rst
@@ -74,6 +74,14 @@ For example,::
 In this case ``bar`` and ``baz`` is overwritten by ``qux``, and ``foo = qux``
 remains.
 
+If you want to append the value to existing key as an array member,
+you can use ``+=`` operator. For example::
+
+ foo = bar, baz
+ foo += qux
+
+In this case, the key ``foo`` has ``bar``, ``baz`` and ``qux``.
+
 Note that a sub-key and a value can not co-exist under a parent key.
 For example, following config is NOT allowed.::
 
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 9a094162ea3e..4afc768489cd 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -586,7 +586,7 @@ static void xbc_node_overwrite(struct xbc_node *node, char *v)
 	node->next = 0;
 }
 
-static int __init xbc_parse_kv(char **k, char *v)
+static int __init xbc_parse_kv(char **k, char *v, int op)
 {
 	struct xbc_node *prev_parent = last_parent;
 	struct xbc_node *child;
@@ -605,7 +605,7 @@ static int __init xbc_parse_kv(char **k, char *v)
 	if (c < 0)
 		return c;
 
-	if (!child) {
+	if (op == '+' || !child) {
 		if (!xbc_add_sibling(v, XBC_VALUE))
 			return -ENOMEM;
 	} else
@@ -781,7 +781,7 @@ int __init xbc_init(char *buf)
 
 	p = buf;
 	do {
-		q = strpbrk(p, "{}=;\n#");
+		q = strpbrk(p, "{}=+;\n#");
 		if (!q) {
 			p = skip_spaces(p);
 			if (*p != '\0')
@@ -792,8 +792,14 @@ int __init xbc_init(char *buf)
 		c = *q;
 		*q++ = '\0';
 		switch (c) {
+		case '+':
+			if (*q++ != '=') {
+				ret = xbc_parse_error("Wrong + operator", q - 2);
+				break;
+			}
+			/* Fall through */
 		case '=':
-			ret = xbc_parse_kv(&p, q);
+			ret = xbc_parse_kv(&p, q, c);
 			break;
 		case '{':
 			ret = xbc_open_brace(&p, q);
diff --git a/tools/bootconfig/test-bootconfig.sh b/tools/bootconfig/test-bootconfig.sh
index f6948790c693..04f65548ca1e 100755
--- a/tools/bootconfig/test-bootconfig.sh
+++ b/tools/bootconfig/test-bootconfig.sh
@@ -108,6 +108,19 @@ xfail grep -q "bar" $OUTFILE
 xfail grep -q "baz" $OUTFILE
 xpass grep -q "qux" $OUTFILE
 
+echo "Adding same-key values"
+cat > $TEMPCONF << EOF
+key = bar, baz
+key += qux
+EOF
+echo > $INITRD
+
+xpass $BOOTCONF -a $TEMPCONF $INITRD
+$BOOTCONF $INITRD > $OUTFILE
+xpass grep -q "bar" $OUTFILE
+xpass grep -q "baz" $OUTFILE
+xpass grep -q "qux" $OUTFILE
+
 echo "=== expected failure cases ==="
 for i in samples/bad-* ; do
   xfail $BOOTCONF -a $i $INITRD

[PATCH 8/8] bootconfig: Print array as multiple commands for legacy command line

[Masami Hiramatsu]

Print arraied values as multiple same options for legacy
kernel command line. With this rule, if the "kernel.*" and
"init.*" array entries in bootconfig are printed out as
multiple same options, e.g.

kernel {
 console = "ttyS0,115200"
 console += "tty0"
}

will be correctly converted to

console="ttyS0,115200" console="tty0"

in the kernel command line.

Reported-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 init/main.c |   22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/init/main.c b/init/main.c
index 7c4ca805562a..606c702f5fc8 100644
--- a/init/main.c
+++ b/init/main.c
@@ -268,7 +268,6 @@ static int __init xbc_snprint_cmdline(char *buf, size_t size,
 {
 	struct xbc_node *knode, *vnode;
 	char *end = buf + size;
-	char c = '\"';
 	const char *val;
 	int ret;
 
@@ -279,25 +278,20 @@ static int __init xbc_snprint_cmdline(char *buf, size_t size,
 			return ret;
 
 		vnode = xbc_node_get_child(knode);
-		ret = snprintf(buf, rest(buf, end), "%s%c", xbc_namebuf,
-				vnode ? '=' : ' ');
-		if (ret < 0)
-			return ret;
-		buf += ret;
-		if (!vnode)
+		if (!vnode) {
+			ret = snprintf(buf, rest(buf, end), "%s ", xbc_namebuf);
+			if (ret < 0)
+				return ret;
+			buf += ret;
 			continue;
-
-		c = '\"';
+		}
 		xbc_array_for_each_value(vnode, val) {
-			ret = snprintf(buf, rest(buf, end), "%c%s", c, val);
+			ret = snprintf(buf, rest(buf, end), "%s=\"%s\" ",
+				       xbc_namebuf, val);
 			if (ret < 0)
 				return ret;
 			buf += ret;
-			c = ',';
 		}
-		if (rest(buf, end) > 2)
-			strcpy(buf, "\" ");
-		buf += 2;
 	}
 
 	return buf - (end - size);

Re: [PATCH 1/8] bootconfig: Set CONFIG_BOOT_CONFIG=n by default

[Geert Uytterhoeven]

Hi Hiramatsu-san,

On Thu, Feb 20, 2020 at 8:26 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> Set CONFIG_BOOT_CONFIG=n by default. This also warns
> user if CONFIG_BOOT_CONFIG=n but "bootconfig" is given
> in the kernel command line.
>
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks for your patch!

> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1218,7 +1218,7 @@ endif
>  config BOOT_CONFIG
>         bool "Boot config support"
>         depends on BLK_DEV_INITRD
> -       default y
> +       default n

"default n" is the default, so you can just drop the line.

>         help
>           Extra boot config allows system admin to pass a config file as
>           complemental extension of kernel cmdline when booting.
> diff --git a/init/main.c b/init/main.c
> index 59248717c925..680ff7123705 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -418,6 +418,14 @@ static void __init setup_boot_config(const char *cmdline)
>  }
>  #else
>  #define setup_boot_config(cmdline)     do { } while (0)
> +
> +static int __init warn_bootconfig(char *str)
> +{
> +       pr_err("WARNING: 'bootconfig' found on the kernel command line but CONFIG_BOOTCONFIG is not set.\n");

pr_warn()?

> +       return 0;
> +}
> +early_param("bootconfig", warn_bootconfig);
> +
>  #endif

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/8] bootconfig: Set CONFIG_BOOT_CONFIG=n by default

[Masami Hiramatsu]

On Thu, 20 Feb 2020 09:16:56 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Hiramatsu-san,
> 
> On Thu, Feb 20, 2020 at 8:26 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > Set CONFIG_BOOT_CONFIG=n by default. This also warns
> > user if CONFIG_BOOT_CONFIG=n but "bootconfig" is given
> > in the kernel command line.
> >
> > Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Thanks for your patch!
> 
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -1218,7 +1218,7 @@ endif
> >  config BOOT_CONFIG
> >         bool "Boot config support"
> >         depends on BLK_DEV_INITRD
> > -       default y
> > +       default n
> 
> "default n" is the default, so you can just drop the line.

OK.

> 
> >         help
> >           Extra boot config allows system admin to pass a config file as
> >           complemental extension of kernel cmdline when booting.
> > diff --git a/init/main.c b/init/main.c
> > index 59248717c925..680ff7123705 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -418,6 +418,14 @@ static void __init setup_boot_config(const char *cmdline)
> >  }
> >  #else
> >  #define setup_boot_config(cmdline)     do { } while (0)
> > +
> > +static int __init warn_bootconfig(char *str)
> > +{
> > +       pr_err("WARNING: 'bootconfig' found on the kernel command line but CONFIG_BOOTCONFIG is not set.\n");
> 
> pr_warn()?

Yeah, agreed. 

OK, I'll update the patch.

Thank you!

> 
> > +       return 0;
> > +}
> > +early_param("bootconfig", warn_bootconfig);
> > +
> >  #endif
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds


-- 
Masami Hiramatsu <mhiramat@kernel.org>