Re: [PATCH 3/3] fix race-cond between default and custom initrd

["Moessbauer, Felix" <felix.moessbauer@siemens.com>]

On Fri, 2023-02-17 at 12:55 +0100, Jan Kiszka wrote:
> On 17.02.23 11:01, Felix Moessbauer wrote:
> > This patch fixes a data race happening when building a custom
> > initrd.
> > Previously, both custom and default initrds were deployed to the
> > image
> > deploy dir. The race is fixed by conditionally deploying either the
> > custom or the default one. For that, we introduce a new variable
> > INITRD_DEPLOY_FILE which provides the name of the initrd in the
> > deploy
> > directory. The existing INITRD_IMAGE variable is defaulted to the
> > empty
> > string and used to control if a custom initrd is requrested. Only
> > if
> > this variable is empty, the default one is deployed.
> > 
> 
> So, if I understand this correctly, the whole change comes
> effectively
> without a change visible at recipe level IF users already overwrote
> INITRD_IMAGE (like isar-cip-core did). Still, this nicely enhanced or
> clarified semantic of INITRD_IMAGE should be documented as recipe-api
> change.

Will add that in the v2. Yes, currently cip-core is not affected, but
the efibootguard plugin should anyways be adapted to the new API, as
otherwise images without custom initrd are broken (cc Quirin).

> 
> And then we are lacking an in-tree test case for such a scenario.
> Some
> WIC image that consumes a custom initramfs. We only have
> mc:qemuamd64-bullseye:isar-initramfs in the test cases. Not a must-
> have
> to move forward with this improvements, but likely an important
> follow-up topic.

Yes, but the custom initramfs case is still broken on sd-boot and grub.
Unfortunately this is also not easy to fix, as ISAR does some
deliberate diversions from the OE pattern in the wic plugins (e.g. get
kernel and initrd from rootfs instead of deploy dir). My fear is that
there is much more broken but currently nobody tests for this (e.g. the
wic initrd parameter). Maybe Henning can have a look on how to fix
that.

Felix

