tools/bootconfig: Align the bootconfig applied initrd

PATCH Superseded Masami Hiramatsu 2 participants 11 messages Nov 18, 2020 – Nov 19, 2020

Subsystems (2)

Documentation Extra boot config

lore

Share & tools

lore

Masami Hiramatsu

author maintainer

2020-11-18 15:35 UTC #0 lore raw

v4 cover tools/bootconfig: Align the bootconfig applied initrd

Hi,

This is the 4th version of the bootconfig tool update to align the total size of initrd + bootconfig to 4.

Previous version is here;

https://lkml.kernel.org/r/160560676899.144950.4148778261999789656.stgit@devnote2

This version adds a new errno after printf fix patch as [1/4].

To adjust the file size, the bootconfig tool adds padding null characters in between the boot configuration data and the footer.

The changing points are
- The bootconfig applied initrd image size is aligned to 4.
- To insert the padding null ('\0') bytes, the size in the footer can be bigger than the actual bootconfig file size.
- But the max size of the boot configuration file is same, because the max size doesn't include the last null characters.

In this series I keep 4 bytes aligned instead of longer size, because only I could found was that the grub might align the initrd filesize to 4, and U-Boot/EDK2 would not change it. So I couldn't say what is the best size.

Anyway, I updated the documentation too, which clearly says that the above changing points, and if the bootloader pass the wrong size, kernel will not find bootconfig from the initrd.

Thank you,

---

Masami Hiramatsu (4):
tools/bootconfig: Fix errno reference after printf() tools/bootconfig: Fix to check the write failure correctly tools/bootconfig: Align the bootconfig applied initrd image size to 4
docs: bootconfig: Update file format on initrd image

 tools/bootconfig/main.c             |  109 ++++++++++++++++++++++-------------
 tools/bootconfig/test-bootconfig.sh |    6 ++
 2 files changed, 75 insertions(+), 40 deletions(-)

--
Masami Hiramatsu (Linaro) <[email protected]>

Masami Hiramatsu

author maintainer

2020-11-18 15:35 UTC #1 lore raw

PATCH v4 1/4 tools/bootconfig: Fix errno reference after printf()

Fix not to refer the errno variable as the result of previous libc functions after printf() because printf() can change the errno.

Fixes 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")

Signed-off-by Masami Hiramatsu <[email protected]>

---
 tools/bootconfig/main.c |   52 ++++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

tools/bootconfig/main.c +29 ~~-23~~

index eb92027817a7..52eb2bbe8966 100644
@@ -147,6 +147,12 @@ static int load_xbc_file(const char *path, char **buf)
 	return ret;
 }
 
+static int pr_errno(const char *msg, int err)
+{
+	pr_err("%s: %d\n", msg, err);
+	return err;
+}
+
 static int load_xbc_from_initrd(int fd, char **buf)
 {
 	struct stat stat;

@@ -162,26 +168,24 @@ static int load_xbc_from_initrd(int fd, char **buf)
 	if (stat.st_size < 8 + BOOTCONFIG_MAGIC_LEN)
 		return 0;
 
-	if (lseek(fd, -BOOTCONFIG_MAGIC_LEN, SEEK_END) < 0) {
-		pr_err("Failed to lseek: %d\n", -errno);
-		return -errno;
-	}
+	if (lseek(fd, -BOOTCONFIG_MAGIC_LEN, SEEK_END) < 0)
+		return pr_errno("Failed to lseek for magic", -errno);
+
 	if (read(fd, magic, BOOTCONFIG_MAGIC_LEN) < 0)
-		return -errno;
+		return pr_errno("Failed to read", -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;
-	}
+	if (lseek(fd, -(8 + BOOTCONFIG_MAGIC_LEN), SEEK_END) < 0)
+		return pr_errno("Failed to lseek for size", -errno);
 
 	if (read(fd, &size, sizeof(u32)) < 0)
-		return -errno;
+		return pr_errno("Failed to read size", -errno);
 
 	if (read(fd, &csum, sizeof(u32)) < 0)
-		return -errno;
+		return pr_errno("Failed to read checksum", -errno);
 
 	/* Wrong size error  */
 	if (stat.st_size < size + 8 + BOOTCONFIG_MAGIC_LEN) {

@@ -190,10 +194,8 @@ static int load_xbc_from_initrd(int fd, char **buf)
 	}
 
 	if (lseek(fd, stat.st_size - (size + 8 + BOOTCONFIG_MAGIC_LEN),
-		  SEEK_SET) < 0) {
-		pr_err("Failed to lseek: %d\n", -errno);
-		return -errno;
-	}
+		  SEEK_SET) < 0)
+		return pr_errno("Failed to lseek", -errno);
 
 	ret = load_xbc_fd(fd, buf, size);
 	if (ret < 0)

