| Message ID | 20230217100118.2912985-4-felix.moessbauer@siemens.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Fix data-race in deployment of initrd | expand |
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. 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. 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'
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' >
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'
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> --- meta/classes/image.bbclass | 24 ++++++++++++++---------- scripts/start_vm | 4 ++-- testsuite/start_vm.py | 2 +- 3 files changed, 17 insertions(+), 13 deletions(-)