[PATCH 1/2] initramfs: Check negative timestamp to prevent broken cpio archive

[PATCH 1/2] initramfs: Check negative timestamp to prevent broken cpio archive

[Benjamin Gray]

Similar to commit 4c9d410f32b3 ("initramfs: Check timestamp to prevent
broken cpio archive"), except asserts that the timestamp is
non-negative. This can happen when the KBUILD_BUILD_TIMESTAMP is a value
before UNIX epoch, which may be set when making reproducible builds that
don't want to look like they use a valid date.

While support for dates before 1970 might not be supported, this is more
about preventing undetected CPIO corruption. The printf's use a minimum
length format specifier, and will happily make the field longer than 8
characters if they need to.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

---

Ran into this when setting KBUILD_BUILD_TIMESTAMP=0000-01-01. The kernel
builds and boots to an initramfs just fine, but inexplicably failed to
load any root disks. It was a pain to debug, because the first sign of
an issue was so deep into the boot sequence.
---
 usr/gen_init_cpio.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/usr/gen_init_cpio.c b/usr/gen_init_cpio.c
index ee01e40e8bc6..61230532fef1 100644
--- a/usr/gen_init_cpio.c
+++ b/usr/gen_init_cpio.c
@@ -353,6 +353,12 @@ static int cpio_mkfile(const char *name, const char *location,
 		buf.st_mtime = 0xffffffff;
 	}
 
+	if (buf.st_mtime < 0) {
+		fprintf(stderr, "%s: Timestamp negative, clipping.\n",
+			location);
+		buf.st_mtime = 0;
+	}
+
 	if (buf.st_size > 0xffffffff) {
 		fprintf(stderr, "%s: Size exceeds maximum cpio file size\n",
 			location);
@@ -602,10 +608,10 @@ int main (int argc, char *argv[])
 	/*
 	 * Timestamps after 2106-02-07 06:28:15 UTC have an ascii hex time_t
 	 * representation that exceeds 8 chars and breaks the cpio header
-	 * specification.
+	 * specification. Negative timestamps similarly exceed 8 chars.
 	 */
-	if (default_mtime > 0xffffffff) {
-		fprintf(stderr, "ERROR: Timestamp too large for cpio format\n");
+	if (default_mtime > 0xffffffff || default_mtime < 0) {
+		fprintf(stderr, "ERROR: Timestamp out of range for cpio format\n");
 		exit(1);
 	}
 

base-commit: 065ffaee73892e8a3629b4cfbe635697807a3c6f
-- 
2.39.2

[PATCH 2/2] initramfs: Encode dependency on KBUILD_BUILD_TIMESTAMP

[Benjamin Gray]

gen_initramfs.sh has an internal dependency on KBUILD_BUILD_TIMESTAMP
for generating file mtimes that is not exposed to make, so changing
KBUILD_BUILD_TIMESTAMP will not trigger a rebuild of the archive.

Declare the mtime date as a new parameter to gen_initramfs.sh to encode
KBUILD_BUILD_TIMESTAMP in the shell command, thereby making make aware
of the dependency.

It will rebuild if KBUILD_BUILD_TIMESTAMP changes or is newly set/unset.
It will _not_ rebuild if KBUILD_BUILD_TIMESTAMP is unset before and
after. This should be fine for anyone who doesn't care about setting
specific build times in the first place.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

---

Ran into this while debugging the issue in the first patch. Made for a
very perplexing debug session before we worked out the state wasn't
being rebuilt.
---
 usr/Makefile         |  2 ++
 usr/gen_initramfs.sh | 16 +++++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/usr/Makefile b/usr/Makefile
index 59d9e8b07a01..2aa386cf48d6 100644
--- a/usr/Makefile
+++ b/usr/Makefile
@@ -64,6 +64,7 @@ quiet_cmd_initfs = GEN     $@
 	$(CONFIG_SHELL) $< -o $@ -l $(obj)/.initramfs_data.cpio.d \
 	$(if $(CONFIG_INITRAMFS_ROOT_UID), -u $(CONFIG_INITRAMFS_ROOT_UID)) \
 	$(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID)) \
+	$(if $(KBUILD_BUILD_TIMESTAMP), -d $(KBUILD_BUILD_TIMESTAMP)) \
 	$(ramfs-input)
 
 # We rebuild initramfs_data.cpio if:
@@ -71,6 +72,7 @@ quiet_cmd_initfs = GEN     $@
 # 2) There are changes in which files are included (added or deleted)
 # 3) If gen_init_cpio are newer than initramfs_data.cpio
 # 4) Arguments to gen_initramfs.sh changes
+# 5) KBUILD_BUILD_TIMESTAMP is changed
 $(obj)/initramfs_data.cpio: $(src)/gen_initramfs.sh $(obj)/gen_init_cpio $(deps_initramfs) FORCE
 	$(call if_changed,initfs)
 
diff --git a/usr/gen_initramfs.sh b/usr/gen_initramfs.sh
index 63476bb70b41..14b5782f961a 100755
--- a/usr/gen_initramfs.sh
+++ b/usr/gen_initramfs.sh
@@ -23,6 +23,7 @@ $0 [-o <file>] [-l <dep_list>] [-u <uid>] [-g <gid>] {-d | <cpio_source>} ...
 	-g <gid>       Group ID to map to group ID 0 (root).
 		       <gid> is only meaningful if <cpio_source> is a
 		       directory.  "squash" forces all files to gid 0.
+	-d <date>      Use date for all file mtime values
 	<cpio_source>  File list or directory for cpio archive.
 		       If <cpio_source> is a .cpio file it will be used
 		       as direct input to initramfs.
@@ -190,6 +191,7 @@ prog=$0
 root_uid=0
 root_gid=0
 dep_list=
+timestamp=
 cpio_list=$(mktemp ${TMPDIR:-/tmp}/cpiolist.XXXXXX)
 output="/dev/stdout"
 
@@ -218,6 +220,13 @@ while [ $# -gt 0 ]; do
 			[ "$root_gid" = "-1" ] && root_gid=$(id -g || echo 0)
 			shift
 			;;
+		"-d")	# date for file mtimes
+			timestamp="$(date -d"$1" +%s || :)"
+			if test -n "$timestamp"; then
+				timestamp="-t $timestamp"
+			fi
+			shift
+			;;
 		"-h")
 			usage
 			exit 0
@@ -237,11 +246,4 @@ done
 
 # If output_file is set we will generate cpio archive
 # we are careful to delete tmp files
-timestamp=
-if test -n "$KBUILD_BUILD_TIMESTAMP"; then
-	timestamp="$(date -d"$KBUILD_BUILD_TIMESTAMP" +%s || :)"
-	if test -n "$timestamp"; then
-		timestamp="-t $timestamp"
-	fi
-fi
 usr/gen_init_cpio $timestamp $cpio_list > $output
-- 
2.39.2

Re: [PATCH 1/2] initramfs: Check negative timestamp to prevent broken cpio archive

[Andrew Donnellan]

On Mon, 2023-03-20 at 15:08 +1100, Benjamin Gray wrote:
> Similar to commit 4c9d410f32b3 ("initramfs: Check timestamp to
> prevent
> broken cpio archive"), except asserts that the timestamp is
> non-negative. This can happen when the KBUILD_BUILD_TIMESTAMP is a
> value
> before UNIX epoch, which may be set when making reproducible builds
> that
> don't want to look like they use a valid date.
> 
> While support for dates before 1970 might not be supported, this is
> more
> about preventing undetected CPIO corruption. The printf's use a
> minimum
> length format specifier, and will happily make the field longer than
> 8
> characters if they need to.
> 
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
Tested-by: Andrew Donnellan <ajd@linux.ibm.com>


-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 2/2] initramfs: Encode dependency on KBUILD_BUILD_TIMESTAMP

[Andrew Donnellan]

On Mon, 2023-03-20 at 15:08 +1100, Benjamin Gray wrote:
> gen_initramfs.sh has an internal dependency on KBUILD_BUILD_TIMESTAMP
> for generating file mtimes that is not exposed to make, so changing
> KBUILD_BUILD_TIMESTAMP will not trigger a rebuild of the archive.
> 
> Declare the mtime date as a new parameter to gen_initramfs.sh to
> encode
> KBUILD_BUILD_TIMESTAMP in the shell command, thereby making make
> aware
> of the dependency.
> 
> It will rebuild if KBUILD_BUILD_TIMESTAMP changes or is newly
> set/unset.
> It will _not_ rebuild if KBUILD_BUILD_TIMESTAMP is unset before and
> after. This should be fine for anyone who doesn't care about setting
> specific build times in the first place.
> 
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>

Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com>
Tested-by: Andrew Donnellan <ajd@linux.ibm.com>

-- 
Andrew Donnellan    OzLabs, ADL Canberra
ajd@linux.ibm.com   IBM Australia Limited

Re: [PATCH 2/2] initramfs: Encode dependency on KBUILD_BUILD_TIMESTAMP

[Masahiro Yamada]

On Mon, Mar 20, 2023 at 1:09 PM Benjamin Gray <bgray@linux.ibm.com> wrote:
>
> gen_initramfs.sh has an internal dependency on KBUILD_BUILD_TIMESTAMP
> for generating file mtimes that is not exposed to make, so changing
> KBUILD_BUILD_TIMESTAMP will not trigger a rebuild of the archive.
>
> Declare the mtime date as a new parameter to gen_initramfs.sh to encode
> KBUILD_BUILD_TIMESTAMP in the shell command, thereby making make aware
> of the dependency.
>
> It will rebuild if KBUILD_BUILD_TIMESTAMP changes or is newly set/unset.
> It will _not_ rebuild if KBUILD_BUILD_TIMESTAMP is unset before and
> after. This should be fine for anyone who doesn't care about setting
> specific build times in the first place.
>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
>
> ---
>
> Ran into this while debugging the issue in the first patch. Made for a
> very perplexing debug session before we worked out the state wasn't
> being rebuilt.
> ---
>  usr/Makefile         |  2 ++
>  usr/gen_initramfs.sh | 16 +++++++++-------
>  2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/usr/Makefile b/usr/Makefile
> index 59d9e8b07a01..2aa386cf48d6 100644
> --- a/usr/Makefile
> +++ b/usr/Makefile
> @@ -64,6 +64,7 @@ quiet_cmd_initfs = GEN     $@
>         $(CONFIG_SHELL) $< -o $@ -l $(obj)/.initramfs_data.cpio.d \
>         $(if $(CONFIG_INITRAMFS_ROOT_UID), -u $(CONFIG_INITRAMFS_ROOT_UID)) \
>         $(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID)) \
> +       $(if $(KBUILD_BUILD_TIMESTAMP), -d $(KBUILD_BUILD_TIMESTAMP)) \
>         $(ramfs-input)



