[PATCH] image: limit search for *.core to regular files

[PATCH] image: limit search for *.core to regular files

[Cedric Hombourger]

Code to search and delete core dumps in the build tree assumes that
the build host has a kernel.core_pattern setting which would result
in core dumps having a .core file suffix: this is not guaranteed. One
may also argue that the build should have failed if a process executed
under qemu-user got to crash (and we should check why qemu has crashed
and fix it). My vote would be to kill that code but for now, make it
less wrong by restricting the search to regular files suffixed with
.core (this would at least stop isar from moving directories such as
"org.eclipse.equinox.p2.core" out of the image).

Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com>
---
 meta/classes/image.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index 73f1d52c..793c21a2 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -457,7 +457,7 @@ EOSUDO
 
     # Sometimes qemu-user-static generates coredumps in chroot, move them
     # to work temporary directory and inform user about it.
-    for f in $(sudo find ${ROOTFSDIR} -name *.core); do
+    for f in $(sudo find ${ROOTFSDIR} -type f -name *.core); do
         sudo mv "${f}" "${WORKDIR}/temp/"
         bbwarn "found core dump in rootfs, check it in ${WORKDIR}/temp/${f##*/}"
     done
-- 
2.39.2

Re: [PATCH] image: limit search for *.core to regular files

[Jan Kiszka]

On 12.03.24 08:39, 'Cedric Hombourger' via isar-users wrote:
> Code to search and delete core dumps in the build tree assumes that
> the build host has a kernel.core_pattern setting which would result
> in core dumps having a .core file suffix: this is not guaranteed. One
> may also argue that the build should have failed if a process executed
> under qemu-user got to crash (and we should check why qemu has crashed
> and fix it). My vote would be to kill that code but for now, make it
> less wrong by restricting the search to regular files suffixed with
> .core (this would at least stop isar from moving directories such as
> "org.eclipse.equinox.p2.core" out of the image).
> 
> Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com>
> ---
>  meta/classes/image.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index 73f1d52c..793c21a2 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -457,7 +457,7 @@ EOSUDO
>  
>      # Sometimes qemu-user-static generates coredumps in chroot, move them
>      # to work temporary directory and inform user about it.
> -    for f in $(sudo find ${ROOTFSDIR} -name *.core); do
> +    for f in $(sudo find ${ROOTFSDIR} -type f -name *.core); do
>          sudo mv "${f}" "${WORKDIR}/temp/"
>          bbwarn "found core dump in rootfs, check it in ${WORKDIR}/temp/${f##*/}"
>      done

Yeah, too much heuristics in play now. We could add a list of valid
"core" files on top, but maybe we should rather demand core file
generation being disabled during the build and enforcing that.

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH] image: limit search for *.core to regular files

[MOESSBAUER, Felix]

