[PATCH v7 0/6] initramfs: "crc" cpio format and INITRAMFS_PRESERVE_MTIME

[PATCH v7 0/6] initramfs: "crc" cpio format and INITRAMFS_PRESERVE_MTIME

[David Disseldorp]

This patchset does some minor initramfs refactoring and allows cpio
entry mtime preservation to be disabled via a new Kconfig
INITRAMFS_PRESERVE_MTIME option.
Patches 4/6 to 6/6 implement support for creation and extraction of
"crc" cpio archives, which carry file data checksums. Basic tests for
this functionality can be found at
Link: https://github.com/rapido-linux/rapido/pull/163

Changes since v6 following feedback from Andrew Morton:
- 3/6: improve commit message and don't split out initramfs_mtime.h
- add extra acks and sob tags for 1/6, 2/6 and 4/6

Changes since v5:
- add PATCH 2/6 initramfs: make dir_entry.name a flexible array member
- minor commit message rewording

Changes since v4, following feedback from Matthew Wilcox:
- implement cpio "crc" archive creation and extraction
- add patch to fix gen_init_cpio short read handling
- drop now-unnecessary "crc" documentation and error msg changes

Changes since v3, following feedback from Martin Wilck:
- 4/4: keep vfs_utimes() call in do_copy() path
  + drop [PATCH v3 4/5] initramfs: use do_utime() wrapper consistently
  + add do_utime_path() helper
  + clean up timespec64 initialisation
- 4/4: move all mtime preservation logic to initramfs_mtime.h and drop
  separate .c
- 4/4: improve commit message


 init/Kconfig        | 10 +++++
 init/initramfs.c    | 76 ++++++++++++++++++++++++-------------
 usr/gen_init_cpio.c | 92 +++++++++++++++++++++++++++++++++------------
 3 files changed, 127 insertions(+), 51 deletions(-)

[PATCH v7 1/6] initramfs: refactor do_header() cpio magic checks

[David Disseldorp]

do_header() is called for each cpio entry and fails if the first six
bytes don't match "newc" magic. The magic check includes a special case
error message if POSIX.1 ASCII (cpio -H odc) magic is detected. This
special case POSIX.1 check can be nested under the "newc" mismatch code
path to avoid calling memcmp() twice in a non-error case.

Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Martin Wilck <mwilck@suse.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 init/initramfs.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 2f3d96dc3db6..2f79b3ec0b40 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -257,12 +257,11 @@ static int __init do_collect(void)
 
 static int __init do_header(void)
 {
-	if (memcmp(collected, "070707", 6)==0) {
-		error("incorrect cpio method used: use -H newc option");
-		return 1;
-	}
 	if (memcmp(collected, "070701", 6)) {
-		error("no cpio magic");
+		if (memcmp(collected, "070707", 6) == 0)
+			error("incorrect cpio method used: use -H newc option");
+		else
+			error("no cpio magic");
 		return 1;
 	}
 	parse_header(collected);
-- 
2.34.1

[PATCH v7 2/6] initramfs: make dir_entry.name a flexible array member

[David Disseldorp]

dir_entry.name is currently allocated via a separate kstrdup(). Change
it to a flexible array member and allocate it along with struct
dir_entry.

Signed-off-by: David Disseldorp <ddiss@suse.de>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 init/initramfs.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 2f79b3ec0b40..656d2d71349f 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -130,17 +130,20 @@ static long __init do_utime(char *filename, time64_t mtime)
 static __initdata LIST_HEAD(dir_list);
 struct dir_entry {
 	struct list_head list;
-	char *name;
 	time64_t mtime;
+	char name[];
 };
 
 static void __init dir_add(const char *name, time64_t mtime)
 {
-	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
+	size_t nlen = strlen(name) + 1;
+	struct dir_entry *de;
+
+	de = kmalloc(sizeof(struct dir_entry) + nlen, GFP_KERNEL);
 	if (!de)
 		panic_show_mem("can't allocate dir_entry buffer");
 	INIT_LIST_HEAD(&de->list);
-	de->name = kstrdup(name, GFP_KERNEL);
+	strscpy(de->name, name, nlen);
 	de->mtime = mtime;
 	list_add(&de->list, &dir_list);
 }
@@ -151,7 +154,6 @@ static void __init dir_utime(void)
 	list_for_each_entry_safe(de, tmp, &dir_list, list) {
 		list_del(&de->list);
 		do_utime(de->name, de->mtime);
-		kfree(de->name);
 		kfree(de);
 	}
 }
