[PATCH 0/2] linux-user/ppc: Fix sigmask endianness issue in sigreturn

[PATCH 0/2] linux-user/ppc: Fix sigmask endianness issue in sigreturn

[Ilya Leoshkevich]

Hi,

This series fixes an issue where an emulated ppc64le process running on
s390x attempting to wake up a sigwait()ing thread would instead
terminate itself. Patch 1 is the fix, patch 2 is a testcase extracted
from the real-world scenario.

Best regards,
Ilya

Ilya Leoshkevich (2):
  linux-user/ppc: Fix sigmask endianness issue in sigreturn
  tests/tcg: Test that sigreturn() does not corrupt the signal mask

 linux-user/ppc/signal.c                 |  2 +-
 tests/tcg/multiarch/sigreturn-sigmask.c | 51 +++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/multiarch/sigreturn-sigmask.c

-- 
2.47.0

[PATCH 1/2] linux-user/ppc: Fix sigmask endianness issue in sigreturn

[Ilya Leoshkevich]

do_setcontext() copies the target sigmask without endianness handling
and then uses target_to_host_sigset_internal(), which expects a
byte-swapped one. Use target_to_host_sigset() instead.

Fixes: bcd4933a23f1 ("linux-user: ppc signal handling")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/ppc/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
index a1d8c0bccc1..24e5a02a782 100644
--- a/linux-user/ppc/signal.c
+++ b/linux-user/ppc/signal.c
@@ -628,7 +628,7 @@ static int do_setcontext(struct target_ucontext *ucp, CPUPPCState *env, int sig)
     if (!lock_user_struct(VERIFY_READ, mcp, mcp_addr, 1))
         return 1;
 
-    target_to_host_sigset_internal(&blocked, &set);
+    target_to_host_sigset(&blocked, &set);
     set_sigmask(&blocked);
     restore_user_regs(env, mcp, sig);
 
-- 
2.47.0

Re: [PATCH 1/2] linux-user/ppc: Fix sigmask endianness issue in sigreturn

[Richard Henderson]

On 10/17/24 05:54, Ilya Leoshkevich wrote:
> do_setcontext() copies the target sigmask without endianness handling
> and then uses target_to_host_sigset_internal(), which expects a
> byte-swapped one. Use target_to_host_sigset() instead.
> 
> Fixes: bcd4933a23f1 ("linux-user: ppc signal handling")
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   linux-user/ppc/signal.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/2] linux-user/ppc: Fix sigmask endianness issue in sigreturn

[Philippe Mathieu-Daudé]

On 17/10/24 09:54, Ilya Leoshkevich wrote:
> do_setcontext() copies the target sigmask without endianness handling
> and then uses target_to_host_sigset_internal(), which expects a
> byte-swapped one. Use target_to_host_sigset() instead.

These function names are confusing.

> Fixes: bcd4933a23f1 ("linux-user: ppc signal handling")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   linux-user/ppc/signal.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
> index a1d8c0bccc1..24e5a02a782 100644
> --- a/linux-user/ppc/signal.c
> +++ b/linux-user/ppc/signal.c
> @@ -628,7 +628,7 @@ static int do_setcontext(struct target_ucontext *ucp, CPUPPCState *env, int sig)
>       if (!lock_user_struct(VERIFY_READ, mcp, mcp_addr, 1))
>           return 1;
>   
> -    target_to_host_sigset_internal(&blocked, &set);
> +    target_to_host_sigset(&blocked, &set);
>       set_sigmask(&blocked);
>       restore_user_regs(env, mcp, sig);
>   

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 1/2] linux-user/ppc: Fix sigmask endianness issue in sigreturn

[Michael Tokarev]

