[Qemu-devel] [PATCH 0/1] m68k linux user-space emulation fix (with sign-off this time)

[Qemu-devel] [PATCH 0/1] m68k linux user-space emulation fix (with sign-off this time)

[Michael Karcher]

This patch fixes cmake blocking on m68k when starting the second child
process. cmake relies on getting SIGCHLD to know when the child process finished,
and the uninitialised sigmask set on return of the first SIGCHLD oftentimes
blocked the second SIGCHLD.

The patch has been created against Laurent's qemu-m68k git tree, but according
to visual inspection, it should also apply to HEAD.

Michael Karcher (1):
  Fix do_rt_sigreturn on m68k linux userspace emulation

 linux-user/signal.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/1] Fix do_rt_sigreturn on m68k linux userspace emulation

[Michael Karcher]

do_rt_sigreturn forgets to initialize the signal mask variable before
trying to use it to restore the mask, so the signal mask is undefined
after do_rt_sigreturn. This bug has been in all the time since
7181155d when do_rt_sigreturn was implemented for m68k.

Signed-off-by: Michael Karcher <karcher@physik.fu-berlin.de>
---
 linux-user/signal.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index e03ed60..ae1014b 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5260,11 +5260,14 @@ long do_rt_sigreturn(CPUM68KState *env)
     abi_ulong frame_addr = env->aregs[7] - 4;
     target_sigset_t target_set;
     sigset_t set;
-    int d0;
+    int d0, i;
 
     if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
         goto badframe;
 
+    for(i = 0; i < TARGET_NSIG_WORDS; i++) {
+        target_set.sig[i] = frame->uc.tuc_sigmask.sig[i];
+    }
     target_to_host_sigset_internal(&set, &target_set);
     do_sigprocmask(SIG_SETMASK, &set, NULL);
 
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/1] Fix do_rt_sigreturn on m68k linux userspace emulation

[Laurent Vivier]

Le 09/12/2015 21:54, Michael Karcher a écrit :
> do_rt_sigreturn forgets to initialize the signal mask variable before
> trying to use it to restore the mask, so the signal mask is undefined
> after do_rt_sigreturn. This bug has been in all the time since
> 7181155d when do_rt_sigreturn was implemented for m68k.
> 
> Signed-off-by: Michael Karcher <karcher@physik.fu-berlin.de>
> ---
>  linux-user/signal.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index e03ed60..ae1014b 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -5260,11 +5260,14 @@ long do_rt_sigreturn(CPUM68KState *env)
>      abi_ulong frame_addr = env->aregs[7] - 4;
>      target_sigset_t target_set;
>      sigset_t set;
> -    int d0;
> +    int d0, i;
>  
>      if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
>          goto badframe;
>  
> +    for(i = 0; i < TARGET_NSIG_WORDS; i++) {
> +        target_set.sig[i] = frame->uc.tuc_sigmask.sig[i];
> +    }
>      target_to_host_sigset_internal(&set, &target_set);
>      do_sigprocmask(SIG_SETMASK, &set, NULL);
>  
> 

Nice catch.

I agree with you that the current code is completely  broken, but on the
other architectures, this operation seems to be done directly by

target_to_host_sigset(&set, &frame->uc.tuc_sigmask);

Could you have try with that ?

Thank you for your help,
Laurent

Re: [Qemu-devel] [PATCH 1/1] Fix do_rt_sigreturn on m68k linux userspace emulation

[Michael Karcher]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

On 09.12.2015 23:03, Laurent Vivier wrote:
>
> Le 09/12/2015 21:54, Michael Karcher a écrit :
>> do_rt_sigreturn forgets to initialize the signal mask variable before
>> trying to use it to restore the mask, so the signal mask is undefined
>> after do_rt_sigreturn. This bug has been in all the time since
>> 7181155d when do_rt_sigreturn was implemented for m68k.
>>
>> Signed-off-by: Michael Karcher <karcher@physik.fu-berlin.de>
>> ---
>>  linux-user/signal.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c
>> index e03ed60..ae1014b 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -5260,11 +5260,14 @@ long do_rt_sigreturn(CPUM68KState *env)
>>      abi_ulong frame_addr = env->aregs[7] - 4;
>>      target_sigset_t target_set;
>>      sigset_t set;
>> -    int d0;
>> +    int d0, i;
>>  
>>      if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
>>          goto badframe;
>>  
>> +    for(i = 0; i < TARGET_NSIG_WORDS; i++) {
>> +        target_set.sig[i] = frame->uc.tuc_sigmask.sig[i];
>> +    }
>>      target_to_host_sigset_internal(&set, &target_set);
>>      do_sigprocmask(SIG_SETMASK, &set, NULL);
>>  
>>
> Nice catch.
>
> I agree with you that the current code is completely  broken, but on the
> other architectures, this operation seems to be done directly by
>
> target_to_host_sigset(&set, &frame->uc.tuc_sigmask);
>
> Could you have try with that ?
target_to_host_sigset does endianness swapping, while my loop does not
do it. I made a test program (attached to this mail) that tests mask
behaviour with "classic" and "siginfo" handlers, and shows that this
patch still doesn't fix the issue completely (output should be four
times 1). Instead of re-masking the SIGCHLD (bit 16) in that example, it
tries to mask SIGKILL (bit 8) which is due to the endian mismatch. I
will send a fixed patch shortly.

Feel free to extend the program to other architectures to test it there,
too.