-- 
2.34.1

[PATCH v7 3/6] initramfs: add INITRAMFS_PRESERVE_MTIME Kconfig option

[David Disseldorp]

initramfs cpio mtime preservation, as implemented in commit 889d51a10712
("initramfs: add option to preserve mtime from initramfs cpio images"),
uses a linked list to defer directory mtime processing until after all
other items in the cpio archive have been processed. This is done to
ensure that parent directory mtimes aren't overwritten via subsequent
child creation.

The lkml link below indicates that the mtime retention use case was for
embedded devices with applications running exclusively out of initramfs,
where the 32-bit mtime value provided a rough file version identifier.
Linux distributions which discard an extracted initramfs immediately
after the root filesystem has been mounted may want to avoid the
unnecessary overhead.

This change adds a new INITRAMFS_PRESERVE_MTIME Kconfig option, which
can be used to disable on-by-default mtime retention and in turn
speed up initramfs extraction, particularly for cpio archives with large
directory counts.

Benchmarks with a one million directory cpio archive extracted 20 times
demonstrated:
				mean extraction time (s)	std dev
INITRAMFS_PRESERVE_MTIME=y		3.808			 0.006
INITRAMFS_PRESERVE_MTIME unset		3.056			 0.004

The above extraction times were measured using ftrace
(initcall_finish - initcall_start) values for populate_rootfs() with
initramfs_async disabled.

Link: https://lkml.org/lkml/2008/9/3/424
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Martin Wilck <mwilck@suse.com>
[ddiss: rebase atop dir_entry.name flexible array member and drop
 separate initramfs_mtime.h header]
---
 init/Kconfig     | 10 ++++++++++
 init/initramfs.c | 28 ++++++++++++++++------------
 2 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index ddcbefe535e9..0fbaa07810a4 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1361,6 +1361,16 @@ config BOOT_CONFIG
 
 	  If unsure, say Y.
 
+config INITRAMFS_PRESERVE_MTIME
+	bool "Preserve cpio archive mtimes in initramfs"
+	default y
+	help
+	  Each entry in an initramfs cpio archive carries an mtime value. When
+	  enabled, extracted cpio items take this mtime, with directory mtime
+	  setting deferred until after creation of any child entries.
+
+	  If unsure, say Y.
+
 choice
 	prompt "Compiler optimization level"
 	default CC_OPTIMIZE_FOR_PERFORMANCE
diff --git a/init/initramfs.c b/init/initramfs.c
index 656d2d71349f..b5bfed859fa9 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -116,15 +116,17 @@ static void __init free_hash(void)
 	}
 }
 
-static long __init do_utime(char *filename, time64_t mtime)
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+static void __init do_utime(char *filename, time64_t mtime)
 {
-	struct timespec64 t[2];
+	struct timespec64 t[2] = { { .tv_sec = mtime }, { .tv_sec = mtime } };
+	init_utimes(filename, t);
+}
 
-	t[0].tv_sec = mtime;
-	t[0].tv_nsec = 0;
-	t[1].tv_sec = mtime;
-	t[1].tv_nsec = 0;
-	return init_utimes(filename, t);
+static void __init do_utime_path(const struct path *path, time64_t mtime)
+{
+	struct timespec64 t[2] = { { .tv_sec = mtime }, { .tv_sec = mtime } };
+	vfs_utimes(path, t);
 }
 
 static __initdata LIST_HEAD(dir_list);
@@ -157,6 +159,12 @@ static void __init dir_utime(void)
 		kfree(de);
 	}
 }
+#else
+static void __init do_utime(char *filename, time64_t mtime) {}
+static void __init do_utime_path(const struct path *path, time64_t mtime) {}
+static void __init dir_add(const char *name, time64_t mtime) {}
+static void __init dir_utime(void) {}
+#endif
 
 static __initdata time64_t mtime;
 