17.10.2024 15:54, Ilya Leoshkevich wrote:
> do_setcontext() copies the target sigmask without endianness handling
> and then uses target_to_host_sigset_internal(), which expects a
> byte-swapped one. Use target_to_host_sigset() instead.
> 
> Fixes: bcd4933a23f1 ("linux-user: ppc signal handling")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   linux-user/ppc/signal.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
> index a1d8c0bccc1..24e5a02a782 100644
> --- a/linux-user/ppc/signal.c
> +++ b/linux-user/ppc/signal.c
> @@ -628,7 +628,7 @@ static int do_setcontext(struct target_ucontext *ucp, CPUPPCState *env, int sig)
>       if (!lock_user_struct(VERIFY_READ, mcp, mcp_addr, 1))
>           return 1;
>   
> -    target_to_host_sigset_internal(&blocked, &set);
> +    target_to_host_sigset(&blocked, &set);
>       set_sigmask(&blocked);
>       restore_user_regs(env, mcp, sig);

Shouldn't this change be picked for all stable releases too?
Yes, the issue is very old so probably does not have big impact,
but it looks like it should be okay to fix it finally?

I'm picking it up, please let me know if I shouldn't.

Thanks,

/mjt

Re: [PATCH 1/2] linux-user/ppc: Fix sigmask endianness issue in sigreturn

[Ilya Leoshkevich]

On Fri, 2024-10-25 at 16:53 +0300, Michael Tokarev wrote:
> 17.10.2024 15:54, Ilya Leoshkevich wrote:
> > do_setcontext() copies the target sigmask without endianness
> > handling
> > and then uses target_to_host_sigset_internal(), which expects a
> > byte-swapped one. Use target_to_host_sigset() instead.
> > 
> > Fixes: bcd4933a23f1 ("linux-user: ppc signal handling")
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   linux-user/ppc/signal.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
> > index a1d8c0bccc1..24e5a02a782 100644
> > --- a/linux-user/ppc/signal.c
> > +++ b/linux-user/ppc/signal.c
> > @@ -628,7 +628,7 @@ static int do_setcontext(struct target_ucontext
> > *ucp, CPUPPCState *env, int sig)
> >       if (!lock_user_struct(VERIFY_READ, mcp, mcp_addr, 1))
> >           return 1;
> >   
> > -    target_to_host_sigset_internal(&blocked, &set);
> > +    target_to_host_sigset(&blocked, &set);
> >       set_sigmask(&blocked);
> >       restore_user_regs(env, mcp, sig);
> 
> Shouldn't this change be picked for all stable releases too?
> Yes, the issue is very old so probably does not have big impact,
> but it looks like it should be okay to fix it finally?
> 
> I'm picking it up, please let me know if I shouldn't.
> 
> Thanks,
> 
> /mjt

I think it's worth taking it, since it leads to weird qemu-user
crashes. I actually intended to Cc: stable here and forgot to do it,
so thanks for bringing this up.

[PATCH 2/2] tests/tcg: Test that sigreturn() does not corrupt the signal mask

[Ilya Leoshkevich]

Add a small test to prevent regressions.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/multiarch/sigreturn-sigmask.c | 51 +++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 tests/tcg/multiarch/sigreturn-sigmask.c

