1. Patchew
  2. QEMU
  3. View series

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

Robbin Ehn posted 1 patch 8 months, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/33f27425878fb529b9e39ef22c303f6e0d90525f.camel@rivosinc.com
Maintainers: Laurent Vivier <laurent@vivier.eu>
linux-user/signal.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
[PATCH] linux-user: Fixed cpu restore with pc 0 on SIGBUS
Posted by Robbin Ehn 8 months, 1 week ago 
Commit f4e1168198 (linux-user: Split out host_sig{segv,bus}_handler)
introduced a bug, when returning from host_sigbus_handler the PC is
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);
             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
Re: [PATCH] linux-user: Fixed cpu restore with pc 0 on SIGBUS
Posted by Richard Henderson 8 months, 1 week ago 
On 1/13/24 07:57, Robbin Ehn wrote:
> Commit f4e1168198 (linux-user: Split out host_sig{segv,bus}_handler)
> introduced a bug, when returning from host_sigbus_handler the PC is
> 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(-)

Queued, thanks.


r~
Re: [PATCH] linux-user: Fixed cpu restore with pc 0 on SIGBUS
Posted by Palmer Dabbelt 8 months, 1 week ago 
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")

> 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

    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. */

as it just does the PC chasing for everyone?

>              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!
Re: [PATCH] linux-user: Fixed cpu restore with pc 0 on SIGBUS
Posted by Robbin Ehn 8 months, 1 week ago 
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!

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.