On Tue, 2024-03-12 at 09:11 +0100, 'Jan Kiszka' via isar-users wrote:
> On 12.03.24 08:39, 'Cedric Hombourger' via isar-users wrote:
> > Code to search and delete core dumps in the build tree assumes that
> > the build host has a kernel.core_pattern setting which would result
> > in core dumps having a .core file suffix: this is not guaranteed.
> > One
> > may also argue that the build should have failed if a process
> > executed
> > under qemu-user got to crash (and we should check why qemu has 
> > crashed

Well... It's not that easy. This is the third time for me that this
coredump discussion pops up somewhere.

Many builders (like CMake) use feature probing (e.g. to check for AVX2)
which execute test examples that either succeed or crash with a
coredump [1]. While most of our builders run inside the schroot, there
might still be cases outside schroot where this is the expected
behavior.

> > and fix it). My vote would be to kill that code but for now, make
> > it
> > less wrong by restricting the search to regular files suffixed with
> > .core (this would at least stop isar from moving directories such
> > as
> > "org.eclipse.equinox.p2.core" out of the image).
> > 
> > Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com>
> > ---
> >  meta/classes/image.bbclass | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/meta/classes/image.bbclass
> > b/meta/classes/image.bbclass
> > index 73f1d52c..793c21a2 100644
> > --- a/meta/classes/image.bbclass
> > +++ b/meta/classes/image.bbclass
> > @@ -457,7 +457,7 @@ EOSUDO
> >  
> >      # Sometimes qemu-user-static generates coredumps in chroot,
> > move them
> >      # to work temporary directory and inform user about it.
> > -    for f in $(sudo find ${ROOTFSDIR} -name *.core); do
> > +    for f in $(sudo find ${ROOTFSDIR} -type f -name *.core); do
> >          sudo mv "${f}" "${WORKDIR}/temp/"
> >          bbwarn "found core dump in rootfs, check it in
> > ${WORKDIR}/temp/${f##*/}"
> >      done
> 
> Yeah, too much heuristics in play now. We could add a list of valid
> "core" files on top, but maybe we should rather demand core file
> generation being disabled during the build and enforcing that.

We had exactly that discussion on the KAS ML as well, were I tried to
introduce a warning on default coredump configurations (which are BTW
really tricky to debug on CI systems). However that was not accepted as
the builders itself shall be responsible for a suitable coredump
configuration [2]. The truths is probably somewhere in between.

Just disabling the coredump generation is not easily possible, as there
is no "coredump" namespace in the kernel. By that, you would fiddle
around with the global system config and potentially interfere with
systemd (systemd-coredump).

The probably best thing we could do is to look for common coredump
patterns and check with file if these are actual coredumps.

[1] https://github.com/DynamoRIO/dynamorio/issues/6126
[2] https://groups.google.com/g/kas-devel/c/-sEyujhICfw/m/1UtVfsDRAAAJ

Best regards,
Felix

> 
> Jan
> 
> -- 
> Siemens AG, Technology
> Linux Expert Center
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH] image: limit search for *.core to regular files

[Jan Kiszka]

On 12.03.24 09:31, Moessbauer, Felix (T CED OES-DE) wrote:
> On Tue, 2024-03-12 at 09:11 +0100, 'Jan Kiszka' via isar-users wrote:
>> On 12.03.24 08:39, 'Cedric Hombourger' via isar-users wrote:
>>> Code to search and delete core dumps in the build tree assumes that
>>> the build host has a kernel.core_pattern setting which would result
>>> in core dumps having a .core file suffix: this is not guaranteed.
>>> One
>>> may also argue that the build should have failed if a process
>>> executed
>>> under qemu-user got to crash (and we should check why qemu has 
>>> crashed
> 
> Well... It's not that easy. This is the third time for me that this
> coredump discussion pops up somewhere.
> 
> Many builders (like CMake) use feature probing (e.g. to check for AVX2)
> which execute test examples that either succeed or crash with a
> coredump [1]. While most of our builders run inside the schroot, there
> might still be cases outside schroot where this is the expected
> behavior.
> 
>>> and fix it). My vote would be to kill that code but for now, make
>>> it
>>> less wrong by restricting the search to regular files suffixed with
>>> .core (this would at least stop isar from moving directories such
>>> as
>>> "org.eclipse.equinox.p2.core" out of the image).
>>>
>>> Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com>
>>> ---
>>>  meta/classes/image.bbclass | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/meta/classes/image.bbclass
>>> b/meta/classes/image.bbclass
>>> index 73f1d52c..793c21a2 100644
>>> --- a/meta/classes/image.bbclass
>>> +++ b/meta/classes/image.bbclass
>>> @@ -457,7 +457,7 @@ EOSUDO
>>>  
>>>      # Sometimes qemu-user-static generates coredumps in chroot,
>>> move them
>>>      # to work temporary directory and inform user about it.
>>> -    for f in $(sudo find ${ROOTFSDIR} -name *.core); do
>>> +    for f in $(sudo find ${ROOTFSDIR} -type f -name *.core); do
>>>          sudo mv "${f}" "${WORKDIR}/temp/"
>>>          bbwarn "found core dump in rootfs, check it in
>>> ${WORKDIR}/temp/${f##*/}"
>>>      done
>>
>> Yeah, too much heuristics in play now. We could add a list of valid
>> "core" files on top, but maybe we should rather demand core file
>> generation being disabled during the build and enforcing that.
> 
> We had exactly that discussion on the KAS ML as well, were I tried to
> introduce a warning on default coredump configurations (which are BTW
> really tricky to debug on CI systems). However that was not accepted as
> the builders itself shall be responsible for a suitable coredump
> configuration [2]. The truths is probably somewhere in between.
> 
> Just disabling the coredump generation is not easily possible, as there
> is no "coredump" namespace in the kernel. By that, you would fiddle
> around with the global system config and potentially interfere with
> systemd (systemd-coredump).
> 
> The probably best thing we could do is to look for common coredump
> patterns and check with file if these are actual coredumps.
> 

Can't we read out in isar what the effective settings are and use them
at least? In addition to possibly adding some exceptions on a per-image
basis.

Jan

> [1] https://github.com/DynamoRIO/dynamorio/issues/6126
> [2] https://groups.google.com/g/kas-devel/c/-sEyujhICfw/m/1UtVfsDRAAAJ
> 
> Best regards,
> Felix
> 
>>
>> Jan
>>
>> -- 
>> Siemens AG, Technology
>> Linux Expert Center
>>
> 

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH] image: limit search for *.core to regular files