BTW: documentation of the stack frame / signature for non-SA_SIGINFO
signal handlers seems to be quite lacking. There is a remark in the
sigaction manpage, but that one obviously only applies to i386...

Thanks for your comments,
  Michael Karcher

[-- Attachment #2: sigtest.c --]
[-- Type: text/x-csrc, Size: 1661 bytes --]

//#include <asm/sigcontext.h>
#include <signal.h>
#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
//#include <ucontext.h>

volatile int child_blocked;
volatile sig_atomic_t got_it;

#if defined(__i386)
#define HAVE_LEGACY
void handle(int signal, struct sigcontext sc)
{
	child_blocked = sc.oldmask == (1 << (SIGCHLD-1));
	got_it = 1;
}
#elif defined(__amd64)
#define HAVE_LEGACY
void handle(int signal, struct ucontext uc)
{
	child_blocked = ((struct sigcontext*)&uc.uc_mcontext)->oldmask == (1 << (SIGCHLD-1));
	got_it = 1;
}
#elif defined(__mc68000)
#define HAVE_LEGACY
void handle(int signal, int code, struct sigcontext *sc)
{
	child_blocked = sc->sc_mask == (1 << (SIGCHLD-1));
	got_it = 1;
}
#endif


void handle_siginfo(int signal, siginfo_t * info, void *ctx)
{
	child_blocked = sigismember(&((ucontext_t*)ctx)->uc_sigmask, SIGCHLD);
	got_it = 1;
}

int main(void)
{
	struct sigaction sa;
	sigset_t set;

#ifdef HAVE_LEGACY
	sa.sa_flags = 0;
	sa.sa_handler = handle;
	sigemptyset(&sa.sa_mask);
	sigaction(SIGUSR1, &sa, NULL);
	sigemptyset(&set);
	sigaddset(&set, SIGCHLD);
	sigprocmask(SIG_SETMASK, &set, NULL);
	got_it = 0;
	kill(getpid(), SIGUSR1);
	while(got_it == 0);
	sigprocmask(SIG_SETMASK, NULL, &set);
	printf("%d %d\n", child_blocked, sigismember(&set, SIGCHLD));
#endif

	got_it = 0;
	sa.sa_flags = SA_SIGINFO;
	sa.sa_sigaction = handle_siginfo;
	sigaction(SIGUSR1, &sa, NULL);
	sigemptyset(&set);
	sigaddset(&set, SIGCHLD);
	sigprocmask(SIG_SETMASK, &set, NULL);
	got_it = 0;
	kill(getpid(), SIGUSR1);
	while(got_it == 0);
	sigprocmask(SIG_SETMASK, NULL, &set);
	printf("%d %d\n", child_blocked, sigismember(&set, SIGCHLD));
}

Re: [Qemu-devel] [PATCH 1/1] Fix do_rt_sigreturn on m68k linux userspace emulation

[Laurent Vivier]

Le 12/12/2015 10:55, Michael Karcher a écrit :
> On 09.12.2015 23:03, Laurent Vivier wrote:
>>
>> Le 09/12/2015 21:54, Michael Karcher a écrit :
>>> do_rt_sigreturn forgets to initialize the signal mask variable before
>>> trying to use it to restore the mask, so the signal mask is undefined
>>> after do_rt_sigreturn. This bug has been in all the time since
>>> 7181155d when do_rt_sigreturn was implemented for m68k.
>>>
[...]
> BTW: documentation of the stack frame / signature for non-SA_SIGINFO
> signal handlers seems to be quite lacking. There is a remark in the
> sigaction manpage, but that one obviously only applies to i386...

The best documentation is the kernel source: if you have a look at it
you will see that these functions (setup_frame(), do_sigreturn(), ...)
are just QEMU "translated" copy&paste.

Laurent

Re: [Qemu-devel] [PATCH 0/1] m68k linux user-space emulation fix (with sign-off this time)

[John Paul Adrian Glaubitz]

Hi Laurent!

On 12/09/2015 09:54 PM, Michael Karcher wrote:
> This patch fixes cmake blocking on m68k when starting the second child
> process. cmake relies on getting SIGCHLD to know when the child process finished,
> and the uninitialised sigmask set on return of the first SIGCHLD oftentimes
> blocked the second SIGCHLD.

I have tested Michael's patch and after applying it to my current
qemu-m68k tree, cmake works normally and no longer hangs when executed
on qemu-m68k as it did previously.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

[Qemu-devel] [PATCH 1/1] Fix do_rt_sigreturn on m68k linux userspace emulation

[Michael Karcher]

do_rt_sigreturn forgets to initialize the signal mask variable before
trying to use it to restore the mask, so the signal mask is undefined
after do_rt_sigreturn. This bug has been in all the time since
7181155d when do_rt_sigreturn was implemented for m68k.
---
 linux-user/signal.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index e03ed60..ae1014b 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5260,11 +5260,14 @@ long do_rt_sigreturn(CPUM68KState *env)
     abi_ulong frame_addr = env->aregs[7] - 4;
     target_sigset_t target_set;
     sigset_t set;
-    int d0;
+    int d0, i;
 
     if (!lock_user_struct(VERIFY_READ, frame, frame_addr, 1))
         goto badframe;
 
+    for(i = 0; i < TARGET_NSIG_WORDS; i++) {
+        target_set.sig[i] = frame->uc.tuc_sigmask.sig[i];
+    }
     target_to_host_sigset_internal(&set, &target_set);
     do_sigprocmask(SIG_SETMASK, &set, NULL);
 
-- 
2.1.4