> 
> Jan
> 
> > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> > ---
> >  meta/classes/image.bbclass | 24 ++++++++++++++----------
> >  scripts/start_vm           |  4 ++--
> >  testsuite/start_vm.py      |  2 +-
> >  3 files changed, 17 insertions(+), 13 deletions(-)
> > 
> > diff --git a/meta/classes/image.bbclass
> > b/meta/classes/image.bbclass
> > index e799d1d4..0d5a521e 100644
> > --- a/meta/classes/image.bbclass
> > +++ b/meta/classes/image.bbclass
> > @@ -3,7 +3,7 @@
> >  
> >  # Make workdir and stamps machine-specific without changing common
> > PN target
> >  WORKDIR = "${TMPDIR}/work/${DISTRO}-${DISTRO_ARCH}/${PN}-
> > ${MACHINE}/${PV}-${PR}"
> > -DEPLOYDIR = "${WORKDIR}/deploy-${PN}"
> > +DEPLOYDIR = "${WORKDIR}/deploy"
> >  STAMP = "${STAMPS_DIR}/${DISTRO}-${DISTRO_ARCH}/${PN}-
> > ${MACHINE}/${PV}-${PR}"
> >  STAMPCLEAN = "${STAMPS_DIR}/${DISTRO}-${DISTRO_ARCH}/${PN}-
> > ${MACHINE}/*-*"
> >  
> > @@ -22,8 +22,9 @@ IMAGE_INSTALL += "${KERNEL_IMAGE_PKG}"
> >  IMAGE_FULLNAME = "${PN}-${DISTRO}-${MACHINE}"
> >  
> >  # These variables are used by wic and start_vm
> > -KERNEL_IMAGE ?= "${IMAGE_FULLNAME}-${KERNEL_FILE}"
> > -INITRD_IMAGE ?= "${IMAGE_FULLNAME}-initrd.img"
> > +KERNEL_IMAGE = "${IMAGE_FULLNAME}-${KERNEL_FILE}"
> > +INITRD_IMAGE ?= ""
> > +INITRD_DEPLOY_FILE = "${@d.getVar('INITRD_IMAGE') or
> > '${IMAGE_FULLNAME}-initrd.img')"
> >  
> >  # This defines the deployed dtbs for reuse by imagers
> >  DTB_FILES ?= ""
> > @@ -353,7 +354,7 @@ EOF
> >  
> >  # Default kernel, initrd and dtb image deploy paths (inside
> > imager)
> >  KERNEL_IMG = "${PP_DEPLOY}/${KERNEL_IMAGE}"
> > -INITRD_IMG = "${PP_DEPLOY}/${INITRD_IMAGE}"
> > +INITRD_IMG = "${PP_DEPLOY}/${INITRD_DEPLOY_FILE}"
> >  # only one dtb file supported, pick the first
> >  DTB_IMG = "${PP_DEPLOY}/${@(d.getVar('DTB_FILES').split() or
> > [''])[0]}"
> >  
> > @@ -370,12 +371,15 @@ do_copy_boot_files() {
> >          sudo cat "$kernel" > "${DEPLOYDIR}/${KERNEL_IMAGE}"
> >      fi
> >  
> > -    initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> > -    if [ ! -f "$initrd" ]; then
> > -        initrd="$(realpath -q '${IMAGE_ROOTFS}/boot/initrd.img')"
> > -    fi
> > -    if [ -f "$initrd" ]; then
> > -        cp -f "$initrd" '${DEPLOYDIR}/${INITRD_IMAGE}'
> > +    # deploy default initrd if no custom one is build
> > +    if [ -e "${INITRD_IMAGE}" ]; then
> > +        initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> > +        if [ ! -f "$initrd" ]; then
> > +            initrd="$(realpath -q
> > '${IMAGE_ROOTFS}/boot/initrd.img')"
> > +        fi
> > +        if [ -f "$initrd" ]; then
> > +            cp -f "$initrd" '${DEPLOYDIR}/${INITRD_DEPLOY_FILE}'
> > +        fi
> >      fi
> >  
> >      for file in ${DTB_FILES}; do
> > diff --git a/scripts/start_vm b/scripts/start_vm
> > index 17091d72..8c696a4a 100755
> > --- a/scripts/start_vm
> > +++ b/scripts/start_vm
> > @@ -125,10 +125,10 @@ case "$IMAGE_FSTYPES" in
> >      readonly ROOTFS_IMAGE=$IMAGE_FULLNAME.ext4
> >  
> >      eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> > "^KERNEL_IMAGE=")
> > -    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> > "^INITRD_IMAGE=")
> > +    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> > "^INITRD_DEPLOY_FILE=")
> >      QKERNEL=$IMAGE_DIR/${KERNEL_IMAGE}
> >      QINITRD=/dev/null
> > -    [ -n "$INITRD_IMAGE" ] && QINITRD=$IMAGE_DIR/${INITRD_IMAGE}
> > +    [ -n "$INITRD_DEPLOY_FILE" ] &&
> > QINITRD=$IMAGE_DIR/${INITRD_DEPLOY_FILE}
> >      if [ "$ARCH" = "riscv64" ]; then
> >          EXTRA_ARGS="$EXTRA_ARGS -device
> > loader,file=$QKERNEL,addr=0x80200000"
> >          QKERNEL="/usr/lib/riscv64-linux-
> > gnu/opensbi/qemu/virt/fw_jump.elf"
> > diff --git a/testsuite/start_vm.py b/testsuite/start_vm.py
> > index 82ecc17d..ba1ba127 100755
> > --- a/testsuite/start_vm.py
> > +++ b/testsuite/start_vm.py
> > @@ -35,7 +35,7 @@ def format_qemu_cmdline(arch, build, distro, out,
> > pid, enforce_pcbios=False):
> >      if image_type == 'ext4':
> >          rootfs_image = 'isar-image-base-' + base + '-' + distro +
> > '-qemu' + arch + '.ext4'
> >          kernel_image = deploy_dir_image + '/' +
> > get_bitbake_var(bb_output, 'KERNEL_IMAGE')
> > -        initrd_image = get_bitbake_var(bb_output, 'INITRD_IMAGE')
> > +        initrd_image = get_bitbake_var(bb_output,
> > 'INITRD_DEPLOY_FILE')
> >  
> >          if not initrd_image:
> >              initrd_image = '/dev/null'
>