[cedric.hombourger]

On Tue, 2024-03-12 at 10:03 +0100, Jan Kiszka wrote:
> On 12.03.24 09:31, Moessbauer, Felix (T CED OES-DE) wrote:
> > On Tue, 2024-03-12 at 09:11 +0100, 'Jan Kiszka' via isar-users
> > wrote:
> > > On 12.03.24 08:39, 'Cedric Hombourger' via isar-users wrote:
> > > > Code to search and delete core dumps in the build tree assumes
> > > > that
> > > > the build host has a kernel.core_pattern setting which would
> > > > result
> > > > in core dumps having a .core file suffix: this is not
> > > > guaranteed.
> > > > One
> > > > may also argue that the build should have failed if a process
> > > > executed
> > > > under qemu-user got to crash (and we should check why qemu has
> > > > crashed
> >
> > Well... It's not that easy. This is the third time for me that this
> > coredump discussion pops up somewhere.
> >
> > Many builders (like CMake) use feature probing (e.g. to check for
> > AVX2)
> > which execute test examples that either succeed or crash with a
> > coredump [1]. While most of our builders run inside the schroot,
> > there
> > might still be cases outside schroot where this is the expected
> > behavior.
> >
> > > > and fix it). My vote would be to kill that code but for now,
> > > > make
> > > > it
> > > > less wrong by restricting the search to regular files suffixed
> > > > with
> > > > .core (this would at least stop isar from moving directories
> > > > such
> > > > as
> > > > "org.eclipse.equinox.p2.core" out of the image).
> > > >
> > > > Signed-off-by: Cedric Hombourger
> > > > <cedric.hombourger@siemens.com>
> > > > ---
> > > >  meta/classes/image.bbclass | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/meta/classes/image.bbclass
> > > > b/meta/classes/image.bbclass
> > > > index 73f1d52c..793c21a2 100644
> > > > --- a/meta/classes/image.bbclass
> > > > +++ b/meta/classes/image.bbclass
> > > > @@ -457,7 +457,7 @@ EOSUDO
> > > >
> > > >      # Sometimes qemu-user-static generates coredumps in
> > > > chroot,
> > > > move them
> > > >      # to work temporary directory and inform user about it.
> > > > -    for f in $(sudo find ${ROOTFSDIR} -name *.core); do
> > > > +    for f in $(sudo find ${ROOTFSDIR} -type f -name *.core);
> > > > do
> > > >          sudo mv "${f}" "${WORKDIR}/temp/"
> > > >          bbwarn "found core dump in rootfs, check it in
> > > > ${WORKDIR}/temp/${f##*/}"
> > > >      done
> > >
> > > Yeah, too much heuristics in play now. We could add a list of
> > > valid
> > > "core" files on top, but maybe we should rather demand core file
> > > generation being disabled during the build and enforcing that.
> >
> > We had exactly that discussion on the KAS ML as well, were I tried
> > to
> > introduce a warning on default coredump configurations (which are
> > BTW
> > really tricky to debug on CI systems). However that was not
> > accepted as
> > the builders itself shall be responsible for a suitable coredump
> > configuration [2]. The truths is probably somewhere in between.
> >
> > Just disabling the coredump generation is not easily possible, as
> > there
> > is no "coredump" namespace in the kernel. By that, you would fiddle
> > around with the global system config and potentially interfere with
> > systemd (systemd-coredump).
> >
> > The probably best thing we could do is to look for common coredump
> > patterns and check with file if these are actual coredumps.
> >
>
> Can't we read out in isar what the effective settings are and use
> them
> at least? In addition to possibly adding some exceptions on a per-
> image
> basis.

Isn't that error-prone when kernel.core_pattern is configured to use an
external program to generate the coredump?

IMO, Isar should not do anything with coredumps, if the build did crash
with a coredump, shouldn't we attempt to debug qemu and fix the issue
there instead of masking the issue for the sole purpose of having a
"reproducible" build? Or maybe people concerned with reproducible
builds should make sure they configure kernel.core_pattern correctly to
e.g. always place coredumps in /var/coredumps. To me that code is out-
of-scope for Isar

>
> Jan
>
> > [1]
> > https://github.com/DynamoRIO/dynamorio/issues/6126
> > [2]
> > https://groups.google.com/g/kas-devel/c/-sEyujhICfw/m/1UtVfsDRAAAJ
> >
> > Best regards,
> > Felix
> >
> > >
> > > Jan
> > >
> > > --
> > > Siemens AG, Technology
> > > Linux Expert Center
> > >
> >
>

--
Cedric Hombourger
Siemens AG
http://www.siemens.com/

Re: [PATCH] image: limit search for *.core to regular files

[Uladzimir Bely]

On Tue, 2024-03-12 at 08:39 +0100, 'Cedric Hombourger' via isar-users
wrote:
> Code to search and delete core dumps in the build tree assumes that
> the build host has a kernel.core_pattern setting which would result
> in core dumps having a .core file suffix: this is not guaranteed. One
> may also argue that the build should have failed if a process
> executed
> under qemu-user got to crash (and we should check why qemu has
> crashed
> and fix it). My vote would be to kill that code but for now, make it
> less wrong by restricting the search to regular files suffixed with
> .core (this would at least stop isar from moving directories such as
> "org.eclipse.equinox.p2.core" out of the image).
> 
> Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com>
> ---
>  meta/classes/image.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Hello everyone.

The patch itself passed CI, but the discussion looks unfinished. If no
one against, we are ready to merge it to next.

Best regards,
Uladzimir.

Re: [PATCH] image: limit search for *.core to regular files

[Uladzimir Bely]

On Tue, 2024-03-12 at 08:39 +0100, 'Cedric Hombourger' via isar-users
wrote:
> Code to search and delete core dumps in the build tree assumes that
> the build host has a kernel.core_pattern setting which would result
> in core dumps having a .core file suffix: this is not guaranteed. One
> may also argue that the build should have failed if a process
> executed
> under qemu-user got to crash (and we should check why qemu has
> crashed
> and fix it). My vote would be to kill that code but for now, make it
> less wrong by restricting the search to regular files suffixed with
> .core (this would at least stop isar from moving directories such as
> "org.eclipse.equinox.p2.core" out of the image).
> 
> Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com>
> ---
>  meta/classes/image.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index 73f1d52c..793c21a2 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -457,7 +457,7 @@ EOSUDO
>  
>      # Sometimes qemu-user-static generates coredumps in chroot, move
> them
>      # to work temporary directory and inform user about it.
> -    for f in $(sudo find ${ROOTFSDIR} -name *.core); do
> +    for f in $(sudo find ${ROOTFSDIR} -type f -name *.core); do
>          sudo mv "${f}" "${WORKDIR}/temp/"
>          bbwarn "found core dump in rootfs, check it in
> ${WORKDIR}/temp/${f##*/}"
>      done
> -- 
> 2.39.2
> 
Applied to next, thanks.