diff --git a/tests/tcg/multiarch/sigreturn-sigmask.c b/tests/tcg/multiarch/sigreturn-sigmask.c
new file mode 100644
index 00000000000..e6cc904898d
--- /dev/null
+++ b/tests/tcg/multiarch/sigreturn-sigmask.c
@@ -0,0 +1,51 @@
+/*
+ * Test that sigreturn() does not corrupt the signal mask.
+ * Block SIGUSR2 and handle SIGUSR1.
+ * Then sigwait() SIGUSR2, which relies on it remaining blocked.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <pthread.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+int seen_sig = -1;
+
+static void signal_func(int sig)
+{
+    seen_sig = sig;
+}
+
+static void *thread_func(void *arg)
+{
+    kill(getpid(), SIGUSR2);
+    return NULL;
+}
+
+int main(void)
+{
+    struct sigaction act = {
+        .sa_handler = signal_func,
+    };
+    pthread_t thread;
+    sigset_t set;
+    int sig;
+
+    assert(sigaction(SIGUSR1, &act, NULL) == 0);
+
+    assert(sigemptyset(&set) == 0);
+    assert(sigaddset(&set, SIGUSR2) == 0);
+    assert(sigprocmask(SIG_BLOCK, &set, NULL) == 0);
+
+    kill(getpid(), SIGUSR1);
+    assert(seen_sig == SIGUSR1);
+
+    assert(pthread_create(&thread, NULL, thread_func, NULL) == 0);
+    assert(sigwait(&set, &sig) == 0);
+    assert(sig == SIGUSR2);
+    assert(pthread_join(thread, NULL) == 0);
+
+    return EXIT_SUCCESS;
+}
-- 
2.47.0

Re: [PATCH 2/2] tests/tcg: Test that sigreturn() does not corrupt the signal mask

[Richard Henderson]

On 10/17/24 05:54, Ilya Leoshkevich wrote:
> Add a small test to prevent regressions.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   tests/tcg/multiarch/sigreturn-sigmask.c | 51 +++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
>   create mode 100644 tests/tcg/multiarch/sigreturn-sigmask.c


Acked-by: Richard Henderson <richard.henderson@linaro.org>

r~

> 
> diff --git a/tests/tcg/multiarch/sigreturn-sigmask.c b/tests/tcg/multiarch/sigreturn-sigmask.c
> new file mode 100644
> index 00000000000..e6cc904898d
> --- /dev/null
> +++ b/tests/tcg/multiarch/sigreturn-sigmask.c
> @@ -0,0 +1,51 @@
> +/*
> + * Test that sigreturn() does not corrupt the signal mask.
> + * Block SIGUSR2 and handle SIGUSR1.
> + * Then sigwait() SIGUSR2, which relies on it remaining blocked.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#include <assert.h>
> +#include <pthread.h>
> +#include <signal.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +int seen_sig = -1;
> +
> +static void signal_func(int sig)
> +{
> +    seen_sig = sig;
> +}
> +
> +static void *thread_func(void *arg)
> +{
> +    kill(getpid(), SIGUSR2);
> +    return NULL;
> +}
> +
> +int main(void)
> +{
> +    struct sigaction act = {
> +        .sa_handler = signal_func,
> +    };
> +    pthread_t thread;
> +    sigset_t set;
> +    int sig;
> +
> +    assert(sigaction(SIGUSR1, &act, NULL) == 0);
> +
> +    assert(sigemptyset(&set) == 0);
> +    assert(sigaddset(&set, SIGUSR2) == 0);
> +    assert(sigprocmask(SIG_BLOCK, &set, NULL) == 0);
> +
> +    kill(getpid(), SIGUSR1);
> +    assert(seen_sig == SIGUSR1);
> +
> +    assert(pthread_create(&thread, NULL, thread_func, NULL) == 0);
> +    assert(sigwait(&set, &sig) == 0);
> +    assert(sig == SIGUSR2);
> +    assert(pthread_join(thread, NULL) == 0);
> +
> +    return EXIT_SUCCESS;
> +}

Re: [PATCH 2/2] tests/tcg: Test that sigreturn() does not corrupt the signal mask

[Richard Henderson]

On 10/17/24 05:54, Ilya Leoshkevich wrote:
> Add a small test to prevent regressions.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   tests/tcg/multiarch/sigreturn-sigmask.c | 51 +++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
>   create mode 100644 tests/tcg/multiarch/sigreturn-sigmask.c

This needs -lpthread.  I'll fix while applying.


r~

Re: [PATCH 0/2] linux-user/ppc: Fix sigmask endianness issue in sigreturn

[Richard Henderson]

On 10/17/24 05:54, Ilya Leoshkevich wrote:
> Hi,
> 
> This series fixes an issue where an emulated ppc64le process running on
> s390x attempting to wake up a sigwait()ing thread would instead
> terminate itself. Patch 1 is the fix, patch 2 is a testcase extracted
> from the real-world scenario.
> 
> Best regards,
> Ilya
> 
> Ilya Leoshkevich (2):
>    linux-user/ppc: Fix sigmask endianness issue in sigreturn
>    tests/tcg: Test that sigreturn() does not corrupt the signal mask
> 
>   linux-user/ppc/signal.c                 |  2 +-
>   tests/tcg/multiarch/sigreturn-sigmask.c | 51 +++++++++++++++++++++++++
>   2 files changed, 52 insertions(+), 1 deletion(-)
>   create mode 100644 tests/tcg/multiarch/sigreturn-sigmask.c
> 

Queued, thanks.

r~