@@ -262,14 +264,16 @@ static int show_xbc(const char *path, bool list)
 
 	ret = stat(path, &st);
 	if (ret < 0) {
-		pr_err("Failed to stat %s: %d\n", path, -errno);
-		return -errno;
+		ret = -errno;
+		pr_err("Failed to stat %s: %d\n", path, ret);
+		return ret;
 	}
 
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
-		pr_err("Failed to open initrd %s: %d\n", path, fd);
-		return -errno;
+		ret = -errno;
+		pr_err("Failed to open initrd %s: %d\n", path, ret);
+		return ret;
 	}
 
 	ret = load_xbc_from_initrd(fd, &buf);

@@ -307,8 +311,9 @@ static int delete_xbc(const char *path)
 
 	fd = open(path, O_RDWR);
 	if (fd < 0) {
-		pr_err("Failed to open initrd %s: %d\n", path, fd);
-		return -errno;
+		ret = -errno;
+		pr_err("Failed to open initrd %s: %d\n", path, ret);
+		return ret;
 	}
 
 	size = load_xbc_from_initrd(fd, &buf);

@@ -383,9 +388,10 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	/* Apply new one */
 	fd = open(path, O_RDWR | O_APPEND);
 	if (fd < 0) {
-		pr_err("Failed to open %s: %d\n", path, fd);
+		ret = -errno;
+		pr_err("Failed to open %s: %d\n", path, ret);
 		free(data);
-		return fd;
+		return ret;
 	}
 	/* TODO: Ensure the @path is initramfs/initrd image */
 	ret = write(fd, data, size + 8);

Masami Hiramatsu

author maintainer

2020-11-18 15:35 UTC #2 lore raw

PATCH v4 2/4 tools/bootconfig: Fix to check the write failure correctly

In reply to Masami Hiramatsu

Fix to check the write(2) failure including partial write correctly and try to rollback the partial write, because if there is no BOOTCONFIG_MAGIC string, we can not remove it.

Fixes 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Chen Yu <[email protected]>

---
 tools/bootconfig/main.c |   27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

tools/bootconfig/main.c +23 -4

index 52eb2bbe8966..905bfaefae35 100644
@@ -337,6 +337,7 @@ static int delete_xbc(const char *path)
 
 static int apply_xbc(const char *path, const char *xbc_path)
 {
+	struct stat stat;
 	u32 size, csum;
 	char *buf, *data;
 	int ret, fd;

@@ -394,16 +395,26 @@ static int apply_xbc(const char *path, const char *xbc_path)
 		return ret;
 	}
 	/* TODO: Ensure the @path is initramfs/initrd image */
+	if (fstat(fd, &stat) < 0) {
+		pr_err("Failed to get the size of %s\n", path);
+		goto out;
+	}
 	ret = write(fd, data, size + 8);