If I apply this patch, I see a build error.


$ make -j16 KBUILD_BUILD_TIMESTAMP="$(date)"
[snip]
  GEN     usr/initramfs_data.cpio
  usr/gen_initramfs.sh: Cannot open 'Apr'
make[2]: *** [usr/Makefile:76: usr/initramfs_data.cpio] Error 1
make[1]: *** [scripts/Makefile.build:494: usr] Error 2
make[1]: *** Waiting for unfinished jobs....




Because KBUILD_BUILD_TIMESTAMP may contain spaces
depending on the format, init/Makefile and
kernel/gen_kheaders.sh surround the date with
double-quotes.


Double-quoting it in the same way fixes the issue.

$(if $(KBUILD_BUILD_TIMESTAMP), -d "$(KBUILD_BUILD_TIMESTAMP)") \







>  # We rebuild initramfs_data.cpio if:
> @@ -71,6 +72,7 @@ quiet_cmd_initfs = GEN     $@
>  # 2) There are changes in which files are included (added or deleted)
>  # 3) If gen_init_cpio are newer than initramfs_data.cpio
>  # 4) Arguments to gen_initramfs.sh changes
> +# 5) KBUILD_BUILD_TIMESTAMP is changed


This is unneeded because the change of KBUILD_BUILD_TIMESTAMP
is contained in

  4) Arguments to gen_initramfs.sh changes







