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

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

On Thu, 2023-02-23 at 08:57 +0300, Uladzimir Bely wrote:
> In the email from Thursday, 23 February 2023 06:36:00 +03 user
> Moessbauer, Felix wrote:
> > On Wed, 2023-02-22 at 20:40 +0300, Uladzimir Bely wrote:
> > > In the email from Saturday, 18 February 2023 13:30:38 +03 user
> > > 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.
> > > > 
> > > > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> > > > ---
> > > >  RECIPE-API-CHANGELOG.md    |  8 ++++++++
> > > >  meta/classes/image.bbclass | 20 ++++++++++++--------
> > > >  scripts/start_vm           |  4 ++--
> > > >  testsuite/start_vm.py      |  2 +-
> > > >  4 files changed, 23 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> > > > index e48c98c7..1e8dbfc8 100644
> > > > --- a/RECIPE-API-CHANGELOG.md
> > > > +++ b/RECIPE-API-CHANGELOG.md
> > > > @@ -476,3 +476,11 @@ Bitbake 2.0 for better performance. It
> > > > also
> > > > requires isar-sstate script to be
> > > >  migrated to zstd.
> > > >  Mixing old Gzip-based and new ZStandatd-based sstate cache is
> > > > not
> > > > recommended
> > > >  and should be avoid for correct compatibility.
> > > > +
> > > > +### Working with a custom initramfs
> > > > +
> > > > +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. By that, the variable cannot be used
> > > > to
> > > > get the name of
> > > > +the images initramfs. Instead, the variable
> > > > `INITRD_DEPLOY_FILE`
> > > > is provided which
> > > > +always povides the name of the initrd file (also when the
> > > > default
> > > > one is used).
> > > > diff --git a/meta/classes/image.bbclass
> > > > b/meta/classes/image.bbclass
> > > > index 6277069f..7b3551b0 100644
> > > > --- a/meta/classes/image.bbclass
> > > > +++ b/meta/classes/image.bbclass
> > > > @@ -23,7 +23,8 @@ 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"
> > > > +INITRD_IMAGE ?= ""
> > > > +INITRD_DEPLOY_FILE = "${@ d.getVar('INITRD_IMAGE', True) 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}'
> > > > +    if [ -e "${INITRD_IMAGE}" ]; then
> > > > +        # deploy default initrd if no custom one is build
> > > > +        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')
> > > 
> > > This seems to be not enough to change the name without directory,
> > > since it's later concatenated with old one (deploy_dir_image)
> > 
> > Why would that make a difference? The new INITRD_DEPLOY_FILE
> > variable
> > should behave exactly the same as the previous INITRD_IMAGE, when
> > set.
> > If not, then this is a bug in my patch.
> > 
> > > 
> > > >  
> > > >          if not initrd_image:
> > > >              initrd_image = '/dev/null'
> > > > 
> > >         else:
> > >             initrd_image = deploy_dir_image + '/' + initrd_image`
> > > 
> > > Moreover, I would say, it's not good that we should now look for
> > > initrd file somewhere in DEPLOYDIR="${WORKDIR}/deploy", while
> > > everyone would expect to find it
> > > DEPLOY_DIR_IMAGE="${DEPLOY_DIR}/images/${MACHINE}"...
> > 
> > This should also not be the case. The deploy location should be
> > exactly
> > the same as before. In my local tests this also always was the
> > case.
> > I'll have a second look.
> > 
> 
> I've rechecked with `bitbake -e mc:qemuarm-bullseye:isar-image-base`.
> 
> Before patchset, these lines in `do_copy_boot_files`
> ```
> if [ -f "$initrd" ]; then
>     cp -f "$initrd" '${DEPLOY_DIR_IMAGE}/${INITRD_IMAGE}'
> fi
> ```
> are converted to:
> ```
> if [ -f "$initrd" ]; then
>     cp -f "$initrd" '/build/tmp/deploy/images/qemuarm/isar-image-
> base-debian-bullseye-qemuarm-initrd.img'
> fi
> ```
> 
> while with the patchset we have a new transition from
> ```
> if [ -f "$initrd" ]; then
>     cp -f "$initrd" '${DEPLOYDIR}/${INITRD_DEPLOY_FILE}'
> fi
> ```
> to
> ```
> if [ -f "$initrd" ]; then
>     cp -f "$initrd" '/build/tmp/work/debian-bullseye-armhf/isar-
> image-base-qemuarm/1.0-r0/deploy/isar-image-base-debian-bullseye-
> qemuarm-initrd.img'
> fi
> ```
> 
> So, the issue caused not by ${INITRD_DEPLOY_FILE}, but by
> ${DEPLOYDIR}

While this might look like a change, we still deploy to
${DEPLOY_DIR_IMAGE} as previously. The only difference is that we now
deploy via the sstate-cache, that means we deploy to DEPLOYDIR and
sstate copies these then to ${DEPLOY_DIR_IMAGE}. From an external point
of view, there is no difference at all.

For details, see p2, mainly:
+do_copy_boot_files[cleandirs] += "${DEPLOYDIR}"
+do_copy_boot_files[sstate-inputdirs] = "${DEPLOYDIR}"
+do_copy_boot_files[sstate-outputdirs] = "${DEPLOY_DIR_IMAGE}"

Best regards,
Felix

> 
> > With all these changes to the testsuite I have to admit that I
> > simply
> > did not run it. I lost track on how to use it after the changes.
> > 
> 
> Yes, there have been many changes in testsuite recently (and more are
> upcoming), but related part in `start_vm.py / format_qemu_cmdline` is
> "stable" for years.
> 
> > I will have a second look at the patch series.
> > 
> > Felix
> > 
> > > 
> > > 
> > > 
> > > 
> > 
> > 
> 
> 
> 
>