Hi Heinrich,

On Tue, 23 Mar 2021 at 08:45, Heinrich Schuchardt xypron.glpk@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@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-sj...

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? 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?

Please, adjust sandbox_reset().

What would you like it to do?

Regards, Simon

Hi Heinrich,

On Tue, 23 Mar 2021 at 14:22, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

Am 23. März 2021 01:57:00 MEZ schrieb Simon Glass sjg@chromium.org:

...

Hi Heinrich,

On Tue, 23 Mar 2021 at 08:45, Heinrich Schuchardt xypron.glpk@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@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-sj...

...

...

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