[PATCH v4 0/4] initramfs: "crc" cpio format and INITRAMFS_PRESERVE_MTIME

[PATCH v4 0/4] initramfs: "crc" cpio format and INITRAMFS_PRESERVE_MTIME

[David Disseldorp]

This patchset does some minor refactoring of cpio header magic checking
and corrects documentation.
Patch 4/4 allows cpio entry mtime preservation to be disabled via a new
INITRAMFS_PRESERVE_MTIME Kconfig option.

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

 .../early-userspace/buffer-format.rst         | 24 +++-----
 init/Kconfig                                  | 10 ++++
 init/initramfs.c                              | 57 +++----------------
 init/initramfs_mtime.h                        | 50 ++++++++++++++++
 4 files changed, 75 insertions(+), 66 deletions(-)

[PATCH v4 1/4] 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>
---
 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.31.1

[PATCH v4 2/4] initramfs: print helpful cpio error on "crc" magic

[David Disseldorp]

Contrary to the buffer-format.rst documentation, initramfs cpio
extraction does not support "crc" archives, which carry "070702"
header magic. Make it a little clearer that "newc" (magic="070701") is
the only supported cpio format, by extending the POSIX.1 ASCII
(magic="070707") specific error message to also cover "crc" magic.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/initramfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 2f79b3ec0b40..44e692ae4646 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -258,7 +258,7 @@ static int __init do_collect(void)
 static int __init do_header(void)
 {
 	if (memcmp(collected, "070701", 6)) {
-		if (memcmp(collected, "070707", 6) == 0)
+		if (memcmp(collected, "0707", 4) == 0)
 			error("incorrect cpio method used: use -H newc option");
 		else
 			error("no cpio magic");
-- 
2.31.1

[PATCH v4 3/4] docs: remove mention of "crc" cpio format support

[David Disseldorp]

init/initramfs.c only supports extraction of cpio archives carrying the
"newc" header magic ("070701"). Remove statements indicating support for
the "crc" cpio format.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 .../early-userspace/buffer-format.rst         | 24 +++++++------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/Documentation/driver-api/early-userspace/buffer-format.rst b/Documentation/driver-api/early-userspace/buffer-format.rst
index 7f74e301fdf3..0df76bca444c 100644
--- a/Documentation/driver-api/early-userspace/buffer-format.rst
+++ b/Documentation/driver-api/early-userspace/buffer-format.rst
@@ -14,10 +14,10 @@ is different.  The initramfs buffer contains an archive which is
 expanded into a ramfs filesystem; this document details the format of
 the initramfs buffer format.
 
-The initramfs buffer format is based around the "newc" or "crc" CPIO
-formats, and can be created with the cpio(1) utility.  The cpio
-archive can be compressed using gzip(1).  One valid version of an
-initramfs buffer is thus a single .cpio.gz file.
+The initramfs buffer format is based around the "newc" CPIO format, and
+can be created with the cpio(1) utility.  The cpio archive can be
+compressed using gzip(1).  One valid version of an initramfs buffer is
+thus a single .cpio.gz file.
 
 The full format of the initramfs buffer is defined by the following
 grammar, where::
@@ -40,9 +40,8 @@ grammar, where::
 
 
 In human terms, the initramfs buffer contains a collection of
-compressed and/or uncompressed cpio archives (in the "newc" or "crc"
-formats); arbitrary amounts zero bytes (for padding) can be added
-between members.
+compressed and/or uncompressed cpio archives (in the "newc" format),
+with arbitrary amount of zero-byte padding between members.
 
 The cpio "TRAILER!!!" entry (cpio end-of-archive) is optional, but is
 not ignored; see "handling of hard links" below.
@@ -55,7 +54,7 @@ by the ASCII string "000012ac"):
 ============= ================== ==============================================
 Field name    Field size	 Meaning
 ============= ================== ==============================================
-c_magic	      6 bytes		 The string "070701" or "070702"
+c_magic	      6 bytes		 The string "070701"
 c_ino	      8 bytes		 File inode number
 c_mode	      8 bytes		 File mode and permissions
 c_uid	      8 bytes		 File uid
@@ -68,8 +67,7 @@ c_min	      8 bytes		 Minor part of file device number
 c_rmaj	      8 bytes		 Major part of device node reference
 c_rmin	      8 bytes		 Minor part of device node reference
 c_namesize    8 bytes		 Length of filename, including final \0
-c_chksum      8 bytes		 Checksum of data field if c_magic is 070702;
-				 otherwise zero
+c_chksum      8 bytes		 Ignored; reserved for unsupported "crc" format
 ============= ================== ==============================================
 
 The c_mode field matches the contents of st_mode returned by stat(2)
@@ -78,12 +76,6 @@ on Linux, and encodes the file type and file permissions.
 The c_filesize should be zero for any file which is not a regular file
 or symlink.
 
-The c_chksum field contains a simple 32-bit unsigned sum of all the
-bytes in the data field.  cpio(1) refers to this as "crc", which is
-clearly incorrect (a cyclic redundancy check is a different and
-significantly stronger integrity check), however, this is the
-algorithm used.
-
 If the filename is "TRAILER!!!" this is actually an end-of-archive
 marker; the c_filesize for an end-of-archive marker must be zero.
 
-- 
2.31.1

[PATCH v4 4/4] 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. Contrary to the 889d51a10712 commit message, the mtime
preservation behaviour is unconditional.

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.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/Kconfig           | 10 +++++++++
 init/initramfs.c       | 48 +++-------------------------------------
 init/initramfs_mtime.h | 50 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 45 deletions(-)
 create mode 100644 init/initramfs_mtime.h

diff --git a/init/Kconfig b/init/Kconfig
index 21b1f4870c80..6c2b919f81a5 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1352,6 +1352,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 44e692ae4646..7329657e233a 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -17,6 +17,8 @@
 #include <linux/init_syscalls.h>
 #include <linux/umh.h>
 
+#include "initramfs_mtime.h"
+
 static ssize_t __init xwrite(struct file *file, const char *p, size_t count,
 		loff_t *pos)
 {
@@ -116,46 +118,6 @@ static void __init free_hash(void)
 	}
 }
 