-	if (ret < 0) {
+	if (ret < size + 8) {
+		if (ret < 0)
+			ret = -errno;
 		pr_err("Failed to apply a boot config: %d\n", ret);
-		goto out;
+		if (ret < 0)
+			goto out;
+		goto out_rollback;
 	}
 	/* Write a magic word of the bootconfig */
 	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
-	if (ret < 0) {
+	if (ret < BOOTCONFIG_MAGIC_LEN) {
+		if (ret < 0)
+			ret = -errno;
 		pr_err("Failed to apply a boot config magic: %d\n", ret);
-		goto out;
+		goto out_rollback;
 	}
 	ret = 0;
 out:

@@ -411,6 +422,14 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	free(data);
 
 	return ret;
+
+out_rollback:
+	if (ftruncate(fd, stat.st_size) < 0) {
+		ret = -errno;
+		pr_err("Failed to rollback the write error: %d\n", ret);
+		pr_err("The initrd %s may be corrupted. Recommend to rebuild.\n", path);
+	}
+	goto out;
 }
 
 static int usage(void)

Steven Rostedt

2020-11-18 16:05 UTC #3 lore raw

Re: [PATCH v4 2/4] tools/bootconfig: Fix to check the write failure correctly

Quoted 56 lines · On Thu, 19 Nov 2020 00:35:35 +0900 Masami Hiramatsu <[email protected]> wrote:

Fix to check the write(2) failure including partial write correctly and try to rollback the partial write, because if there is no BOOTCONFIG_MAGIC string, we can not remove it.

Fixes 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Chen Yu <[email protected]>

---
 tools/bootconfig/main.c |   27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

tools/bootconfig/main.c +15 -4

index 52eb2bbe8966..905bfaefae35 100644
@@ -337,6 +337,7 @@ static int delete_xbc(const char *path)
 
 static int apply_xbc(const char *path, const char *xbc_path)
 {
+	struct stat stat;
 	u32 size, csum;
 	char *buf, *data;
 	int ret, fd;

@@ -394,16 +395,26 @@ static int apply_xbc(const char *path, const char *xbc_path)
 		return ret;
 	}
 	/* TODO: Ensure the @path is initramfs/initrd image */
+	if (fstat(fd, &stat) < 0) {
+		pr_err("Failed to get the size of %s\n", path);
+		goto out;
+	}
 	ret = write(fd, data, size + 8);
-	if (ret < 0) {
+	if (ret < size + 8) {
+		if (ret < 0)
+			ret = -errno;
 		pr_err("Failed to apply a boot config: %d\n", ret);
-		goto out;
+		if (ret < 0)
+			goto out;
+		goto out_rollback;
 	}
 	/* Write a magic word of the bootconfig */
 	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
-	if (ret < 0) {
+	if (ret < BOOTCONFIG_MAGIC_LEN) {
+		if (ret < 0)
+			ret = -errno;
 		pr_err("Failed to apply a boot config magic: %d\n", ret);
-		goto out;
+		goto out_rollback;
 	}
 	ret = 0;
 out:

tools/bootconfig/main.c static int apply_xbc(const char *path, const char *xbc_path) · line 427

Quoted 6 lines

tools/bootconfig/main.c +3 -0

@@ -411,6 +422,14 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	free(data);
 
 	return ret;
+
+out_rollback:
+	if (ftruncate(fd, stat.st_size) < 0) {

If the first write fails (doesn't modify the initrd), and then this fails, you will show a message of: "may be corrupted. Recommend to rebuild", when that would not be the case. Should a test of the size be done again, to see if it is already the same?

out_rollback:

	old_size = stat.st_size;
	if (stat(fd, &stat) < 0) /* Shouldn't happen, it worked once before */
		goto print_error;

	if (old_size < stat.st_size)
		if (ftruncate(fd, old_size) < 0)
			goto print_error;

	goto out;

print_error:

tools/bootconfig/main.c static int apply_xbc(const char *path, const char *xbc_path) · line 435

Quoted 8 lines

tools/bootconfig/main.c +5 -0

+		ret = -errno;
+		pr_err("Failed to rollback the write error: %d\n", ret);
+		pr_err("The initrd %s may be corrupted. Recommend to rebuild.\n", path);
+	}
+	goto out;
 }
 
 static int usage(void)

-- Steve

Masami Hiramatsu

author maintainer

2020-11-19 01:41 UTC #4 lore raw

Re: [PATCH v4 2/4] tools/bootconfig: Fix to check the write failure correctly

Quoted 61 lines · On Wed, 18 Nov 2020 11:04:37 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 58 lines · On Thu, 19 Nov 2020 00:35:35 +0900 Masami Hiramatsu <[email protected]> wrote:

Fix to check the write(2) failure including partial write correctly and try to rollback the partial write, because if there is no BOOTCONFIG_MAGIC string, we can not remove it.

Fixes 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Chen Yu <[email protected]>

---
 tools/bootconfig/main.c |   27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

tools/bootconfig/main.c +15 -4

index 52eb2bbe8966..905bfaefae35 100644
@@ -337,6 +337,7 @@ static int delete_xbc(const char *path)
 
 static int apply_xbc(const char *path, const char *xbc_path)
 {
+	struct stat stat;
 	u32 size, csum;
 	char *buf, *data;
 	int ret, fd;

@@ -394,16 +395,26 @@ static int apply_xbc(const char *path, const char *xbc_path)
 		return ret;
 	}
 	/* TODO: Ensure the @path is initramfs/initrd image */
+	if (fstat(fd, &stat) < 0) {
+		pr_err("Failed to get the size of %s\n", path);
+		goto out;
+	}
 	ret = write(fd, data, size + 8);
-	if (ret < 0) {
+	if (ret < size + 8) {
+		if (ret < 0)
+			ret = -errno;
 		pr_err("Failed to apply a boot config: %d\n", ret);
-		goto out;
+		if (ret < 0)
+			goto out;
+		goto out_rollback;
 	}
 	/* Write a magic word of the bootconfig */
 	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
-	if (ret < 0) {
+	if (ret < BOOTCONFIG_MAGIC_LEN) {
+		if (ret < 0)
+			ret = -errno;
 		pr_err("Failed to apply a boot config magic: %d\n", ret);
-		goto out;
+		goto out_rollback;
 	}
 	ret = 0;
 out:

@@ -411,6 +422,14 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	free(data);

Quoted 9 lines

tools/bootconfig/main.c static int apply_xbc(const char *path, const char *xbc_path) · line 427
Quoted 4 lines
tools/bootconfig/main.c +3 -0
 	return ret;
+
+out_rollback:
+	if (ftruncate(fd, stat.st_size) < 0) {
If the first write fails (doesn't modify the initrd), and then this fails, you will show a message of: "may be corrupted. Recommend to rebuild", when that would not be the case. Should a test of the size be done again, to see if it is already the same?

Please see the above 2 write error check. You can find

if (ret < 0)

	goto out;

only in the first check. (I know, it might a bit misleadable) So, the first write is failed with error message, it doesn't call rollback.

Quoted 9 lines

out_rollback:

	old_size = stat.st_size;
	if (stat(fd, &stat) < 0) /* Shouldn't happen, it worked once before */
		goto print_error;

	if (old_size < stat.st_size)
		if (ftruncate(fd, old_size) < 0)
			goto print_error;

If the old_size is current size, ftruncate must do nothing, doesn't it?

Thank you,

Quoted 15 lines

	goto out;
print_error:
tools/bootconfig/main.c static int apply_xbc(const char *path, const char *xbc_path) · line 435
Quoted 8 lines
tools/bootconfig/main.c +5 -0
+		ret = -errno;
+		pr_err("Failed to rollback the write error: %d\n", ret);
+		pr_err("The initrd %s may be corrupted. Recommend to rebuild.\n", path);
+	}
+	goto out;
 }
 
 static int usage(void)
-- Steve

Signature · 1 line

Masami Hiramatsu <[email protected]>

Steven Rostedt

2020-11-19 13:59 UTC #5 lore raw

Re: [PATCH v4 2/4] tools/bootconfig: Fix to check the write failure correctly

Quoted 5 lines · On Thu, 19 Nov 2020 10:41:09 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 4 lines
If the first write fails (doesn't modify the initrd), and then this fails, you will show a message of: "may be corrupted. Recommend to rebuild", when that would not be the case. Should a test of the size be done again, to see if it is already the same?

Quoted 7 lines

Please see the above 2 write error check. You can find
if (ret < 0)
	goto out;
only in the first check. (I know, it might a bit misleadable) So, the first write is failed with error message, it doesn't call rollback.

Ah right, I missed check within the error check. I was only looking at the first error and the goto at the end.

-- Steve

Masami Hiramatsu

author maintainer

2020-11-19 05:43 UTC #6 lore raw

Re: [PATCH v4 2/4] tools/bootconfig: Fix to check the write failure correctly

In reply to Masami Hiramatsu

Quoted 56 lines · On Thu, 19 Nov 2020 00:35:35 +0900 Masami Hiramatsu <[email protected]> wrote:

Fix to check the write(2) failure including partial write correctly and try to rollback the partial write, because if there is no BOOTCONFIG_MAGIC string, we can not remove it.

Fixes 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Chen Yu <[email protected]>

---
 tools/bootconfig/main.c |   27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

tools/bootconfig/main.c +15 -4

index 52eb2bbe8966..905bfaefae35 100644
@@ -337,6 +337,7 @@ static int delete_xbc(const char *path)
 
 static int apply_xbc(const char *path, const char *xbc_path)
 {
+	struct stat stat;
 	u32 size, csum;
 	char *buf, *data;
 	int ret, fd;

@@ -394,16 +395,26 @@ static int apply_xbc(const char *path, const char *xbc_path)
 		return ret;
 	}
 	/* TODO: Ensure the @path is initramfs/initrd image */
+	if (fstat(fd, &stat) < 0) {
+		pr_err("Failed to get the size of %s\n", path);
+		goto out;
+	}
 	ret = write(fd, data, size + 8);
-	if (ret < 0) {
+	if (ret < size + 8) {
+		if (ret < 0)
+			ret = -errno;
 		pr_err("Failed to apply a boot config: %d\n", ret);
-		goto out;
+		if (ret < 0)
+			goto out;
+		goto out_rollback;
 	}
 	/* Write a magic word of the bootconfig */
 	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
-	if (ret < 0) {
+	if (ret < BOOTCONFIG_MAGIC_LEN) {
+		if (ret < 0)
+			ret = -errno;
 		pr_err("Failed to apply a boot config magic: %d\n", ret);
-		goto out;
+		goto out_rollback;
 	}
 	ret = 0;
 out:

tools/bootconfig/main.c static int apply_xbc(const char *path, const char *xbc_path) · line 426

Quoted 5 lines

tools/bootconfig/main.c +2 -0

@@ -411,6 +422,14 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	free(data);
 
 	return ret;
+
+out_rollback:

Oops, I forgot to set error to return value here.

I'll fix that.

Thank you,

Quoted 10 lines

tools/bootconfig/main.c +6 -0

+	if (ftruncate(fd, stat.st_size) < 0) {
+		ret = -errno;
+		pr_err("Failed to rollback the write error: %d\n", ret);
+		pr_err("The initrd %s may be corrupted. Recommend to rebuild.\n", path);
+	}
+	goto out;
 }
 
 static int usage(void)

Signature · 1 line

Masami Hiramatsu <[email protected]>

Masami Hiramatsu

author maintainer

2020-11-18 15:35 UTC #7 lore raw

PATCH v4 3/4 tools/bootconfig: Align the bootconfig applied initrd image size to 4

In reply to Masami Hiramatsu

Align the bootconfig applied initrd image size to 4. To fill the gap, the bootconfig command uses null characters in between the bootconfig data and the footer. This will expands the footer size but don't change the checksum.
Thus the block image of the initrd file with bootconfig is as follows.

[initrd][bootconfig][(pad)][size][csum]["#BOOTCONFIG\n"]

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Chen Yu <[email protected]>

---
Changes in v3:
- Fix patch description
- Fix a typo.
- Consolidate several write()s to 1 time write to fix/simplify the error check.
Changes in v2:
- Fix to add the footer size.

---
 tools/bootconfig/main.c             |   48 ++++++++++++++++++++---------------
 tools/bootconfig/test-bootconfig.sh |    6 ++++
 2 files changed, 32 insertions(+), 22 deletions(-)

include/linux/bootconfig.h +1 -0

index 9903088891fa..461f621047f3 100644
@@ -12,6 +12,7 @@
 
 #define BOOTCONFIG_MAGIC	"#BOOTCONFIG\n"
 #define BOOTCONFIG_MAGIC_LEN	12
+#define BOOTCONFIG_ALIGN	4
 
 /* XBC tree node */
 struct xbc_node {

tools/bootconfig/main.c +27 ~~-21~~

76 lines not on this page · raw message

tools/bootconfig/test-bootconfig.sh +5 -1

index d295e406a756..baed891d0ba4 100755
@@ -9,6 +9,7 @@ else
   TESTDIR=.
 fi
 BOOTCONF=${TESTDIR}/bootconfig
+ALIGN=4
 
 INITRD=`mktemp ${TESTDIR}/initrd-XXXX`
 TEMPCONF=`mktemp ${TESTDIR}/temp-XXXX.bconf`

@@ -59,7 +60,10 @@ echo "Show command test"
 xpass $BOOTCONF $INITRD
 
 echo "File size check"
-xpass test $new_size -eq $(expr $bconf_size + $initrd_size + 9 + 12)
+total_size=$(expr $bconf_size + $initrd_size + 9 + 12 + $ALIGN - 1 )
+total_size=$(expr $total_size / $ALIGN)
+total_size=$(expr $total_size \* $ALIGN)
+xpass test $new_size -eq $total_size
 
 echo "Apply command repeat test"
 xpass $BOOTCONF -a $TEMPCONF $INITRD

Steven Rostedt

2020-11-18 16:23 UTC #8 lore raw

Re: [PATCH v4 3/4] tools/bootconfig: Align the bootconfig applied initrd image size to 4

tools/bootconfig/main.c static int apply_xbc(const char *path, const char *xbc_path) · line 407

Quoted 6 lines · On Thu, 19 Nov 2020 00:35:44 +0900 Masami Hiramatsu <[email protected]> wrote:

tools/bootconfig/main.c +6 -0

+
+	/* To align up the total size to BOOTCONFIG_ALIGN, get padding size */
+	total_size = stat.st_size + size + sizeof(u32) * 2 + BOOTCONFIG_MAGIC_LEN;
+	pad = BOOTCONFIG_ALIGN - total_size % BOOTCONFIG_ALIGN;
+	if (pad == BOOTCONFIG_ALIGN)
+		pad = 0;

If alignment is always a power of two, you could simply do:

	pad = (total_size + BOOTCONFIG_ALIGN - 1) &  ~(BOOTCONFIG_ALIGN - 1)) - total_size;

Which will give you the proper padding, without the if.

tools/bootconfig/main.c static int apply_xbc(const char *path, const char *xbc_path) · line 414

Quoted 7 lines

tools/bootconfig/main.c +7 -0

+	size += pad;
+
+	/* Add a footer */
+	*(u32 *)(data + size) = size;
+	*(u32 *)(data + size + sizeof(u32)) = csum;
+	memcpy(data + size + sizeof(u32) * 2, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
+	total_size = size + sizeof(u32) * 2 + BOOTCONFIG_MAGIC_LEN;

I wonder if it would be cleaner to just have a void pointer index for the above:

	void *p;

	p = data + size;
	*(u32 *)p = size;
	p += sizeof(u32);

	*(u32 *)p = csum;
	p += sizeof(u32);

	memcpy(p, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
	p += BOOTCONFIG_MAGIC_LEN;

	total_size = p - (void *)data;

Also, how does this work if we run this on a little endian box for a big endian crossbuild?

-- Steve

Quoted 27 lines

tools/bootconfig/main.c +8 ~~-13~~

+
+	ret = write(fd, data, total_size);
+	if (ret < total_size) {
 		if (ret < 0)
 			ret = -errno;
 		pr_err("Failed to apply a boot config: %d\n", ret);
-		if (ret < 0)
-			goto out;
-		goto out_rollback;
-	}
-	/* Write a magic word of the bootconfig */
-	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
-	if (ret < BOOTCONFIG_MAGIC_LEN) {
-		if (ret < 0)
-			ret = -errno;
-		pr_err("Failed to apply a boot config magic: %d\n", ret);
-		goto out_rollback;
-	}
-	ret = 0;
+		if (ret > 0)
+			goto out_rollback;
+	} else
+		ret = 0;
+
 out:
 	close(fd);
 	free(data);

Masami Hiramatsu

author maintainer

2020-11-19 02:33 UTC #9 lore raw

Re: [PATCH v4 3/4] tools/bootconfig: Align the bootconfig applied initrd image size to 4

Quoted 12 lines · On Wed, 18 Nov 2020 11:22:49 -0500 Steven Rostedt <[email protected]> wrote:

tools/bootconfig/main.c static int apply_xbc(const char *path, const char *xbc_path) · line 407
Quoted 6 lines · On Thu, 19 Nov 2020 00:35:44 +0900 Masami Hiramatsu <[email protected]> wrote:
tools/bootconfig/main.c +6 -0
+
+	/* To align up the total size to BOOTCONFIG_ALIGN, get padding size */
+	total_size = stat.st_size + size + sizeof(u32) * 2 + BOOTCONFIG_MAGIC_LEN;
+	pad = BOOTCONFIG_ALIGN - total_size % BOOTCONFIG_ALIGN;
+	if (pad == BOOTCONFIG_ALIGN)
+		pad = 0;
If alignment is always a power of two, you could simply do:

Quoted 3 lines

	pad = (total_size + BOOTCONFIG_ALIGN - 1) &  ~(BOOTCONFIG_ALIGN - 1)) - total_size;
Which will give you the proper padding, without the if.

Thanks, and in that case, I would like to introduce BOOTCONFIG_ALINE_BITS and BOOTCONFIG_ALINE_MASK so that we make sure the alignment is power of two.

Quoted 10 lines

tools/bootconfig/main.c static int apply_xbc(const char *path, const char *xbc_path) · line 414
Quoted 7 lines
tools/bootconfig/main.c +7 -0
+	size += pad;
+
+	/* Add a footer */
+	*(u32 *)(data + size) = size;
+	*(u32 *)(data + size + sizeof(u32)) = csum;
+	memcpy(data + size + sizeof(u32) * 2, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
+	total_size = size + sizeof(u32) * 2 + BOOTCONFIG_MAGIC_LEN;
I wonder if it would be cleaner to just have a void pointer index for the above:

Quoted 13 lines

	void *p;

	p = data + size;
	*(u32 *)p = size;
	p += sizeof(u32);

	*(u32 *)p = csum;
	p += sizeof(u32);

	memcpy(p, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
	p += BOOTCONFIG_MAGIC_LEN;

	total_size = p - (void *)data;

OK.

Quoted 2 lines

Also, how does this work if we run this on a little endian box for a big endian crossbuild?

Hmm, good point. I expected that the bootconfig command will be used natively. Hmm, there are 2 options.

- Add endian option to the bootconfig (-le/-be) for cross build.
- Update the footer format to use "ascii" size and checksum.

To generalize the format, latter one is better. But it also involves the kernel side update too.

Thank you,

Quoted 31 lines

-- Steve

Quoted 27 lines

tools/bootconfig/main.c +8 ~~-13~~

+
+	ret = write(fd, data, total_size);
+	if (ret < total_size) {
 		if (ret < 0)
 			ret = -errno;
 		pr_err("Failed to apply a boot config: %d\n", ret);
-		if (ret < 0)
-			goto out;
-		goto out_rollback;
-	}
-	/* Write a magic word of the bootconfig */
-	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
-	if (ret < BOOTCONFIG_MAGIC_LEN) {
-		if (ret < 0)
-			ret = -errno;
-		pr_err("Failed to apply a boot config magic: %d\n", ret);
-		goto out_rollback;
-	}
-	ret = 0;
+		if (ret > 0)
+			goto out_rollback;
+	} else
+		ret = 0;
+
 out:
 	close(fd);
 	free(data);

Signature · 1 line

Masami Hiramatsu <[email protected]>

Masami Hiramatsu

author maintainer

2020-11-18 15:36 UTC #10 lore raw

PATCH v4 4/4 docs: bootconfig: Update file format on initrd image

In reply to Masami Hiramatsu

To align the total file size, add padding null character when appending the bootconfig to initrd image.

Signed-off-by Masami Hiramatsu <[email protected]>

---

 0 files changed

Documentation/admin-guide/bootconfig.rst +13 -5

index a22024f9175e..363599683784 100644
@@ -137,15 +137,22 @@ 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 padding, size,
+checksum and 12-byte magic word as below.
 
-[initrd][bootconfig][size(u32)][checksum(u32)][#BOOTCONFIG\n]
+[initrd][bootconfig][padding][size(u32)][checksum(u32)][#BOOTCONFIG\n]
+
+When the boot configuration is added to the initrd image, the total
+file size is aligned to 4 bytes. To fill the gap, null characters
+(``\0``) will be added. Thus the ``size`` is the length of the bootconfig
+file + padding bytes.
 
 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.
+update the boot loader and the kernel image itself as long as the boot
+loader passes the correct initrd file size. If by any chance, the boot
+loader passes a longer size, the kernel feils to find the bootconfig data.
 
 To do this operation, Linux kernel provides "bootconfig" command under
 tools/bootconfig, which allows admin to apply or delete the config file

@@ -176,7 +183,8 @@ up to 512 key-value pairs. If keys contains 3 words in average, it can
 contain 256 key-value pairs. In most cases, the number of config items
 will be under 100 entries and smaller than 8KB, so it would be enough.
 If the node number exceeds 1024, parser returns an error even if the file
-size is smaller than 32KB.
+size is smaller than 32KB. (Note that this maximum size is not including
+the padding null characters.)
 Anyway, since bootconfig command verifies it when appending a boot config
 to initrd image, user can notice it before boot.