Re: [PATCH] linux-user: Fixed cpu restore with pc 0 on SIGBUS

[Robbin Ehn]

Hi, Palmer,

On Fri, Jan 12, 2024 at 10:03 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Fri, 12 Jan 2024 12:57:22 PST (-0800), rehn@rivosinc.com wrote:
> > Commit f4e1168198 (linux-user: Split out host_sig{segv,bus}_handler)
> > introduced a bug, when returning from host_sigbus_handler the PC is
>
> So we should probably have a
>
> Fixes: f4e1168198 ("linux-user: Split out host_sig{segv,bus}_handler")

You are correct.

>
> > never set. Thus cpu_loop_exit_restore is called with a zero PC and
> > we immediate get a SIGSEGV.
> >
> > Signed-off-by: Robbin Ehn <rehn@rivosinc.com>
> > ---
> >  linux-user/signal.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/linux-user/signal.c b/linux-user/signal.c
> > index b35d1e512f..c9527adfa3 100644
> > --- a/linux-user/signal.c
> > +++ b/linux-user/signal.c
> > @@ -925,7 +925,7 @@ static void host_sigsegv_handler(CPUState *cpu, 
> > siginfo_t *info,
> >      cpu_loop_exit_sigsegv(cpu, guest_addr, access_type, maperr, pc);
> >  }
> >
> > -static void host_sigbus_handler(CPUState *cpu, siginfo_t *info,
> > +static uintptr_t host_sigbus_handler(CPUState *cpu, siginfo_t *info,
> >                                  host_sigcontext *uc)
> >  {
> >      uintptr_t pc = host_signal_pc(uc);
> > @@ -947,6 +947,7 @@ static void host_sigbus_handler(CPUState *cpu, 
> > siginfo_t *info,
> >          sigprocmask(SIG_SETMASK, host_signal_mask(uc), NULL);
> >          cpu_loop_exit_sigbus(cpu, guest_addr, access_type, pc);
> >      }
> > +    return pc;
> >  }
> >
> >  static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
> > @@ -974,7 +975,7 @@ static void host_signal_handler(int host_sig, siginfo_t 
> > *info, void *puc)
> >              host_sigsegv_handler(cpu, info, uc);
>
> Do we have the same problem for SEGV?  They both used to set

Yea, it's not easy to follow the different paths... this code needs
another refactor, I was tempted but refrained myself.
So in the switch state if we have SEGV (and si_code>0) we always long
jump or return.
Only SIGBUS sets sync_sig to true, and thus calls
cpu_loop_exit_restore, hence needs a PC.
But the comment makes you think it's for multiple signals.

>
>     pc = host_signal_pc(uc);
>
> but with this it's only SIGBUS.  Maybe the same for the others, so just
> something like
>
>     diff --git a/linux-user/signal.c b/linux-user/signal.c
>     index b35d1e512f..55840bdf31 100644
>     --- a/linux-user/signal.c
>     +++ b/linux-user/signal.c
>     @@ -968,6 +968,8 @@ static void host_signal_handler(int host_sig, 
> siginfo_t *info, void *puc)
>           * SIGFPE, SIGTRAP are always host bugs.
>           */
>          if (info->si_code > 0) {
>     +        pc = host_signal_pc(uc);
>     +
>              switch (host_sig) {
>              case SIGSEGV:
>                  /* Only returns on handle_sigsegv_accerr_write success. */
>

Only those (SIGBUS) setting sync_sig need a PC.

> as it just does the PC chasing for everyone?
>

The sneaky return below.
Let me know if you still think setting the PC before the switch
statement is better.

> >              return;
> >          case SIGBUS:
> > -            host_sigbus_handler(cpu, info, uc);
> > +            pc = host_sigbus_handler(cpu, info, uc);
> >              sync_sig = true;
> >              break;
> >          case SIGILL:
> > --
> > 2.40.1
>
> Either way,
>
> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>

Thanks!

>
> Thanks!