[PATCH] classes/image_types_wic.bbclass: fix racing risk on rootfs

[PATCH] classes/image_types_wic.bbclass: fix racing risk on rootfs

[hongxu]

Since wic image creation will temporarily update rootfs/etc/fstab
to add UUID (*temporarily* means rootfs/etc/fstab will be recovered
after wic image creation), there is a potential racing risk with other
image creation (such as tar, ext)

Such as UUID was unexpected in tar.bz2's fstab:
$ cat unpack_tar_bz2_dir/etc/fstab
...
UUID=219B-2933 /boot vfat defaults 0 0
...

Explicitly make do_image_wic depend on other do_image_XXX (listed in
IMAGE_FSTYPES except do_image_wicXXX) to avoid potential racing

Signed-off-by: Hongxu Jia <hongxu.jia@windriver.com>
---
 meta/classes/image_types_wic.bbclass | 1 +
 1 file changed, 1 insertion(+)

diff --git a/meta/classes/image_types_wic.bbclass b/meta/classes/image_types_wic.bbclass
index 96ed0473ee..79bafeb818 100644
--- a/meta/classes/image_types_wic.bbclass
+++ b/meta/classes/image_types_wic.bbclass
@@ -25,6 +25,7 @@ def wks_search(files, search_path):
 
 WIC_CREATE_EXTRA_ARGS ?= ""
 
