[PATCH] wic-img: Fix bind mounting

[PATCH] wic-img: Fix bind mounting

[Jan Kiszka]

This addresses two issues of the current code:

 - we must make the bind mount of STAGING_DIR private because it will
   otherwise pick up bind mounts done underneath it in the various
   rootfs used by parallel targets

 - locking was missing to make check and mount atomic, just like we do
   inside buildchroot_do_mounts

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

My theory on what went wrong /wrt rbind-mounted /sys and /dev: As the
bind-mounting of STAGING_DIR of a wic target (qemu-amd64) eventually
started to contain shared(!) bind-mounts of parallel targets (qemu-
armhf, qemu-arm64), and all those stick to the end, isar_handler started
to umount their now also shared /dev and /sys mounts. And that
destroyed the host mounts.

Lesson to be recalled: never paper over issues that were not yet fully
understood. They may point to sleeping problems that will only bite you
again later.

 meta/classes/wic-img.bbclass | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/meta/classes/wic-img.bbclass b/meta/classes/wic-img.bbclass
index 225463e..76602d8 100644
--- a/meta/classes/wic-img.bbclass
+++ b/meta/classes/wic-img.bbclass
@@ -87,11 +87,14 @@ do_build[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
 
 do_wic_image() {
     buildchroot_do_mounts
-    for dir in ${BBLAYERS} ${STAGING_DIR} ${ISARROOT}/scripts; do
-	sudo mkdir -p ${BUILDCHROOT_DIR}/$dir
-        mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1 \
-        || sudo mount --bind $dir ${BUILDCHROOT_DIR}/$dir
-    done
+    sudo flock ${MOUNT_LOCKFILE} -c ' \
+        for dir in ${BBLAYERS} ${STAGING_DIR} ${ISARROOT}/scripts; do
+            mkdir -p ${BUILDCHROOT_DIR}/$dir
+            if ! mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1; then
+                mount --bind --make-private $dir ${BUILDCHROOT_DIR}/$dir
+            fi
+        done
+        '
     export FAKEROOTCMD=${FAKEROOTCMD}
     export BUILDDIR=${BUILDDIR}
     export MTOOLS_SKIP_CHECK=1
-- 
2.16.4

Re: [PATCH] wic-img: Fix bind mounting

[Maxim Yu. Osipov]

Just to doublecheck: What is your patch queue for that?

On 12/6/18 11:13 AM, Jan Kiszka wrote:
> 
> This addresses two issues of the current code:
> 
>   - we must make the bind mount of STAGING_DIR private because it will
>     otherwise pick up bind mounts done underneath it in the various
>     rootfs used by parallel targets
> 
>   - locking was missing to make check and mount atomic, just like we do
>     inside buildchroot_do_mounts
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> My theory on what went wrong /wrt rbind-mounted /sys and /dev: As the
> bind-mounting of STAGING_DIR of a wic target (qemu-amd64) eventually
> started to contain shared(!) bind-mounts of parallel targets (qemu-
> armhf, qemu-arm64), and all those stick to the end, isar_handler started
> to umount their now also shared /dev and /sys mounts. And that
> destroyed the host mounts.
> 
> Lesson to be recalled: never paper over issues that were not yet fully
> understood. They may point to sleeping problems that will only bite you
> again later.
> 
>   meta/classes/wic-img.bbclass | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/meta/classes/wic-img.bbclass b/meta/classes/wic-img.bbclass
> index 225463e..76602d8 100644
> --- a/meta/classes/wic-img.bbclass
> +++ b/meta/classes/wic-img.bbclass
> @@ -87,11 +87,14 @@ do_build[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
>   
>   do_wic_image() {
>       buildchroot_do_mounts
> -    for dir in ${BBLAYERS} ${STAGING_DIR} ${ISARROOT}/scripts; do
> -	sudo mkdir -p ${BUILDCHROOT_DIR}/$dir
> -        mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1 \
> -        || sudo mount --bind $dir ${BUILDCHROOT_DIR}/$dir
> -    done
> +    sudo flock ${MOUNT_LOCKFILE} -c ' \
> +        for dir in ${BBLAYERS} ${STAGING_DIR} ${ISARROOT}/scripts; do
> +            mkdir -p ${BUILDCHROOT_DIR}/$dir
> +            if ! mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1; then
> +                mount --bind --make-private $dir ${BUILDCHROOT_DIR}/$dir
> +            fi
> +        done
> +        '
>       export FAKEROOTCMD=${FAKEROOTCMD}
>       export BUILDDIR=${BUILDDIR}
>       export MTOOLS_SKIP_CHECK=1
> 


-- 
Maxim Osipov
ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn
Germany
+49 (151) 6517 6917
mosipov@ilbers.de
http://ilbers.de/
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov

Re: [PATCH] wic-img: Fix bind mounting

[Jan Kiszka]

On 06.12.18 10:44, Maxim Yu. Osipov wrote:
> Just to doublecheck: What is your patch queue for that?
> 

Just updated jan/queue on github.com/siemens/isar. It's not the best ordering of 
patches, this fix should probably come earlier, but the top is what I tested 
successfully also in a stretch VM.

Jan

> On 12/6/18 11:13 AM, Jan Kiszka wrote:
>>
>> This addresses two issues of the current code:
>>
>>   - we must make the bind mount of STAGING_DIR private because it will
>>     otherwise pick up bind mounts done underneath it in the various
>>     rootfs used by parallel targets
>>
>>   - locking was missing to make check and mount atomic, just like we do
>>     inside buildchroot_do_mounts
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> My theory on what went wrong /wrt rbind-mounted /sys and /dev: As the
>> bind-mounting of STAGING_DIR of a wic target (qemu-amd64) eventually
>> started to contain shared(!) bind-mounts of parallel targets (qemu-
>> armhf, qemu-arm64), and all those stick to the end, isar_handler started
>> to umount their now also shared /dev and /sys mounts. And that
>> destroyed the host mounts.
>>
>> Lesson to be recalled: never paper over issues that were not yet fully
>> understood. They may point to sleeping problems that will only bite you
>> again later.
>>
>>   meta/classes/wic-img.bbclass | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/meta/classes/wic-img.bbclass b/meta/classes/wic-img.bbclass
>> index 225463e..76602d8 100644
>> --- a/meta/classes/wic-img.bbclass
>> +++ b/meta/classes/wic-img.bbclass
>> @@ -87,11 +87,14 @@ do_build[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
>>   do_wic_image() {
>>       buildchroot_do_mounts
>> -    for dir in ${BBLAYERS} ${STAGING_DIR} ${ISARROOT}/scripts; do
>> -    sudo mkdir -p ${BUILDCHROOT_DIR}/$dir
>> -        mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1 \
>> -        || sudo mount --bind $dir ${BUILDCHROOT_DIR}/$dir
>> -    done
>> +    sudo flock ${MOUNT_LOCKFILE} -c ' \
>> +        for dir in ${BBLAYERS} ${STAGING_DIR} ${ISARROOT}/scripts; do
>> +            mkdir -p ${BUILDCHROOT_DIR}/$dir
>> +            if ! mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1; then
>> +                mount --bind --make-private $dir ${BUILDCHROOT_DIR}/$dir
>> +            fi
>> +        done
>> +        '
>>       export FAKEROOTCMD=${FAKEROOTCMD}
>>       export BUILDDIR=${BUILDDIR}
>>       export MTOOLS_SKIP_CHECK=1
>>
> 
> 

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH] wic-img: Fix bind mounting

[Maxim Yu. Osipov]

Hi Jan,

On 12/6/18 2:17 PM, Jan Kiszka wrote:
> On 06.12.18 10:44, Maxim Yu. Osipov wrote:
>> Just to doublecheck: What is your patch queue for that?
>>
> 
> Just updated jan/queue on github.com/siemens/isar. It's not the best 
> ordering of patches, this fix should probably come earlier, but the top 
> is what I tested successfully also in a stretch VM.

I'm convinced that this patch fixes these two issues.
CI passes OK on my stretch laptop.
It fails on isar-build.org by evident reason (CI runs in stretch chroot 
on jessie host - the host's kernel 3.16 has troubles with 'mount 
--rbind'). We are now upgrading the host system.

When applying to the 'next' I may rearrange the ordering of patches in 
more logical path

git log --oneline:

c46ad1b isar-image: Make do_rootfs normal shell function
05d0fc4 isar-events: Improve umount handler
93c4795 ci: Make partial rebuild test non-destructive
4ca0bff ci: Wait for bitbake worker to finish before deleting artifacts
698fb56 Remove redundant recursive umounts
bbc61fb wic-img: Fix bind mounting
9cf29e6 isar-bootstrap: Fix and cleanup bind mounting
b354026 isar-image: umount base-apt when doing offline build
e965c0d gitlab-ci: Switch to ci_build.sh

is it OK?

Maxim.


> Jan
> 
>> On 12/6/18 11:13 AM, Jan Kiszka wrote:
>>>
>>> This addresses two issues of the current code:
>>>
>>>   - we must make the bind mount of STAGING_DIR private because it will
>>>     otherwise pick up bind mounts done underneath it in the various
>>>     rootfs used by parallel targets
>>>
>>>   - locking was missing to make check and mount atomic, just like we do
>>>     inside buildchroot_do_mounts
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>
>>> My theory on what went wrong /wrt rbind-mounted /sys and /dev: As the
>>> bind-mounting of STAGING_DIR of a wic target (qemu-amd64) eventually
>>> started to contain shared(!) bind-mounts of parallel targets (qemu-
>>> armhf, qemu-arm64), and all those stick to the end, isar_handler started
>>> to umount their now also shared /dev and /sys mounts. And that
>>> destroyed the host mounts.
>>>
>>> Lesson to be recalled: never paper over issues that were not yet fully
>>> understood. They may point to sleeping problems that will only bite you
>>> again later.
>>>
>>>   meta/classes/wic-img.bbclass | 13 ++++++++-----
>>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/meta/classes/wic-img.bbclass b/meta/classes/wic-img.bbclass
>>> index 225463e..76602d8 100644
>>> --- a/meta/classes/wic-img.bbclass
>>> +++ b/meta/classes/wic-img.bbclass
>>> @@ -87,11 +87,14 @@ do_build[stamp-extra-info] = 
>>> "${DISTRO}-${DISTRO_ARCH}"
>>>   do_wic_image() {
>>>       buildchroot_do_mounts
>>> -    for dir in ${BBLAYERS} ${STAGING_DIR} ${ISARROOT}/scripts; do
>>> -    sudo mkdir -p ${BUILDCHROOT_DIR}/$dir
>>> -        mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1 \
>>> -        || sudo mount --bind $dir ${BUILDCHROOT_DIR}/$dir
>>> -    done
>>> +    sudo flock ${MOUNT_LOCKFILE} -c ' \
>>> +        for dir in ${BBLAYERS} ${STAGING_DIR} ${ISARROOT}/scripts; do
>>> +            mkdir -p ${BUILDCHROOT_DIR}/$dir
>>> +            if ! mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1; 
>>> then
>>> +                mount --bind --make-private $dir 
>>> ${BUILDCHROOT_DIR}/$dir
>>> +            fi
>>> +        done
>>> +        '
>>>       export FAKEROOTCMD=${FAKEROOTCMD}
>>>       export BUILDDIR=${BUILDDIR}
>>>       export MTOOLS_SKIP_CHECK=1
>>>
>>
>>
> 


-- 
Maxim Osipov
ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn
Germany
+49 (151) 6517 6917
mosipov@ilbers.de
http://ilbers.de/
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov

Re: [PATCH] wic-img: Fix bind mounting

[Jan Kiszka]

Hi Maxim,

On 07.12.18 12:18, Maxim Yu. Osipov wrote:
> Hi Jan,
> 
> On 12/6/18 2:17 PM, Jan Kiszka wrote:
>> On 06.12.18 10:44, Maxim Yu. Osipov wrote:
>>> Just to doublecheck: What is your patch queue for that?
>>>
>>
>> Just updated jan/queue on github.com/siemens/isar. It's not the best ordering 
>> of patches, this fix should probably come earlier, but the top is what I 
>> tested successfully also in a stretch VM.
> 
> I'm convinced that this patch fixes these two issues.
> CI passes OK on my stretch laptop.
> It fails on isar-build.org by evident reason (CI runs in stretch chroot on 
> jessie host - the host's kernel 3.16 has troubles with 'mount --rbind'). We are 
> now upgrading the host system.

Should we document this dependency "host >= stretch" somewhere, or did we already?

> 
> When applying to the 'next' I may rearrange the ordering of patches in more 
> logical path
> 
> git log --oneline:
> 
> c46ad1b isar-image: Make do_rootfs normal shell function
> 05d0fc4 isar-events: Improve umount handler
> 93c4795 ci: Make partial rebuild test non-destructive
> 4ca0bff ci: Wait for bitbake worker to finish before deleting artifacts
> 698fb56 Remove redundant recursive umounts
> bbc61fb wic-img: Fix bind mounting
> 9cf29e6 isar-bootstrap: Fix and cleanup bind mounting
> b354026 isar-image: umount base-apt when doing offline build
> e965c0d gitlab-ci: Switch to ci_build.sh
> 
> is it OK?
> 

Looks good to me.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH] wic-img: Fix bind mounting

[Maxim Yu. Osipov]

On 12/6/18 11:13 AM, Jan Kiszka wrote:
> 
> This addresses two issues of the current code:
> 
>   - we must make the bind mount of STAGING_DIR private because it will
>     otherwise pick up bind mounts done underneath it in the various
>     rootfs used by parallel targets
> 
>   - locking was missing to make check and mount atomic, just like we do
>     inside buildchroot_do_mounts

Appled to the 'next',

Thanks,
Maxim.

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> My theory on what went wrong /wrt rbind-mounted /sys and /dev: As the
> bind-mounting of STAGING_DIR of a wic target (qemu-amd64) eventually
> started to contain shared(!) bind-mounts of parallel targets (qemu-
> armhf, qemu-arm64), and all those stick to the end, isar_handler started
> to umount their now also shared /dev and /sys mounts. And that
> destroyed the host mounts.
> 
> Lesson to be recalled: never paper over issues that were not yet fully
> understood. They may point to sleeping problems that will only bite you
> again later.
> 
>   meta/classes/wic-img.bbclass | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/meta/classes/wic-img.bbclass b/meta/classes/wic-img.bbclass
> index 225463e..76602d8 100644
> --- a/meta/classes/wic-img.bbclass
> +++ b/meta/classes/wic-img.bbclass
> @@ -87,11 +87,14 @@ do_build[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
>   
>   do_wic_image() {
>       buildchroot_do_mounts
> -    for dir in ${BBLAYERS} ${STAGING_DIR} ${ISARROOT}/scripts; do
> -	sudo mkdir -p ${BUILDCHROOT_DIR}/$dir
> -        mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1 \
> -        || sudo mount --bind $dir ${BUILDCHROOT_DIR}/$dir
> -    done
> +    sudo flock ${MOUNT_LOCKFILE} -c ' \
> +        for dir in ${BBLAYERS} ${STAGING_DIR} ${ISARROOT}/scripts; do
> +            mkdir -p ${BUILDCHROOT_DIR}/$dir
> +            if ! mountpoint ${BUILDCHROOT_DIR}/$dir >/dev/null 2>&1; then
> +                mount --bind --make-private $dir ${BUILDCHROOT_DIR}/$dir
> +            fi
> +        done
> +        '
>       export FAKEROOTCMD=${FAKEROOTCMD}
>       export BUILDDIR=${BUILDDIR}
>       export MTOOLS_SKIP_CHECK=1
> 


-- 
Maxim Osipov
ilbers GmbH
Maria-Merian-Str. 8
85521 Ottobrunn
Germany
+49 (151) 6517 6917
mosipov@ilbers.de
http://ilbers.de/
Commercial register Munich, HRB 214197
General Manager: Baurzhan Ismagulov