[RFC PATCH 0/6] initramfs: reduce buffer footprint

[RFC PATCH 0/6] initramfs: reduce buffer footprint

[David Disseldorp]

There are a number of stack, heap and data-segment buffers which are
unnecessary for initramfs unpacking. This patchset attempts to remove
them by:
- parsing cpio hex strings in place, instead of copying them for
  nul-termination. (Patches 1 & 2).
- reusing a single heap buffer for cpio header, file and symlink paths,
  instead of three separate buffers. (Patches 3 & 4).
- reusing the heap-allocated cpio buffer across both builtin and
  bootloader-provided unpack attempts. (Patch 5).
- reusing the heap-allocated cpio buffer for error messages on
  FSM-exit, instead of a data-segment buffer. (Patch 6).

I've flagged this as an RFC as I'd like to improve the commit messages
and also provide more thorough testing, likely via kunit / kselftest
integration.

Feedback appreciated.

David Disseldorp (6):
      vsprintf: add simple_strntoul
      initramfs: avoid memcpy for hex header fields
      initramfs: remove extra symlink path buffer
      initramfs: merge header_buf and name_buf
      initramfs: reuse buf for built-in and bootloader initramfs
      initramfs: avoid static buffer for error message

 include/linux/kstrtox.h |  1 +
 init/initramfs.c        | 50 +++++++++++++++++++++--------------------
 lib/vsprintf.c          |  7 ++++++
 3 files changed, 34 insertions(+), 24 deletions(-)

[RFC PATCH 1/6] vsprintf: add simple_strntoul

[David Disseldorp]

cpio extraction currently does a memcpy to ensure that the archive hex
fields are null terminated for simple_strtoul(). simple_strntoul() will
allow us to avoid the memcpy.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 include/linux/kstrtox.h | 1 +
 lib/vsprintf.c          | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/include/linux/kstrtox.h b/include/linux/kstrtox.h
index 7fcf29a4e0de4..6ea897222af1d 100644
--- a/include/linux/kstrtox.h
+++ b/include/linux/kstrtox.h
@@ -143,6 +143,7 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_t
  */
 
 extern unsigned long simple_strtoul(const char *,char **,unsigned int);
+extern unsigned long simple_strntoul(const char *,char **,unsigned int,size_t);
 extern long simple_strtol(const char *,char **,unsigned int);
 extern unsigned long long simple_strtoull(const char *,char **,unsigned int);
 extern long long simple_strtoll(const char *,char **,unsigned int);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index c5e2ec9303c5d..32eacaae97990 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -114,6 +114,13 @@ unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base)
 }
 EXPORT_SYMBOL(simple_strtoul);
 