>  $(obj)/initramfs_data.cpio: $(src)/gen_initramfs.sh $(obj)/gen_init_cpio $(deps_initramfs) FORCE
>         $(call if_changed,initfs)
>
> diff --git a/usr/gen_initramfs.sh b/usr/gen_initramfs.sh
> index 63476bb70b41..14b5782f961a 100755
> --- a/usr/gen_initramfs.sh
> +++ b/usr/gen_initramfs.sh
> @@ -23,6 +23,7 @@ $0 [-o <file>] [-l <dep_list>] [-u <uid>] [-g <gid>] {-d | <cpio_source>} ...
>         -g <gid>       Group ID to map to group ID 0 (root).
>                        <gid> is only meaningful if <cpio_source> is a
>                        directory.  "squash" forces all files to gid 0.
> +       -d <date>      Use date for all file mtime values
>         <cpio_source>  File list or directory for cpio archive.
>                        If <cpio_source> is a .cpio file it will be used
>                        as direct input to initramfs.
> @@ -190,6 +191,7 @@ prog=$0
>  root_uid=0
>  root_gid=0
>  dep_list=
> +timestamp=
>  cpio_list=$(mktemp ${TMPDIR:-/tmp}/cpiolist.XXXXXX)
>  output="/dev/stdout"
>
> @@ -218,6 +220,13 @@ while [ $# -gt 0 ]; do
>                         [ "$root_gid" = "-1" ] && root_gid=$(id -g || echo 0)
>                         shift
>                         ;;
> +               "-d")   # date for file mtimes
> +                       timestamp="$(date -d"$1" +%s || :)"
> +                       if test -n "$timestamp"; then
> +                               timestamp="-t $timestamp"
> +                       fi
> +                       shift
> +                       ;;
>                 "-h")
>                         usage
>                         exit 0
> @@ -237,11 +246,4 @@ done
>
>  # If output_file is set we will generate cpio archive
>  # we are careful to delete tmp files
> -timestamp=
> -if test -n "$KBUILD_BUILD_TIMESTAMP"; then
> -       timestamp="$(date -d"$KBUILD_BUILD_TIMESTAMP" +%s || :)"
> -       if test -n "$timestamp"; then
> -               timestamp="-t $timestamp"
> -       fi
> -fi
>  usr/gen_init_cpio $timestamp $cpio_list > $output
> --
> 2.39.2
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/2] initramfs: Check negative timestamp to prevent broken cpio archive