@@ -381,14 +389,10 @@ static int __init do_name(void)
 static int __init do_copy(void)
 {
 	if (byte_count >= body_len) {
-		struct timespec64 t[2] = { };
 		if (xwrite(wfile, victim, body_len, &wfile_pos) != body_len)
 			error("write error");
 
-		t[0].tv_sec = mtime;
-		t[1].tv_sec = mtime;
-		vfs_utimes(&wfile->f_path, t);
-
+		do_utime_path(&wfile->f_path, mtime);
 		fput(wfile);
 		eat(body_len);
 		state = SkipIt;
-- 
2.34.1

[PATCH v7 4/6] gen_init_cpio: fix short read file handling

[David Disseldorp]

When processing a "file" entry, gen_init_cpio attempts to allocate a
buffer large enough to stage the entire contents of the source file.
It then attempts to fill the buffer via a single read() call and
subsequently writes out the entire buffer length, without checking that
read() returned the full length, potentially writing uninitialized
buffer memory.

Fix this by breaking up file I/O into 64k chunks and only writing the
length returned by the prior read() call.

Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 usr/gen_init_cpio.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/usr/gen_init_cpio.c b/usr/gen_init_cpio.c
index 0e2c8a5838b1..9a0f8c37273a 100644
--- a/usr/gen_init_cpio.c
+++ b/usr/gen_init_cpio.c
@@ -20,6 +20,7 @@
 
 #define xstr(s) #s
 #define str(s) xstr(s)
+#define MIN(a, b) ((a) < (b) ? (a) : (b))
 
 static unsigned int offset;
 static unsigned int ino = 721;
@@ -297,9 +298,8 @@ static int cpio_mkfile(const char *name, const char *location,
 			unsigned int nlinks)
 {
 	char s[256];
-	char *filebuf = NULL;
 	struct stat buf;
-	long size;
+	unsigned long size;
 	int file = -1;
 	int retval;
 	int rc = -1;
@@ -326,22 +326,17 @@ static int cpio_mkfile(const char *name, const char *location,
 		buf.st_mtime = 0xffffffff;
 	}
 
-	filebuf = malloc(buf.st_size);
-	if (!filebuf) {
-		fprintf (stderr, "out of memory\n");
-		goto error;
-	}
-
-	retval = read (file, filebuf, buf.st_size);
-	if (retval < 0) {
-		fprintf (stderr, "Can not read %s file\n", location);
+	if (buf.st_size > 0xffffffff) {
+		fprintf(stderr, "%s: Size exceeds maximum cpio file size\n",
+			location);
 		goto error;
 	}
 
 	size = 0;
 	for (i = 1; i <= nlinks; i++) {
 		/* data goes on last link */
-		if (i == nlinks) size = buf.st_size;
+		if (i == nlinks)
+			size = buf.st_size;
 
 		if (name[0] == '/')
 			name++;
@@ -366,23 +361,34 @@ static int cpio_mkfile(const char *name, const char *location,
 		push_string(name);
 		push_pad();
 
-		if (size) {
-			if (fwrite(filebuf, size, 1, stdout) != 1) {
+		while (size) {
+			unsigned char filebuf[65536];
+			ssize_t this_read;
+			size_t this_size = MIN(size, sizeof(filebuf));
+
+			this_read = read(file, filebuf, this_size);
+			if (this_read <= 0 || this_read > this_size) {
+				fprintf(stderr, "Can not read %s file\n", location);
+				goto error;
+			}
+
+			if (fwrite(filebuf, this_read, 1, stdout) != 1) {
 				fprintf(stderr, "writing filebuf failed\n");
 				goto error;
 			}
-			offset += size;
-			push_pad();
+			offset += this_read;
+			size -= this_read;
 		}
+		push_pad();
 
 		name += namesize;
 	}
 	ino++;
 	rc = 0;
-	
+
 error:
-	if (filebuf) free(filebuf);
-	if (file >= 0) close(file);
+	if (file >= 0)
+		close(file);
 	return rc;
 }
 
-- 
2.34.1

[PATCH v7 5/6] gen_init_cpio: support file checksum archiving

[David Disseldorp]

Documentation/driver-api/early-userspace/buffer-format.rst includes the
specification for checksum-enabled cpio archives. Implement support for
this format in gen_init_cpio via a new '-c' parameter.

Suggested-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 usr/gen_init_cpio.c | 54 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/usr/gen_init_cpio.c b/usr/gen_init_cpio.c
index 9a0f8c37273a..dc838e26a5b9 100644
--- a/usr/gen_init_cpio.c
+++ b/usr/gen_init_cpio.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <stdio.h>
 #include <stdlib.h>
+#include <stdint.h>
+#include <stdbool.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <string.h>
@@ -25,6 +27,7 @@
 static unsigned int offset;
 static unsigned int ino = 721;
 static time_t default_mtime;
+static bool do_csum = false;
 
 struct file_handler {
 	const char *type;
@@ -78,7 +81,7 @@ static void cpio_trailer(void)
 
 	sprintf(s, "%s%08X%08X%08lX%08lX%08X%08lX"
 	       "%08X%08X%08X%08X%08X%08X%08X",
-		"070701",		/* magic */
+		do_csum ? "070702" : "070701", /* magic */
 		0,			/* ino */
 		0,			/* mode */
 		(long) 0,		/* uid */
@@ -110,7 +113,7 @@ static int cpio_mkslink(const char *name, const char *target,
 		name++;
 	sprintf(s,"%s%08X%08X%08lX%08lX%08X%08lX"
 	       "%08X%08X%08X%08X%08X%08X%08X",
-		"070701",		/* magic */
+		do_csum ? "070702" : "070701", /* magic */
 		ino++,			/* ino */
 		S_IFLNK | mode,		/* mode */
 		(long) uid,		/* uid */
@@ -159,7 +162,7 @@ static int cpio_mkgeneric(const char *name, unsigned int mode,
 		name++;
 	sprintf(s,"%s%08X%08X%08lX%08lX%08X%08lX"
 	       "%08X%08X%08X%08X%08X%08X%08X",
-		"070701",		/* magic */
+		do_csum ? "070702" : "070701", /* magic */
 		ino++,			/* ino */
 		mode,			/* mode */
 		(long) uid,		/* uid */
@@ -253,7 +256,7 @@ static int cpio_mknod(const char *name, unsigned int mode,
 		name++;
 	sprintf(s,"%s%08X%08X%08lX%08lX%08X%08lX"
 	       "%08X%08X%08X%08X%08X%08X%08X",
-		"070701",		/* magic */
+		do_csum ? "070702" : "070701", /* magic */
 		ino++,			/* ino */
 		mode,			/* mode */
 		(long) uid,		/* uid */
@@ -293,6 +296,29 @@ static int cpio_mknod_line(const char *line)
 	return rc;
 }
 
+static int cpio_mkfile_csum(int fd, unsigned long size, uint32_t *csum)
+{
+	while (size) {
+		unsigned char filebuf[65536];
+		ssize_t this_read;
+		size_t i, this_size = MIN(size, sizeof(filebuf));
+
+		this_read = read(fd, filebuf, this_size);
+		if (this_read <= 0 || this_read > this_size)
+			return -1;
+
+		for (i = 0; i < this_read; i++)
+			*csum += filebuf[i];
+
+		size -= this_read;
+	}
+	/* seek back to the start for data segment I/O */
+	if (lseek(fd, 0, SEEK_SET) < 0)
+		return -1;
+
+	return 0;
+}
+
 static int cpio_mkfile(const char *name, const char *location,
 			unsigned int mode, uid_t uid, gid_t gid,
 			unsigned int nlinks)
@@ -305,6 +331,7 @@ static int cpio_mkfile(const char *name, const char *location,
 	int rc = -1;
 	int namesize;
 	unsigned int i;
+	uint32_t csum = 0;
 
 	mode |= S_IFREG;
 
@@ -332,6 +359,11 @@ static int cpio_mkfile(const char *name, const char *location,
 		goto error;
 	}
 
+	if (do_csum && cpio_mkfile_csum(file, buf.st_size, &csum) < 0) {
+		fprintf(stderr, "Failed to checksum file %s\n", location);
+		goto error;
+	}
+
 	size = 0;
 	for (i = 1; i <= nlinks; i++) {
 		/* data goes on last link */
@@ -343,7 +375,7 @@ static int cpio_mkfile(const char *name, const char *location,
 		namesize = strlen(name) + 1;
 		sprintf(s,"%s%08X%08X%08lX%08lX%08X%08lX"
 		       "%08lX%08X%08X%08X%08X%08X%08X",
-			"070701",		/* magic */
+			do_csum ? "070702" : "070701", /* magic */
 			ino,			/* ino */
 			mode,			/* mode */
 			(long) uid,		/* uid */
@@ -356,7 +388,7 @@ static int cpio_mkfile(const char *name, const char *location,
 			0,			/* rmajor */
 			0,			/* rminor */
 			namesize,		/* namesize */
-			0);			/* chksum */
+			size ? csum : 0);	/* chksum */
 		push_hdr(s);
 		push_string(name);
 		push_pad();
@@ -464,7 +496,7 @@ static int cpio_mkfile_line(const char *line)
 static void usage(const char *prog)
 {
 	fprintf(stderr, "Usage:\n"
-		"\t%s [-t <timestamp>] <cpio_list>\n"
+		"\t%s [-t <timestamp>] [-c] <cpio_list>\n"
 		"\n"
 		"<cpio_list> is a file containing newline separated entries that\n"
 		"describe the files to be included in the initramfs archive:\n"
@@ -499,7 +531,8 @@ static void usage(const char *prog)
 		"\n"
 		"<timestamp> is time in seconds since Epoch that will be used\n"
 		"as mtime for symlinks, special files and directories. The default\n"
-		"is to use the current time for these entries.\n",
+		"is to use the current time for these entries.\n"
+		"-c: calculate and store 32-bit checksums for file data.\n",
 		prog);
 }
 
@@ -541,7 +574,7 @@ int main (int argc, char *argv[])
 
 	default_mtime = time(NULL);
 	while (1) {
-		int opt = getopt(argc, argv, "t:h");
+		int opt = getopt(argc, argv, "t:ch");
 		char *invalid;
 
 		if (opt == -1)
@@ -556,6 +589,9 @@ int main (int argc, char *argv[])
 				exit(1);
 			}
 			break;
+		case 'c':
+			do_csum = true;
+			break;
 		case 'h':
 		case '?':
 			usage(argv[0]);
-- 
2.34.1

[PATCH v7 6/6] initramfs: support cpio extraction with file checksums

[David Disseldorp]

Add support for extraction of checksum-enabled "070702" cpio archives,
specified in Documentation/driver-api/early-userspace/buffer-format.rst.
Fail extraction if the calculated file data checksum doesn't match the
value carried in the header.

Suggested-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/initramfs.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index b5bfed859fa9..dc84cf756cea 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -17,8 +17,11 @@
 #include <linux/init_syscalls.h>
 #include <linux/umh.h>
 
-static ssize_t __init xwrite(struct file *file, const char *p, size_t count,
-		loff_t *pos)
+static __initdata bool csum_present;
+static __initdata u32 io_csum;
+
+static ssize_t __init xwrite(struct file *file, const unsigned char *p,
+		size_t count, loff_t *pos)
 {
 	ssize_t out = 0;
 
@@ -33,6 +36,13 @@ static ssize_t __init xwrite(struct file *file, const char *p, size_t count,
 		} else if (rv == 0)
 			break;
 
+		if (csum_present) {
+			ssize_t i;
+
+			for (i = 0; i < rv; i++)
+				io_csum += p[i];
+		}
+
 		p += rv;
 		out += rv;
 		count -= rv;
@@ -176,15 +186,16 @@ static __initdata unsigned long body_len, name_len;
 static __initdata uid_t uid;
 static __initdata gid_t gid;
 static __initdata unsigned rdev;
+static __initdata u32 hdr_csum;
 
 static void __init parse_header(char *s)
 {
-	unsigned long parsed[12];
+	unsigned long parsed[13];
 	char buf[9];
 	int i;
 
 	buf[8] = '\0';
-	for (i = 0, s += 6; i < 12; i++, s += 8) {
+	for (i = 0, s += 6; i < 13; i++, s += 8) {
 		memcpy(buf, s, 8);
 		parsed[i] = simple_strtoul(buf, NULL, 16);
 	}
@@ -199,6 +210,7 @@ static void __init parse_header(char *s)
 	minor = parsed[8];
 	rdev = new_encode_dev(MKDEV(parsed[9], parsed[10]));
 	name_len = parsed[11];
+	hdr_csum = parsed[12];
 }
 
 /* FSM */
@@ -267,7 +279,11 @@ static int __init do_collect(void)
 
 static int __init do_header(void)
 {
-	if (memcmp(collected, "070701", 6)) {
+	if (!memcmp(collected, "070701", 6)) {
+		csum_present = false;
+	} else if (!memcmp(collected, "070702", 6)) {
+		csum_present = true;
+	} else {
 		if (memcmp(collected, "070707", 6) == 0)
 			error("incorrect cpio method used: use -H newc option");
 		else
@@ -362,6 +378,7 @@ static int __init do_name(void)
 			if (IS_ERR(wfile))
 				return 0;
 			wfile_pos = 0;
+			io_csum = 0;
 
 			vfs_fchown(wfile, uid, gid);
 			vfs_fchmod(wfile, mode);
@@ -394,6 +411,8 @@ static int __init do_copy(void)
 
 		do_utime_path(&wfile->f_path, mtime);
 		fput(wfile);
+		if (csum_present && io_csum != hdr_csum)
+			error("bad data checksum");
 		eat(body_len);
 		state = SkipIt;
 		return 0;
-- 
2.34.1

Re: [PATCH v7 0/6] initramfs: "crc" cpio format and INITRAMFS_PRESERVE_MTIME

[David Disseldorp]

Ping...

@Matthew: "crc" archive support was added following your suggestion in
https://lore.kernel.org/all/YYwBzj0isuKOjjUe@casper.infradead.org/ -
I'd really appreciate some feedback on those patches.

On Mon,  4 Apr 2022 11:34:24 +0200, David Disseldorp wrote:

> This patchset does some minor initramfs refactoring and allows cpio
> entry mtime preservation to be disabled via a new Kconfig
> INITRAMFS_PRESERVE_MTIME option.
> Patches 4/6 to 6/6 implement support for creation and extraction of
> "crc" cpio archives, which carry file data checksums. Basic tests for
> this functionality can be found at
> Link: https://github.com/rapido-linux/rapido/pull/163
> 
> Changes since v6 following feedback from Andrew Morton:
> - 3/6: improve commit message and don't split out initramfs_mtime.h
> - add extra acks and sob tags for 1/6, 2/6 and 4/6
> 
> Changes since v5:
> - add PATCH 2/6 initramfs: make dir_entry.name a flexible array member
> - minor commit message rewording
> 
> Changes since v4, following feedback from Matthew Wilcox:
> - implement cpio "crc" archive creation and extraction
> - add patch to fix gen_init_cpio short read handling
> - drop now-unnecessary "crc" documentation and error msg changes
> 
> Changes since v3, following feedback from Martin Wilck:
> - 4/4: keep vfs_utimes() call in do_copy() path
>   + drop [PATCH v3 4/5] initramfs: use do_utime() wrapper consistently
>   + add do_utime_path() helper
>   + clean up timespec64 initialisation
> - 4/4: move all mtime preservation logic to initramfs_mtime.h and drop
>   separate .c
> - 4/4: improve commit message
> 
> 
>  init/Kconfig        | 10 +++++
>  init/initramfs.c    | 76 ++++++++++++++++++++++++-------------
>  usr/gen_init_cpio.c | 92 +++++++++++++++++++++++++++++++++------------
>  3 files changed, 127 insertions(+), 51 deletions(-)

Re: [PATCH v7 3/6] initramfs: add INITRAMFS_PRESERVE_MTIME Kconfig option

[Andrew Morton]

On Mon,  4 Apr 2022 11:34:27 +0200 David Disseldorp <ddiss@suse.de> wrote:

> initramfs cpio mtime preservation, as implemented in commit 889d51a10712
> ("initramfs: add option to preserve mtime from initramfs cpio images"),
> uses a linked list to defer directory mtime processing until after all
> other items in the cpio archive have been processed. This is done to
> ensure that parent directory mtimes aren't overwritten via subsequent
> child creation.
> 
> The lkml link below indicates that the mtime retention use case was for
> embedded devices with applications running exclusively out of initramfs,
> where the 32-bit mtime value provided a rough file version identifier.
> Linux distributions which discard an extracted initramfs immediately
> after the root filesystem has been mounted may want to avoid the
> unnecessary overhead.
> 
> This change adds a new INITRAMFS_PRESERVE_MTIME Kconfig option, which
> can be used to disable on-by-default mtime retention and in turn
> speed up initramfs extraction, particularly for cpio archives with large
> directory counts.
> 
> Benchmarks with a one million directory cpio archive extracted 20 times
> demonstrated:
> 				mean extraction time (s)	std dev
> INITRAMFS_PRESERVE_MTIME=y		3.808			 0.006
> INITRAMFS_PRESERVE_MTIME unset		3.056			 0.004

So about 35 nsec per directory?

By how much is this likely to reduce boot time in a real-world situation?

Re: [PATCH v7 4/6] gen_init_cpio: fix short read file handling

[Andrew Morton]

On Mon,  4 Apr 2022 11:34:28 +0200 David Disseldorp <ddiss@suse.de> wrote:

> When processing a "file" entry, gen_init_cpio attempts to allocate a
> buffer large enough to stage the entire contents of the source file.
> It then attempts to fill the buffer via a single read() call and
> subsequently writes out the entire buffer length, without checking that
> read() returned the full length, potentially writing uninitialized
> buffer memory.

That was rather rude of it.

> Fix this by breaking up file I/O into 64k chunks and only writing the
> length returned by the prior read() call.

Does this change fix any known or reported problems?

Re: [PATCH v7 3/6] initramfs: add INITRAMFS_PRESERVE_MTIME Kconfig option

[David Disseldorp]

On Tue, 26 Apr 2022 13:39:08 -0700, Andrew Morton wrote:

> On Mon,  4 Apr 2022 11:34:27 +0200 David Disseldorp <ddiss@suse.de> wrote:
> 
> > initramfs cpio mtime preservation, as implemented in commit 889d51a10712
> > ("initramfs: add option to preserve mtime from initramfs cpio images"),
> > uses a linked list to defer directory mtime processing until after all
> > other items in the cpio archive have been processed. This is done to
> > ensure that parent directory mtimes aren't overwritten via subsequent
> > child creation.
> > 
> > The lkml link below indicates that the mtime retention use case was for
> > embedded devices with applications running exclusively out of initramfs,
> > where the 32-bit mtime value provided a rough file version identifier.
> > Linux distributions which discard an extracted initramfs immediately
> > after the root filesystem has been mounted may want to avoid the
> > unnecessary overhead.
> > 
> > This change adds a new INITRAMFS_PRESERVE_MTIME Kconfig option, which
> > can be used to disable on-by-default mtime retention and in turn
> > speed up initramfs extraction, particularly for cpio archives with large
> > directory counts.
> > 
> > Benchmarks with a one million directory cpio archive extracted 20 times
> > demonstrated:
> > 				mean extraction time (s)	std dev
> > INITRAMFS_PRESERVE_MTIME=y		3.808			 0.006
> > INITRAMFS_PRESERVE_MTIME unset		3.056			 0.004  
> 
> So about 35 nsec per directory?

~750 nsec - I should have clarified that the "20" refers to the number
of runs over which the "mean extraction time" is averaged.

> By how much is this likely to reduce boot time in a real-world situation?

Not much, although my xfstests initramfs images tend to get into the
hundreds of directories. These numbers were captured using QEMU/kvm on
my laptop - I could rerun the benchmark on an old ARM SBC if more data
points are needed.

Cheers, David

Re: [PATCH v7 4/6] gen_init_cpio: fix short read file handling

[David Disseldorp]

On Tue, 26 Apr 2022 13:40:39 -0700, Andrew Morton wrote:

> On Mon,  4 Apr 2022 11:34:28 +0200 David Disseldorp <ddiss@suse.de> wrote:
> 
> > When processing a "file" entry, gen_init_cpio attempts to allocate a
> > buffer large enough to stage the entire contents of the source file.
> > It then attempts to fill the buffer via a single read() call and
> > subsequently writes out the entire buffer length, without checking that
> > read() returned the full length, potentially writing uninitialized
> > buffer memory.  
> 
> That was rather rude of it.
> 
> > Fix this by breaking up file I/O into 64k chunks and only writing the
> > length returned by the prior read() call.  
> 
> Does this change fix any known or reported problems?

This was found via code inspection. I'm not aware of anyone hitting it
in the wild.

Thanks for the feedback, Andrew.

Cheers, David