+unsigned long simple_strntoul(const char *cp, char **endp, unsigned int base,
+			      size_t max_chars)
+{
+	return simple_strntoull(cp, endp, base, max_chars);
+}
+EXPORT_SYMBOL(simple_strntoul);
+
 /**
  * simple_strtol - convert a string to a signed long
  * @cp: The start of the string
-- 
2.43.0

[RFC PATCH 2/6] initramfs: avoid memcpy for hex header fields

[David Disseldorp]

newc/crc cpio headers contain a bunch of 8-character hexadecimal fields
which we convert via simple_strtoul(), following memcpy() into a
zero-terminated stack buffer. The new simple_strntoul() helper allows us
to pass in max_chars=8 to avoid zero-termination and memcpy().

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/initramfs.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index bc911e466d5bb..c35600d49a50a 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -188,14 +188,11 @@ static __initdata u32 hdr_csum;
 static void __init parse_header(char *s)
 {
 	unsigned long parsed[13];
-	char buf[9];
 	int i;
 
-	buf[8] = '\0';
-	for (i = 0, s += 6; i < 13; i++, s += 8) {
-		memcpy(buf, s, 8);
-		parsed[i] = simple_strtoul(buf, NULL, 16);
-	}
+	for (i = 0, s += 6; i < 13; i++, s += 8)
+		parsed[i] = simple_strntoul(s, NULL, 16, 8);
+
 	ino = parsed[0];
 	mode = parsed[1];
 	uid = parsed[2];
-- 
2.43.0

[RFC PATCH 3/6] initramfs: remove extra symlink path buffer

[David Disseldorp]

A (newc/crc) cpio entry with mode.S_IFLNK set carries the symlink target
in the cpio data segment, following the padded name_len sized file
path. symlink_buf is heap-allocated for staging both file path and
symlink target, while name_buf is additionally allocated for staging the
path for non-symlink cpio entries.

Separate symlink / non-symlink buffers are unnecessary, so just extend
the size of name_buf and use it for both.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/initramfs.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index c35600d49a50a..4b7f20fbf23ae 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -249,7 +249,7 @@ static void __init read_into(char *buf, unsigned size, enum state next)
 	}
 }
 
-static __initdata char *header_buf, *symlink_buf, *name_buf;
+static __initdata char *header_buf, *name_buf;
 
 static int __init do_start(void)
 {
@@ -293,7 +293,7 @@ static int __init do_header(void)
 	if (S_ISLNK(mode)) {
 		if (body_len > PATH_MAX)
 			return 0;
-		collect = collected = symlink_buf;
+		collect = collected = name_buf;
 		remains = N_ALIGN(name_len) + body_len;
 		next_state = GotSymlink;
 		state = Collect;
@@ -487,10 +487,9 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
 	static __initdata char msg_buf[64];
 
 	header_buf = kmalloc(110, GFP_KERNEL);
-	symlink_buf = kmalloc(PATH_MAX + N_ALIGN(PATH_MAX) + 1, GFP_KERNEL);
-	name_buf = kmalloc(N_ALIGN(PATH_MAX), GFP_KERNEL);
-
-	if (!header_buf || !symlink_buf || !name_buf)
+	/* 2x PATH_MAX as @name_buf is also used for staging symlink targets */
+	name_buf = kmalloc(N_ALIGN(PATH_MAX) + PATH_MAX + 1, GFP_KERNEL);
+	if (!header_buf || !name_buf)
 		panic_show_mem("can't allocate buffers");
 
 	state = Start;
@@ -536,7 +535,6 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
 	}
 	dir_utime();
 	kfree(name_buf);
-	kfree(symlink_buf);
 	kfree(header_buf);
 	return message;
 }
-- 
2.43.0

[RFC PATCH 4/6] initramfs: merge header_buf and name_buf

[David Disseldorp]

header_buf is only used in FSM states up to GotHeader, while name_buf is
only used in states following GotHeader (Collect is shared, but the
collect pointer tracks each buffer). These buffers can therefore be
combined into a single cpio_buf, which is used for both header and file
name storage.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/initramfs.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 4b7f20fbf23ae..c0aea453d2792 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -249,11 +249,11 @@ static void __init read_into(char *buf, unsigned size, enum state next)
 	}
 }
 
-static __initdata char *header_buf, *name_buf;
+static __initdata char *cpio_buf;
 
 static int __init do_start(void)
 {
-	read_into(header_buf, 110, GotHeader);
+	read_into(cpio_buf, 110, GotHeader);
 	return 0;
 }
 
@@ -293,14 +293,14 @@ static int __init do_header(void)
 	if (S_ISLNK(mode)) {
 		if (body_len > PATH_MAX)
 			return 0;
-		collect = collected = name_buf;
+		collect = collected = cpio_buf;
 		remains = N_ALIGN(name_len) + body_len;
 		next_state = GotSymlink;
 		state = Collect;
 		return 0;
 	}
 	if (S_ISREG(mode) || !body_len)
-		read_into(name_buf, N_ALIGN(name_len), GotName);
+		read_into(cpio_buf, N_ALIGN(name_len), GotName);
 	return 0;
 }
 
@@ -486,10 +486,14 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
 	const char *compress_name;
 	static __initdata char msg_buf[64];
 
