| Message ID | 20230320040839.660475-2-bgray@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] initramfs: Check negative timestamp to prevent broken cpio archive | expand |
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>
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 >
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
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.
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
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(-)