+IMAGE_TYPEDEP_wic += "${@' '.join('%s' % r for r in d.getVar('IMAGE_FSTYPES').split() if not r.startswith('wic'))}"
 IMAGE_CMD_wic () {
 	out="${IMGDEPLOYDIR}/${IMAGE_NAME}"
 	build_wic="${WORKDIR}/build-wic"
-- 
2.18.2

Re: [OE-core] [PATCH] classes/image_types_wic.bbclass: fix racing risk on rootfs

[Richard Purdie]

On Tue, 2020-05-19 at 00:00 +0800, hongxu wrote:
> Since wic image creation will temporarily update rootfs/etc/fstab
> to add UUID (*temporarily* means rootfs/etc/fstab will be recovered
> after wic image creation), there is a potential racing risk with other
> image creation (such as tar, ext)
> 
> Such as UUID was unexpected in tar.bz2's fstab:
> $ cat unpack_tar_bz2_dir/etc/fstab
> ...
> UUID=219B-2933 /boot vfat defaults 0 0
> ...
> 
> Explicitly make do_image_wic depend on other do_image_XXX (listed in
> IMAGE_FSTYPES except do_image_wicXXX) to avoid potential racing
> 
> Signed-off-by: Hongxu Jia <hongxu.jia@windriver.com>
> ---
>  meta/classes/image_types_wic.bbclass | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/meta/classes/image_types_wic.bbclass b/meta/classes/image_types_wic.bbclass
> index 96ed0473ee..79bafeb818 100644
> --- a/meta/classes/image_types_wic.bbclass
> +++ b/meta/classes/image_types_wic.bbclass
> @@ -25,6 +25,7 @@ def wks_search(files, search_path):
>  
>  WIC_CREATE_EXTRA_ARGS ?= ""
>  
> +IMAGE_TYPEDEP_wic += "${@' '.join('%s' % r for r in d.getVar('IMAGE_FSTYPES').split() if not r.startswith('wic'))}"
>  IMAGE_CMD_wic () {
>  	out="${IMGDEPLOYDIR}/${IMAGE_NAME}"
>  	build_wic="${WORKDIR}/build-wic"

I thought wic took a copy of the rootfs which it could them modify for
this reason but I can't find the code which would do that.

I think we should teach wic to take a copy (using hardlinks) as editing
files in the rootfs in do_image_* is not allowed. By using hardlinks
and putting it on the same filesystem as the original (rootfsdir +
".wic" maybe?) then it should be fast.

Cheers,

Richard

Re: [OE-core] [PATCH] classes/image_types_wic.bbclass: fix racing risk on rootfs

[hongxu]

On 5/19/20 6:19 PM, Richard Purdie wrote:
> On Tue, 2020-05-19 at 00:00 +0800, hongxu wrote:
>> Since wic image creation will temporarily update rootfs/etc/fstab
>> to add UUID (*temporarily* means rootfs/etc/fstab will be recovered
>> after wic image creation), there is a potential racing risk with other
>> image creation (such as tar, ext)
>>
>> Such as UUID was unexpected in tar.bz2's fstab:
>> $ cat unpack_tar_bz2_dir/etc/fstab
>> ...
>> UUID=219B-2933 /boot vfat defaults 0 0
>> ...
>>
>> Explicitly make do_image_wic depend on other do_image_XXX (listed in
>> IMAGE_FSTYPES except do_image_wicXXX) to avoid potential racing
>>
>> Signed-off-by: Hongxu Jia <hongxu.jia@windriver.com>
>> ---
>>   meta/classes/image_types_wic.bbclass | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/meta/classes/image_types_wic.bbclass b/meta/classes/image_types_wic.bbclass
>> index 96ed0473ee..79bafeb818 100644
>> --- a/meta/classes/image_types_wic.bbclass
>> +++ b/meta/classes/image_types_wic.bbclass
>> @@ -25,6 +25,7 @@ def wks_search(files, search_path):
>>   
>>   WIC_CREATE_EXTRA_ARGS ?= ""
>>   
>> +IMAGE_TYPEDEP_wic += "${@' '.join('%s' % r for r in d.getVar('IMAGE_FSTYPES').split() if not r.startswith('wic'))}"
>>   IMAGE_CMD_wic () {
>>   	out="${IMGDEPLOYDIR}/${IMAGE_NAME}"
>>   	build_wic="${WORKDIR}/build-wic"
> I thought wic took a copy of the rootfs which it could them modify for
> this reason but I can't find the code which would do that.

The is my v1 fix for this issue long ago

--- a/meta/classes/image_types_wic.bbclass
+++ b/meta/classes/image_types_wic.bbclass
@@ -26,14 +26,21 @@ def wks_search(files, search_path):
  WIC_CREATE_EXTRA_ARGS ?= ""

  IMAGE_CMD_wic () {
+       [ -d "${IMAGE_ROOTFS}_wic" ] && rm -rf "${IMAGE_ROOTFS}_wic"
+       # Refer oe.path.copytree(src, dst): ${WORKDIR}/rootfs -> 
${WORKDIR}/rootfs_wic
+       mkdir -p "${IMAGE_ROOTFS}_wic"
+       tar --xattrs --xattrs-include="*" -cf - -S -C "${IMAGE_ROOTFS}" 
-p . | tar --xattrs --xattrs-include="*" -xf - -C "${IMAGE_ROOTFS}_wic"
+
         out="${IMGDEPLOYDIR}/${IMAGE_NAME}"
         build_wic="${WORKDIR}/build-wic"
         wks="${WKS_FULL_PATH}"
         if [ -z "$wks" ]; then
                 bbfatal "No kickstart files from WKS_FILES were found: 
${WKS_FILES}. Please set WKS_FILE or WKS_FILES appropriately."
         fi
-       BUILDDIR="${TOPDIR}" PSEUDO_UNLOAD=1 wic create "$wks" --vars 
"${STAGING_DIR}/${MACHINE}/imgdata/" -e "${IMAGE_BASENAME}" -o 
"$build_wic/" ${WIC_CREATE_EXTRA_ARGS}
+
+       BUILDDIR="${TOPDIR}" PSEUDO_UNLOAD=1 wic create "$wks" --vars 
"${STAGING_DIR}/${MACHINE}/imgdata/" -e "${IMAGE_BASENAME}" -r 
"${IMAGE_ROOTFS}_wic" -o "$build_wic/" ${WIC_CREATE_EXTRA_ARGS}
         mv "$build_wic/$(basename "${wks%.wks}")"*.direct 
"$out${IMAGE_NAME_SUFFIX}.wic"
+       rm -rf "${IMAGE_ROOTFS}_wic"
  }


> I think we should teach wic to take a copy (using hardlinks) as editing
> files in the rootfs in do_image_* is not allowed. By using hardlinks
> and putting it on the same filesystem as the original (rootfsdir +
> ".wic" maybe?) then it should be fast.

But it does not quick, just a copy, no hardlink

I guess we could split the copy operation into two parts,

for the editing file, we copy it, for the others, using hardlink

//Hong
> Cheers,
>
> Richard
>