-	header_buf = kmalloc(110, GFP_KERNEL);
-	/* 2x PATH_MAX as @name_buf is also used for staging symlink targets */
-	name_buf = kmalloc(N_ALIGN(PATH_MAX) + PATH_MAX + 1, GFP_KERNEL);
-	if (!header_buf || !name_buf)
+	/*
+	 * @cpio_buf is first used for staging the 110 byte newc/crc cpio
+	 * header, after which parse_header() converts and stashes fields into
+	 * corresponding types. The same buffer is then reused for file path
+	 * staging. 2 x PATH_MAX covers any possible symlink target.
+	 */
+	cpio_buf = kmalloc(N_ALIGN(PATH_MAX) + PATH_MAX + 1, GFP_KERNEL);
+	if (!cpio_buf)
 		panic_show_mem("can't allocate buffers");
 
 	state = Start;
@@ -534,8 +538,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
 		len -= my_inptr;
 	}
 	dir_utime();
-	kfree(name_buf);
-	kfree(header_buf);
+	kfree(cpio_buf);
 	return message;
 }
 
-- 
2.43.0

[RFC PATCH 5/6] initramfs: reuse buf for built-in and bootloader initramfs

[David Disseldorp]

cpio_buf is allocated and freed within unpack_to_rootfs(), while the
do_populate_rootfs() parent function may call it twice to unpack both
built-in and bootloader-provided cpio archives.
Move allocation out into the caller and reuse cpio_buf.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/initramfs.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index c0aea453d2792..7594a176d8d91 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -486,16 +486,6 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
 	const char *compress_name;
 	static __initdata char msg_buf[64];
 
-	/*
-	 * @cpio_buf is first used for staging the 110 byte newc/crc cpio
-	 * header, after which parse_header() converts and stashes fields into
-	 * corresponding types. The same buffer is then reused for file path
-	 * staging. 2 x PATH_MAX covers any possible symlink target.
-	 */
-	cpio_buf = kmalloc(N_ALIGN(PATH_MAX) + PATH_MAX + 1, GFP_KERNEL);
-	if (!cpio_buf)
-		panic_show_mem("can't allocate buffers");
-
 	state = Start;
 	this_header = 0;
 	message = NULL;
@@ -538,7 +528,6 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
 		len -= my_inptr;
 	}
 	dir_utime();
-	kfree(cpio_buf);
 	return message;
 }
 
@@ -688,8 +677,20 @@ static void __init populate_initrd_image(char *err)
 
 static void __init do_populate_rootfs(void *unused, async_cookie_t cookie)
 {
+	char *err;
+
+	/*
+	 * @cpio_buf is first used for staging the 110 byte newc/crc cpio
+	 * header, after which parse_header() converts and stashes fields into
+	 * corresponding types. The same buffer is then reused for file path
+	 * staging. 2 x PATH_MAX covers any possible symlink target.
+	 */
+	cpio_buf = kmalloc(N_ALIGN(PATH_MAX) + PATH_MAX + 1, GFP_KERNEL);
+	if (!cpio_buf)
+		panic_show_mem("can't allocate buffers");
+
 	/* Load the built in initramfs */
-	char *err = unpack_to_rootfs(__initramfs_start, __initramfs_size);
+	err = unpack_to_rootfs(__initramfs_start, __initramfs_size);
 	if (err)
 		panic_show_mem("%s", err); /* Failed to decompress INTERNAL initramfs */
 
@@ -711,6 +712,7 @@ static void __init do_populate_rootfs(void *unused, async_cookie_t cookie)
 	}
 
 done:
+	kfree(cpio_buf);
 	security_initramfs_populated();
 
 	/*
-- 
2.43.0

[RFC PATCH 6/6] initramfs: avoid static buffer for error message

[David Disseldorp]

With cpio_buf now allocated / freed by the unpack_to_rootfs() caller,
FSM-exit error messages can now be stashed in cpio_buf instead of in a
static buffer.

Before:
   text    data     bss     dec     hex filename
   7423    1062       8    8493    212d ./init/initramfs.o

After:
   text    data     bss     dec     hex filename
   7423     966       8    8397    20cd ./init/initramfs.o

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 init/initramfs.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 7594a176d8d91..815a5daaa27ce 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -484,7 +484,6 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
 	long written;
 	decompress_fn decompress;
 	const char *compress_name;
-	static __initdata char msg_buf[64];
 
 	state = Start;
 	this_header = 0;
@@ -514,10 +513,12 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
 				error("decompressor failed");
 		} else if (compress_name) {
 			if (!message) {
-				snprintf(msg_buf, sizeof msg_buf,
+				/* FSM exit: cpio_buf reuse is safe */
+				snprintf(cpio_buf,
+					 N_ALIGN(PATH_MAX) + PATH_MAX + 1,
 					 "compression method %s not configured",
 					 compress_name);