[Masahiro Yamada]

On Mon, Mar 20, 2023 at 1:09 PM Benjamin Gray <bgray@linux.ibm.com> wrote:
>
> Similar to commit 4c9d410f32b3 ("initramfs: Check timestamp to prevent
> broken cpio archive"), except asserts that the timestamp is
> non-negative. This can happen when the KBUILD_BUILD_TIMESTAMP is a value
> before UNIX epoch, which may be set when making reproducible builds that
> don't want to look like they use a valid date.
>
> While support for dates before 1970 might not be supported, this is more
> about preventing undetected CPIO corruption. The printf's use a minimum
> length format specifier, and will happily make the field longer than 8
> characters if they need to.
>
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
>
> ---

Applied to linux-kbuild.   (only 1/2)
Thanks.




>
> Ran into this when setting KBUILD_BUILD_TIMESTAMP=0000-01-01. The kernel
> builds and boots to an initramfs just fine, but inexplicably failed to
> load any root disks. It was a pain to debug, because the first sign of
> an issue was so deep into the boot sequence.
> ---
>  usr/gen_init_cpio.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/usr/gen_init_cpio.c b/usr/gen_init_cpio.c
> index ee01e40e8bc6..61230532fef1 100644
> --- a/usr/gen_init_cpio.c
> +++ b/usr/gen_init_cpio.c
> @@ -353,6 +353,12 @@ static int cpio_mkfile(const char *name, const char *location,
>                 buf.st_mtime = 0xffffffff;
>         }
>
> +       if (buf.st_mtime < 0) {
> +               fprintf(stderr, "%s: Timestamp negative, clipping.\n",
> +                       location);
> +               buf.st_mtime = 0;
> +       }
> +
>         if (buf.st_size > 0xffffffff) {
>                 fprintf(stderr, "%s: Size exceeds maximum cpio file size\n",
>                         location);
> @@ -602,10 +608,10 @@ int main (int argc, char *argv[])
>         /*
>          * Timestamps after 2106-02-07 06:28:15 UTC have an ascii hex time_t
>          * representation that exceeds 8 chars and breaks the cpio header
> -        * specification.
> +        * specification. Negative timestamps similarly exceed 8 chars.
>          */
> -       if (default_mtime > 0xffffffff) {
> -               fprintf(stderr, "ERROR: Timestamp too large for cpio format\n");
> +       if (default_mtime > 0xffffffff || default_mtime < 0) {
> +               fprintf(stderr, "ERROR: Timestamp out of range for cpio format\n");
>                 exit(1);
>         }
>
>
> base-commit: 065ffaee73892e8a3629b4cfbe635697807a3c6f
> --
> 2.39.2
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] initramfs: Encode dependency on KBUILD_BUILD_TIMESTAMP

[Masahiro Yamada]

Hi Benjamin,


I like this patch.

Will you send v2 with fixes I pointed out?
(add double-quotes and remove the unneeded comment)


On Sun, Apr 16, 2023 at 1:06 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Mon, Mar 20, 2023 at 1:09 PM Benjamin Gray <bgray@linux.ibm.com> wrote:
> >
> > gen_initramfs.sh has an internal dependency on KBUILD_BUILD_TIMESTAMP
> > for generating file mtimes that is not exposed to make, so changing
> > KBUILD_BUILD_TIMESTAMP will not trigger a rebuild of the archive.
> >
> > Declare the mtime date as a new parameter to gen_initramfs.sh to encode
> > KBUILD_BUILD_TIMESTAMP in the shell command, thereby making make aware
> > of the dependency.
> >
> > It will rebuild if KBUILD_BUILD_TIMESTAMP changes or is newly set/unset.
> > It will _not_ rebuild if KBUILD_BUILD_TIMESTAMP is unset before and
> > after. This should be fine for anyone who doesn't care about setting
> > specific build times in the first place.
> >
> > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> >
> > ---
> >
> > Ran into this while debugging the issue in the first patch. Made for a
> > very perplexing debug session before we worked out the state wasn't
> > being rebuilt.
> > ---
> >  usr/Makefile         |  2 ++
> >  usr/gen_initramfs.sh | 16 +++++++++-------
> >  2 files changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/usr/Makefile b/usr/Makefile
> > index 59d9e8b07a01..2aa386cf48d6 100644
> > --- a/usr/Makefile
> > +++ b/usr/Makefile
> > @@ -64,6 +64,7 @@ quiet_cmd_initfs = GEN     $@
> >         $(CONFIG_SHELL) $< -o $@ -l $(obj)/.initramfs_data.cpio.d \
> >         $(if $(CONFIG_INITRAMFS_ROOT_UID), -u $(CONFIG_INITRAMFS_ROOT_UID)) \
> >         $(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID)) \
> > +       $(if $(KBUILD_BUILD_TIMESTAMP), -d $(KBUILD_BUILD_TIMESTAMP)) \
> >         $(ramfs-input)
>
>
>
> If I apply this patch, I see a build error.
>
>
> $ make -j16 KBUILD_BUILD_TIMESTAMP="$(date)"
> [snip]
>   GEN     usr/initramfs_data.cpio
>   usr/gen_initramfs.sh: Cannot open 'Apr'
> make[2]: *** [usr/Makefile:76: usr/initramfs_data.cpio] Error 1
> make[1]: *** [scripts/Makefile.build:494: usr] Error 2
> make[1]: *** Waiting for unfinished jobs....
>
>
>
>
> Because KBUILD_BUILD_TIMESTAMP may contain spaces
> depending on the format, init/Makefile and
> kernel/gen_kheaders.sh surround the date with
> double-quotes.
>
>
> Double-quoting it in the same way fixes the issue.
>
> $(if $(KBUILD_BUILD_TIMESTAMP), -d "$(KBUILD_BUILD_TIMESTAMP)") \
>
>
>
>
>
>
>
> >  # We rebuild initramfs_data.cpio if:
> > @@ -71,6 +72,7 @@ quiet_cmd_initfs = GEN     $@
> >  # 2) There are changes in which files are included (added or deleted)
> >  # 3) If gen_init_cpio are newer than initramfs_data.cpio
> >  # 4) Arguments to gen_initramfs.sh changes
> > +# 5) KBUILD_BUILD_TIMESTAMP is changed
>
>
> This is unneeded because the change of KBUILD_BUILD_TIMESTAMP
> is contained in
>
>   4) Arguments to gen_initramfs.sh changes
>
>
>
>
>
>
>
> >  $(obj)/initramfs_data.cpio: $(src)/gen_initramfs.sh $(obj)/gen_init_cpio $(deps_initramfs) FORCE
> >         $(call if_changed,initfs)
> >
> > diff --git a/usr/gen_initramfs.sh b/usr/gen_initramfs.sh
> > index 63476bb70b41..14b5782f961a 100755
> > --- a/usr/gen_initramfs.sh
> > +++ b/usr/gen_initramfs.sh
> > @@ -23,6 +23,7 @@ $0 [-o <file>] [-l <dep_list>] [-u <uid>] [-g <gid>] {-d | <cpio_source>} ...
> >         -g <gid>       Group ID to map to group ID 0 (root).
> >                        <gid> is only meaningful if <cpio_source> is a
> >                        directory.  "squash" forces all files to gid 0.
> > +       -d <date>      Use date for all file mtime values
> >         <cpio_source>  File list or directory for cpio archive.
> >                        If <cpio_source> is a .cpio file it will be used
> >                        as direct input to initramfs.
> > @@ -190,6 +191,7 @@ prog=$0
> >  root_uid=0
> >  root_gid=0
> >  dep_list=
> > +timestamp=
> >  cpio_list=$(mktemp ${TMPDIR:-/tmp}/cpiolist.XXXXXX)
> >  output="/dev/stdout"
> >
> > @@ -218,6 +220,13 @@ while [ $# -gt 0 ]; do
> >                         [ "$root_gid" = "-1" ] && root_gid=$(id -g || echo 0)
> >                         shift
> >                         ;;
> > +               "-d")   # date for file mtimes
> > +                       timestamp="$(date -d"$1" +%s || :)"
> > +                       if test -n "$timestamp"; then
> > +                               timestamp="-t $timestamp"
> > +                       fi
> > +                       shift
> > +                       ;;
> >                 "-h")
> >                         usage
> >                         exit 0
> > @@ -237,11 +246,4 @@ done
> >
> >  # If output_file is set we will generate cpio archive
> >  # we are careful to delete tmp files
> > -timestamp=
> > -if test -n "$KBUILD_BUILD_TIMESTAMP"; then
> > -       timestamp="$(date -d"$KBUILD_BUILD_TIMESTAMP" +%s || :)"
> > -       if test -n "$timestamp"; then
> > -               timestamp="-t $timestamp"
> > -       fi
> > -fi
> >  usr/gen_init_cpio $timestamp $cpio_list > $output
> > --
> > 2.39.2
> >
>
>
> --
> Best Regards
> Masahiro Yamada