-static long __init do_utime(char *filename, time64_t mtime)
-{
-	struct timespec64 t[2];
-
-	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 __initdata LIST_HEAD(dir_list);
-struct dir_entry {
-	struct list_head list;
-	char *name;
-	time64_t mtime;
-};
-
-static void __init dir_add(const char *name, time64_t mtime)
-{
-	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
-	if (!de)
-		panic_show_mem("can't allocate dir_entry buffer");
-	INIT_LIST_HEAD(&de->list);
-	de->name = kstrdup(name, GFP_KERNEL);
-	de->mtime = mtime;
-	list_add(&de->list, &dir_list);
-}
-
-static void __init dir_utime(void)
-{
-	struct dir_entry *de, *tmp;
-	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);
-	}
-}
-
 static __initdata time64_t mtime;
 
 /* cpio header parsing */
@@ -379,14 +341,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;
diff --git a/init/initramfs_mtime.h b/init/initramfs_mtime.h
new file mode 100644
index 000000000000..fbd8757b34a9
--- /dev/null
+++ b/init/initramfs_mtime.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME
+static void __init do_utime(char *filename, time64_t mtime)
+{
+	struct timespec64 t[2] = { { .tv_sec = mtime }, { .tv_sec = mtime } };
+	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);
+struct dir_entry {
+	struct list_head list;
+	char *name;
+	time64_t mtime;
+};
+
+static void __init dir_add(const char *name, time64_t mtime)
+{
+	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
+	if (!de)
+		panic("can't allocate dir_entry buffer");
+	INIT_LIST_HEAD(&de->list);
+	de->name = kstrdup(name, GFP_KERNEL);
+	de->mtime = mtime;
+	list_add(&de->list, &dir_list);
+}
+
+static void __init dir_utime(void)
+{
+	struct dir_entry *de, *tmp;
+
+	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);
+	}
+}
+#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
-- 
2.31.1