-				message = msg_buf;
+				message = cpio_buf;
 			}
 		} else
 			error("invalid magic at start of compressed archive");
@@ -684,6 +685,7 @@ static void __init do_populate_rootfs(void *unused, async_cookie_t cookie)
 	 * header, after which parse_header() converts and stashes fields into
 	 * corresponding types. The same buffer is then reused for file path
 	 * staging. 2 x PATH_MAX covers any possible symlink target.
+	 * On error, @err may point to a @cpio_buf backed error message.
 	 */
 	cpio_buf = kmalloc(N_ALIGN(PATH_MAX) + PATH_MAX + 1, GFP_KERNEL);
 	if (!cpio_buf)
-- 
2.43.0

Re: [RFC PATCH 0/6] initramfs: reduce buffer footprint

[Al Viro]

On Tue, Oct 15, 2024 at 01:11:57PM +0000, David Disseldorp wrote:
> There are a number of stack, heap and data-segment buffers which are
> unnecessary for initramfs unpacking. This patchset attempts to remove
> them by:
> - parsing cpio hex strings in place, instead of copying them for
>   nul-termination. (Patches 1 & 2).
> - reusing a single heap buffer for cpio header, file and symlink paths,
>   instead of three separate buffers. (Patches 3 & 4).
> - reusing the heap-allocated cpio buffer across both builtin and
>   bootloader-provided unpack attempts. (Patch 5).
> - reusing the heap-allocated cpio buffer for error messages on
>   FSM-exit, instead of a data-segment buffer. (Patch 6).
> 
> I've flagged this as an RFC as I'd like to improve the commit messages
> and also provide more thorough testing, likely via kunit / kselftest
> integration.

Umm...  An obvious question: what's the point?  Reducing the amount of
temporary allocations (and not particularly large ones, at that) done
during early boot and freed before we run anything in user mode?

Re: [RFC PATCH 0/6] initramfs: reduce buffer footprint

[David Disseldorp]

On Wed, 16 Oct 2024 00:34:15 +0100, Al Viro wrote:

> On Tue, Oct 15, 2024 at 01:11:57PM +0000, David Disseldorp wrote:
> > There are a number of stack, heap and data-segment buffers which are
> > unnecessary for initramfs unpacking. This patchset attempts to remove
> > them by:
> > - parsing cpio hex strings in place, instead of copying them for
> >   nul-termination. (Patches 1 & 2).
> > - reusing a single heap buffer for cpio header, file and symlink paths,
> >   instead of three separate buffers. (Patches 3 & 4).
> > - reusing the heap-allocated cpio buffer across both builtin and
> >   bootloader-provided unpack attempts. (Patch 5).
> > - reusing the heap-allocated cpio buffer for error messages on
> >   FSM-exit, instead of a data-segment buffer. (Patch 6).
> > 
> > I've flagged this as an RFC as I'd like to improve the commit messages
> > and also provide more thorough testing, likely via kunit / kselftest
> > integration.  
> 
> Umm...  An obvious question: what's the point?  Reducing the amount of
> temporary allocations (and not particularly large ones, at that) done
> during early boot and freed before we run anything in user mode?

"reduce buffer footprint" is a bad title... My initial motivation was to
improve initramfs unpack error reporting (still WIP), following a
downstream bug report.
Patches 1 & 2 avoid 13 memcpy() calls for every initramfs entry and IMO
leave the code more readable, so should be justified once I have
profiling data.
Patches 3-5 remove five extra kmalloc() calls when booting with built-in
and bootloader initramfses. I doubt it'll be visible in profiling, but
they allow for buffer reuse for dynamic error messages instead of
sprinkling data-segment buffers around, like the one removed in patch 6.