Patchwork cpio rootfs build: Avoid modifying rootfs dir

login
register
mail settings
Submitter Jonas Eriksson
Date March 24, 2014, 3:56 p.m.
Message ID <1395676565-14427-2-git-send-email-jonas.eriksson@enea.com>
Download mbox | patch
Permalink /patch/69071/
State Accepted
Commit cdf0d9715eca62f9bd86e0a1ae6e435b43cf8d87
Headers show

Comments

Jonas Eriksson - March 24, 2014, 3:56 p.m.
The Linux kernel requires that initrd images contain a /init file for
the image to be used as an initrd, even if it is empty. Adding it into
the rootfs directory creates a race, that can upset tar when building
both a .tar and .cpio image file ("tar: .: file changed as we read it").
Additionally, whether or not the tar file will contain the /init file is
also up to the race condition.

To avoid this problem, move the /init addition out from the rootfs
directory, and thus only include it in the .cpio image.

Signed-off-by: Jonas Eriksson <jonas.eriksson@enea.com>
Signed-off-by: Josep Puigdemont <josep.puigdemont@enea.com>
Cc: Laurentiu Palcu <laurentiu.palcu@intel.com>
---
 meta/classes/image_types.bbclass | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Andrea Adami - March 25, 2014, 11:04 p.m.
On Mon, Mar 24, 2014 at 4:56 PM, Jonas Eriksson <jonas.eriksson@enea.com> wrote:
> The Linux kernel requires that initrd images contain a /init file for
> the image to be used as an initrd, even if it is empty. Adding it into
> the rootfs directory creates a race, that can upset tar when building
> both a .tar and .cpio image file ("tar: .: file changed as we read it").
> Additionally, whether or not the tar file will contain the /init file is
> also up to the race condition.
>
> To avoid this problem, move the /init addition out from the rootfs
> directory, and thus only include it in the .cpio image.
>
> Signed-off-by: Jonas Eriksson <jonas.eriksson@enea.com>
> Signed-off-by: Josep Puigdemont <josep.puigdemont@enea.com>
> Cc: Laurentiu Palcu <laurentiu.palcu@intel.com>
> ---
>  meta/classes/image_types.bbclass | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/meta/classes/image_types.bbclass b/meta/classes/image_types.bbclass
> index 602c1f0..8001025 100644
> --- a/meta/classes/image_types.bbclass
> +++ b/meta/classes/image_types.bbclass
> @@ -67,10 +67,12 @@ IMAGE_CMD_squashfs-xz = "mksquashfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_
>  IMAGE_CMD_tar = "tar -cvf ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.tar -C ${IMAGE_ROOTFS} ."
>
>  IMAGE_CMD_cpio () {
> -       if [ ! -L ${IMAGE_ROOTFS}/init ]; then
> -               touch ${IMAGE_ROOTFS}/init
> -       fi
>         (cd ${IMAGE_ROOTFS} && find . | cpio -o -H newc >${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cpio)
> +       if [ ! -e ${IMAGE_ROOTFS}/init ]; then
> +               mkdir -p ${WORKDIR}/cpio_append
> +               touch ${WORKDIR}/cpio_append/init
> +               (cd  ${WORKDIR}/cpio_append && echo ./init | cpio -oA -H newc -F ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cpio)
> +       fi
>  }
>
>  ELF_KERNEL ?= "${STAGING_DIR_HOST}/usr/src/kernel/${KERNEL_IMAGETYPE}"
> --
> 1.9.0
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

I admit I did not encounter this race, still I tested the patch
because it caught my attention.

There are some rare cases where init is a symlink pointing to a custom
executable so the previous test was specifically done in order to
avoid to touch a symlink.
I was initially afraid of the change but actually the test is done
after the creation of the cpio so this patch doesn't seem to break our
special case.

Acked-by: Andrea Adami <andrea.adami@gmail.com>
Jonas Eriksson - March 26, 2014, 7:29 a.m.
Hi Andrea,

On Wed, Mar 26, 2014 at 00:04:55 +0100 Andrea Adami wrote:
> I admit I did not encounter this race, still I tested the patch
> because it caught my attention.
> 
> There are some rare cases where init is a symlink pointing to a custom
> executable so the previous test was specifically done in order to
> avoid to touch a symlink.

Yes, that makes sense. It's however a bit problematic to alter
the rootfs directory after the do_rootfs has finished executing.
This is why the ./init append was moved to work with a separate
directory.

To clarify around the symlink situation: The patch moves from a
-L to a -e to make sure that we don't append the ./init to the
cpio if there is any kind of ./init in the rootfs directory, be
it a file or a symlink.

> I was initially afraid of the change but actually the test is done
> after the creation of the cpio so this patch doesn't seem to break our
> special case.
> 
> Acked-by: Andrea Adami <andrea.adami@gmail.com>

Thanks for the feedback and Ack!

/Jonas

Patch

diff --git a/meta/classes/image_types.bbclass b/meta/classes/image_types.bbclass
index 602c1f0..8001025 100644
--- a/meta/classes/image_types.bbclass
+++ b/meta/classes/image_types.bbclass
@@ -67,10 +67,12 @@  IMAGE_CMD_squashfs-xz = "mksquashfs ${IMAGE_ROOTFS} ${DEPLOY_DIR_IMAGE}/${IMAGE_
 IMAGE_CMD_tar = "tar -cvf ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.tar -C ${IMAGE_ROOTFS} ."
 
 IMAGE_CMD_cpio () {
-	if [ ! -L ${IMAGE_ROOTFS}/init ]; then
-		touch ${IMAGE_ROOTFS}/init
-	fi
 	(cd ${IMAGE_ROOTFS} && find . | cpio -o -H newc >${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cpio)
+	if [ ! -e ${IMAGE_ROOTFS}/init ]; then
+		mkdir -p ${WORKDIR}/cpio_append
+		touch ${WORKDIR}/cpio_append/init
+		(cd  ${WORKDIR}/cpio_append && echo ./init | cpio -oA -H newc -F ${DEPLOY_DIR_IMAGE}/${IMAGE_NAME}.rootfs.cpio)
+	fi
 }
 
 ELF_KERNEL ?= "${STAGING_DIR_HOST}/usr/src/kernel/${KERNEL_IMAGETYPE}"