--
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] initramfs: Encode dependency on KBUILD_BUILD_TIMESTAMP

[Benjamin Gray]

On Tue, 2023-06-06 at 13:14 +0900, Masahiro Yamada wrote:
> Hi Benjamin,
> 
> 
> I like this patch.
> 
> Will you send v2 with fixes I pointed out?
> (add double-quotes and remove the unneeded comment)
> 
> 
> On Sun, Apr 16, 2023 at 1:06 PM Masahiro Yamada
> <masahiroy@kernel.org> wrote:
> > 
> > On Mon, Mar 20, 2023 at 1:09 PM Benjamin Gray <bgray@linux.ibm.com>
> > wrote:
> > > 
> > > gen_initramfs.sh has an internal dependency on
> > > KBUILD_BUILD_TIMESTAMP
> > > for generating file mtimes that is not exposed to make, so
> > > changing
> > > KBUILD_BUILD_TIMESTAMP will not trigger a rebuild of the archive.
> > > 
> > > Declare the mtime date as a new parameter to gen_initramfs.sh to
> > > encode
> > > KBUILD_BUILD_TIMESTAMP in the shell command, thereby making make
> > > aware
> > > of the dependency.
> > > 
> > > It will rebuild if KBUILD_BUILD_TIMESTAMP changes or is newly
> > > set/unset.
> > > It will _not_ rebuild if KBUILD_BUILD_TIMESTAMP is unset before
> > > and
> > > after. This should be fine for anyone who doesn't care about
> > > setting
> > > specific build times in the first place.
> > > 
> > > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> > > 
> > > ---
> > > 
> > > Ran into this while debugging the issue in the first patch. Made
> > > for a
> > > very perplexing debug session before we worked out the state
> > > wasn't
> > > being rebuilt.
> > > ---
> > >  usr/Makefile         |  2 ++
> > >  usr/gen_initramfs.sh | 16 +++++++++-------
> > >  2 files changed, 11 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/usr/Makefile b/usr/Makefile
> > > index 59d9e8b07a01..2aa386cf48d6 100644
> > > --- a/usr/Makefile
> > > +++ b/usr/Makefile
> > > @@ -64,6 +64,7 @@ quiet_cmd_initfs = GEN     $@
> > >         $(CONFIG_SHELL) $< -o $@ -l $(obj)/.initramfs_data.cpio.d
> > > \
> > >         $(if $(CONFIG_INITRAMFS_ROOT_UID), -u
> > > $(CONFIG_INITRAMFS_ROOT_UID)) \
> > >         $(if $(CONFIG_INITRAMFS_ROOT_GID), -g
> > > $(CONFIG_INITRAMFS_ROOT_GID)) \
> > > +       $(if $(KBUILD_BUILD_TIMESTAMP), -d
> > > $(KBUILD_BUILD_TIMESTAMP)) \
> > >         $(ramfs-input)
> > 
> > 
> > 
> > If I apply this patch, I see a build error.
> > 
> > 
> > $ make -j16 KBUILD_BUILD_TIMESTAMP="$(date)"
> > [snip]
> >   GEN     usr/initramfs_data.cpio
> >   usr/gen_initramfs.sh: Cannot open 'Apr'
> > make[2]: *** [usr/Makefile:76: usr/initramfs_data.cpio] Error 1
> > make[1]: *** [scripts/Makefile.build:494: usr] Error 2
> > make[1]: *** Waiting for unfinished jobs....
> > 
> > 
> > 
> > 
> > Because KBUILD_BUILD_TIMESTAMP may contain spaces
> > depending on the format, init/Makefile and
> > kernel/gen_kheaders.sh surround the date with
> > double-quotes.
> > 
> > 
> > Double-quoting it in the same way fixes the issue.
> > 
> > $(if $(KBUILD_BUILD_TIMESTAMP), -d "$(KBUILD_BUILD_TIMESTAMP)") \
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > >  # We rebuild initramfs_data.cpio if:
> > > @@ -71,6 +72,7 @@ quiet_cmd_initfs = GEN     $@
> > >  # 2) There are changes in which files are included (added or
> > > deleted)
> > >  # 3) If gen_init_cpio are newer than initramfs_data.cpio
> > >  # 4) Arguments to gen_initramfs.sh changes
> > > +# 5) KBUILD_BUILD_TIMESTAMP is changed
> > 
> > 
> > This is unneeded because the change of KBUILD_BUILD_TIMESTAMP
> > is contained in
> > 
> >   4) Arguments to gen_initramfs.sh changes
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > >  $(obj)/initramfs_data.cpio: $(src)/gen_initramfs.sh
> > > $(obj)/gen_init_cpio $(deps_initramfs) FORCE
> > >         $(call if_changed,initfs)
> > > 
> > > diff --git a/usr/gen_initramfs.sh b/usr/gen_initramfs.sh
> > > index 63476bb70b41..14b5782f961a 100755
> > > --- a/usr/gen_initramfs.sh
> > > +++ b/usr/gen_initramfs.sh
> > > @@ -23,6 +23,7 @@ $0 [-o <file>] [-l <dep_list>] [-u <uid>] [-g
> > > <gid>] {-d | <cpio_source>} ...
> > >         -g <gid>       Group ID to map to group ID 0 (root).
> > >                        <gid> is only meaningful if <cpio_source>
> > > is a
> > >                        directory.  "squash" forces all files to
> > > gid 0.
> > > +       -d <date>      Use date for all file mtime values
> > >         <cpio_source>  File list or directory for cpio archive.
> > >                        If <cpio_source> is a .cpio file it will
> > > be used
> > >                        as direct input to initramfs.
> > > @@ -190,6 +191,7 @@ prog=$0
> > >  root_uid=0
> > >  root_gid=0
> > >  dep_list=
> > > +timestamp=
> > >  cpio_list=$(mktemp ${TMPDIR:-/tmp}/cpiolist.XXXXXX)
> > >  output="/dev/stdout"
> > > 
> > > @@ -218,6 +220,13 @@ while [ $# -gt 0 ]; do
> > >                         [ "$root_gid" = "-1" ] && root_gid=$(id -
> > > g || echo 0)
> > >                         shift
> > >                         ;;
> > > +               "-d")   # date for file mtimes
> > > +                       timestamp="$(date -d"$1" +%s || :)"
> > > +                       if test -n "$timestamp"; then
> > > +                               timestamp="-t $timestamp"
> > > +                       fi
> > > +                       shift
> > > +                       ;;
> > >                 "-h")
> > >                         usage
> > >                         exit 0
> > > @@ -237,11 +246,4 @@ done
> > > 
> > >  # If output_file is set we will generate cpio archive
> > >  # we are careful to delete tmp files
> > > -timestamp=
> > > -if test -n "$KBUILD_BUILD_TIMESTAMP"; then
> > > -       timestamp="$(date -d"$KBUILD_BUILD_TIMESTAMP" +%s || :)"
> > > -       if test -n "$timestamp"; then
> > > -               timestamp="-t $timestamp"
> > > -       fi
> > > -fi
> > >  usr/gen_init_cpio $timestamp $cpio_list > $output
> > > --
> > > 2.39.2
> > > 
> > 
> > 
> > --
> > Best Regards
> > Masahiro Yamada
> 
> 
> 
> --
> Best Regards
> Masahiro Yamada

Sorry, I forgot to follow up on this one. Sent the v2 now.