[PATCH 1/1] sandbox: don't set SA_NODEFER in signal handler

[PATCH 1/1] sandbox: don't set SA_NODEFER in signal handler

[Heinrich Schuchardt]

The sandbox can handle signals. Due to a damaged global data pointer
additional exceptions in the signal handler may occur leading to an endless
loop. In this case leave the handling of the secondary exception to the
operating system.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 arch/sandbox/cpu/os.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 0d21827e1b..9a73ae3b18 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -226,7 +226,7 @@ int os_setup_signal_handlers(void)

 	act.sa_sigaction = os_signal_handler;
 	sigemptyset(&act.sa_mask);
-	act.sa_flags = SA_SIGINFO | SA_NODEFER;
+	act.sa_flags = SA_SIGINFO;
 	if (sigaction(SIGILL, &act, NULL) ||
 	    sigaction(SIGBUS, &act, NULL) ||
 	    sigaction(SIGSEGV, &act, NULL))
--
2.30.2

Re: [PATCH 1/1] sandbox: don't set SA_NODEFER in signal handler

[Simon Glass]

Hi Heinrich,

On Mon, 5 Jul 2021 at 11:43, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> The sandbox can handle signals. Due to a damaged global data pointer
> additional exceptions in the signal handler may occur leading to an endless
> loop. In this case leave the handling of the secondary exception to the
> operating system.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  arch/sandbox/cpu/os.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I'm OK with this since it doesn't actually matter, now that the
signal-catching behaviour is optional.

But as I understand it, it will mean that the second exception is
reported, but not the first? In other words, any core dump will be
'incorrect' in that it is not suitable for postmortem debugging.

>
> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
> index 0d21827e1b..9a73ae3b18 100644
> --- a/arch/sandbox/cpu/os.c
> +++ b/arch/sandbox/cpu/os.c
> @@ -226,7 +226,7 @@ int os_setup_signal_handlers(void)
>
>         act.sa_sigaction = os_signal_handler;
>         sigemptyset(&act.sa_mask);
> -       act.sa_flags = SA_SIGINFO | SA_NODEFER;
> +       act.sa_flags = SA_SIGINFO;
>         if (sigaction(SIGILL, &act, NULL) ||
>             sigaction(SIGBUS, &act, NULL) ||
>             sigaction(SIGSEGV, &act, NULL))
> --
> 2.30.2
>

Regards,
Simon

Re: [PATCH 1/1] sandbox: don't set SA_NODEFER in signal handler

[Simon Glass]

Hi Heinrich,

On Wed, 7 Jul 2021 at 11:37, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Heinrich,
>
> On Mon, 5 Jul 2021 at 11:43, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > The sandbox can handle signals. Due to a damaged global data pointer
> > additional exceptions in the signal handler may occur leading to an endless
> > loop. In this case leave the handling of the secondary exception to the
> > operating system.
> >
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >  arch/sandbox/cpu/os.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I'm OK with this since it doesn't actually matter, now that the
> signal-catching behaviour is optional.
>
> But as I understand it, it will mean that the second exception is
> reported, but not the first? In other words, any core dump will be
> 'incorrect' in that it is not suitable for postmortem debugging.

I tested this by:

- setting sandbox's 'state' pointer to NULL in the 'md' command
- dropping state = &main_state from state_uninit
- running it and typing 'md'

This causes a recursive crash without your patch. With your patch it
is not recursive.

So I think it makes the -S option work better.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

Re: [PATCH 1/1] sandbox: don't set SA_NODEFER in signal handler

[Simon Glass]

Hi Heinrich,

On Wed, 7 Jul 2021 at 11:37, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Heinrich,
>
> On Mon, 5 Jul 2021 at 11:43, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > The sandbox can handle signals. Due to a damaged global data pointer
> > additional exceptions in the signal handler may occur leading to an endless
> > loop. In this case leave the handling of the secondary exception to the
> > operating system.
> >
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >  arch/sandbox/cpu/os.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> I'm OK with this since it doesn't actually matter, now that the
> signal-catching behaviour is optional.
>
> But as I understand it, it will mean that the second exception is
> reported, but not the first? In other words, any core dump will be
> 'incorrect' in that it is not suitable for postmortem debugging.

I tested this by:

- setting sandbox's 'state' pointer to NULL in the 'md' command
- dropping state = &main_state from state_uninit
- running it and typing 'md'

This causes a recursive crash without your patch. With your patch it
is not recursive.

So I think it makes the -S option work better.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

Applied to u-boot-dm, thanks!