Re: [PATCH v4 0/4] initramfs: "crc" cpio format and INITRAMFS_PRESERVE_MTIME

[Martin Wilck]

On Wed, 2021-11-10 at 13:38 +0100, David Disseldorp wrote:
> This patchset does some minor refactoring of cpio header magic
> checking
> and corrects documentation.
> Patch 4/4 allows cpio entry mtime preservation to be disabled via a
> new
> INITRAMFS_PRESERVE_MTIME Kconfig option.
> 
> 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
> 
>  .../early-userspace/buffer-format.rst         | 24 +++-----
>  init/Kconfig                                  | 10 ++++
>  init/initramfs.c                              | 57 +++--------------
> --
>  init/initramfs_mtime.h                        | 50 ++++++++++++++++
>  4 files changed, 75 insertions(+), 66 deletions(-)
> 

For the set:

Reviewed-by: Martin Wilck <mwilck@suse.com>

Re: [PATCH v4 2/4] initramfs: print helpful cpio error on "crc" magic

[Matthew Wilcox]

On Wed, Nov 10, 2021 at 01:38:48PM +0100, David Disseldorp wrote:
> Contrary to the buffer-format.rst documentation, initramfs cpio
> extraction does not support "crc" archives, which carry "070702"
> header magic. Make it a little clearer that "newc" (magic="070701") is
> the only supported cpio format, by extending the POSIX.1 ASCII
> (magic="070707") specific error message to also cover "crc" magic.

Wouldn't it be easier to just add support?  As far as I can tell from
looking at documentation, the "crc" format is the same as newc, except
that it uses some reserved bits to store the crc.  Since we ignore those
bits, we could just check for either 070701 or 070702.

> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  init/initramfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/init/initramfs.c b/init/initramfs.c
> index 2f79b3ec0b40..44e692ae4646 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> @@ -258,7 +258,7 @@ static int __init do_collect(void)
>  static int __init do_header(void)
>  {
>  	if (memcmp(collected, "070701", 6)) {
> -		if (memcmp(collected, "070707", 6) == 0)
> +		if (memcmp(collected, "0707", 4) == 0)
>  			error("incorrect cpio method used: use -H newc option");
>  		else
>  			error("no cpio magic");
> -- 
> 2.31.1
> 

Re: [PATCH v4 2/4] initramfs: print helpful cpio error on "crc" magic

[David Disseldorp]

On Wed, 10 Nov 2021 17:30:54 +0000, Matthew Wilcox wrote:

> On Wed, Nov 10, 2021 at 01:38:48PM +0100, David Disseldorp wrote:
> > Contrary to the buffer-format.rst documentation, initramfs cpio
> > extraction does not support "crc" archives, which carry "070702"
> > header magic. Make it a little clearer that "newc" (magic="070701") is
> > the only supported cpio format, by extending the POSIX.1 ASCII
> > (magic="070707") specific error message to also cover "crc" magic.  
> 
> Wouldn't it be easier to just add support?

Well, no, this patch already exists. :-)

> As far as I can tell from
> looking at documentation, the "crc" format is the same as newc, except
> that it uses some reserved bits to store the crc.  Since we ignore those
> bits, we could just check for either 070701 or 070702.

Sure, it'd be pretty straightforward to implement "crc" format support,
but I'm not sure how useful a 32-bit checksum would be... If we're going
down this route, wouldn't proper IMA/EVM support make sense via some new
cpio variant with space for the attributes (or as header/trailer like
bootconfig)?
cc'ing Jeff, as I seem to recall him mentioning some work in this area.

Cheers, David