[BUG] sandbox error handling broken on origin/next

[Simon Glass]

Hi Heinrich,

On Tue, 23 Mar 2021 at 14:22, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Am 23. März 2021 01:57:00 MEZ schrieb Simon Glass <sjg at chromium.org>:
> >Hi Heinrich,
> >
> >On Tue, 23 Mar 2021 at 08:45, Heinrich Schuchardt <xypron.glpk at gmx.de>
> >wrote:
> >>
> >> On 3/22/21 7:16 PM, Simon Glass wrote:
> >> > Hi Heinrich,
> >> >
> >> > On Tue, 23 Mar 2021 at 07:12, Heinrich Schuchardt
> ><xypron.glpk at gmx.de> wrote:
> >> >>
> >> >> Hello Simon,
> >> >>
> >> >> using sandbox_defconfig on origin/master:
> >> >>
> >> >> Hit any key to stop autoboot:  0
> >> >> => exception sigsegv
> >> >>
> >> >> Segmentation violation
> >> >> pc = 0x55d3566d04f9, pc_reloc = 0x554f9
> >> >>
> >> >> $
> >> >>
> >> >> Here the SIGSEGV is correctly handled by the sandbox.
> >> >>
> >> >> On origin/next:
> >> >>
> >> >> => exception sigsegv
> >> >>
> >> >> Segmentation violation
> >> >> pc = 0x5567966da96b, pc_reloc = 0x5567866da96b
> >> >>
> >> >> Writing sandbox state
> >> >> Segmentation fault
> >> >> $
> >> >>
> >> >> The same problem is visible when executing the poweroff command.
> >> >>
> >> >> => poweroff
> >> >> poweroff ...
> >> >> Segmentation fault
> >> >> $
> >> >>
> >> >> Bisecting points to your commit
> >> >>
> >> >> b308d9fd18fa
> >> >> sandbox: Avoid using malloc() for system state
> >> >>
> >> >> The segmentation fault occurs when os_exit() calls dm_uninit().
> >> >> The value of gd is invalid at this point.
> >> >
> >> > Can you please check this patch?
> >> >
> >> >
> >http://patchwork.ozlabs.org/project/uboot/patch/20210315051124.1940496-10-sjg@chromium.org/
> >> >
> >> > Also, is there no test covering the above?
> >> >
> >> > Regards,
> >> > Simon
> >> >
> >>
> >> Hello Simon,
> >>
> >> We have a poweroff test but there is no detection for the
> >'Segmentation
> >> fault' string.
> >>
> >> For CONFIG_SANDBOX_CRASH_RESET=n the patch helps.
> >>
> >> For CONFIG_SANDBOX_CRASH_RESET=y you still get a segmentation fault
> >when
> >> executing 'exception sigsegv'.
> >
> >OK, is that caused by the same commit?
>
> Yes
>
> > The problem is that the commit
> >is actually fixing a bug. I'll need to think about how to fix the fix.
> >
> >>
> >> Unfortunately you decided to disable CONFIG_SANDBOX_CRASH_RESET in
> >> sandbox_defconfig. Otherwise you would have detected the problem as
> >> "FAILED test/py/tests/test_sandbox_exit.py::test_exception_reset".
> >
> >Well we don't generally want to reset when we get a crash, right?
> >
>
> Resetting after a crash is what all other boards do.
>
> I personally don't have a need for the sandbox to behave differently.

I see sandbox as a normal program. If you run grimp and it crashes, it
doesn't reset. The only reason we have a reset feature in sandbox is
for testing purposes.

>
> >>
> >> Please, adjust sandbox_reset().
> >
> >What would you like it to do?
>
> Executing the 'reset' command fails with a crash.
>
> The crash occurs when dm_uninit() is invoked.

This is definitely a tricky area. Since devices are allocated in the
'emulated' RAM we really can't close down sandbox before uniniting
devices, and vice versa. Some separation is needed.

I will take a look at this at some point, but it needs a fair bit of thought.

Regards,
Simon