| Message ID | 20200204171053.1718013-5-laurent@vivier.eu |
|---|---|
| State | New |
| Headers | show |
| Series | linux-user: fix use of SIGRTMIN | expand |
Reviewed-by: Taylor Simson <tsimpson@quicinc.com> > -----Original Message----- > From: Laurent Vivier <laurent@vivier.eu> > Sent: Tuesday, February 4, 2020 11:11 AM > To: qemu-devel@nongnu.org > Cc: Peter Maydell <peter.maydell@linaro.org>; Josh Kunz > <jkz@google.com>; Laurent Vivier <laurent@vivier.eu>; Aleksandar > Markovic <aleksandar.markovic@rt-rk.com>; Matus Kysel > <mkysel@tachyum.com>; Riku Voipio <riku.voipio@iki.fi>; Taylor Simpson > <tsimpson@quicinc.com>; milos.stojanovic@rt-rk.com; Marlies Ruck > <marlies.ruck@gmail.com> > Subject: [PATCH v2 4/4] linux-user: fix use of SIGRTMIN > > ------------------------------------------------------------------------- > CAUTION: This email originated from outside of the organization. > ------------------------------------------------------------------------- > > Some RT signals can be in use by glibc, > it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32). > > So SIGRTMIN cannot be mapped to TARGET_SIGRTMIN. > > Instead of swapping only SIGRTMIN and SIGRTMAX, map all the > range [TARGET_SIGRTMIN ... TARGET_SIGRTMAX - X] to > [__SIGRTMIN + X ... SIGRTMAX ] > (SIGRTMIN is __SIGRTMIN + X). > > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > --- > > Notes: > v2: ignore error when target sig <= TARGET_NSIG but host sig > SIGRTMAX > replace i, j by target_sig, host_sig > update signal_table_init() trace message > > linux-user/signal.c | 37 ++++++++++++++++++++++++++++++------- > linux-user/trace-events | 3 +++ > 2 files changed, 33 insertions(+), 7 deletions(-) > > diff --git a/linux-user/signal.c b/linux-user/signal.c > index c1e664f97a7c..e7e5581a016f 100644 > --- a/linux-user/signal.c > +++ b/linux-user/signal.c > @@ -498,18 +498,23 @@ static int core_dump_signal(int sig) > > static void signal_table_init(void) > { > - int host_sig, target_sig; > + int host_sig, target_sig, count; > > /* > - * Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with > - * host libpthread signals. This assumes no one actually uses SIGRTMAX :-/ > - * To fix this properly we need to do manual signal delivery multiplexed > - * over a single host signal. > + * some RT signals can be in use by glibc, > + * it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32) > */ > - host_to_target_signal_table[__SIGRTMIN] = __SIGRTMAX; > - host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN; > + for (host_sig = SIGRTMIN; host_sig <= SIGRTMAX; host_sig++) { > + target_sig = host_sig - SIGRTMIN + TARGET_SIGRTMIN; > + if (target_sig <= TARGET_NSIG) { > + host_to_target_signal_table[host_sig] = target_sig; > + } > + } > > /* generate signal conversion tables */ > + for (target_sig = 1; target_sig <= TARGET_NSIG; target_sig++) { > + target_to_host_signal_table[target_sig] = _NSIG; /* poison */ > + } > for (host_sig = 1; host_sig < _NSIG; host_sig++) { > if (host_to_target_signal_table[host_sig] == 0) { > host_to_target_signal_table[host_sig] = host_sig; > @@ -519,6 +524,15 @@ static void signal_table_init(void) > target_to_host_signal_table[target_sig] = host_sig; > } > } > + > + if (TRACE_SIGNAL_TABLE_INIT_BACKEND_DSTATE()) { > + for (target_sig = 1, count = 0; target_sig <= TARGET_NSIG; target_sig++) > { > + if (target_to_host_signal_table[target_sig] == _NSIG) { > + count++; > + } > + } > + trace_signal_table_init(count); > + } > } > > void signal_init(void) > @@ -817,6 +831,8 @@ int do_sigaction(int sig, const struct target_sigaction > *act, > int host_sig; > int ret = 0; > > + trace_signal_do_sigaction_guest(sig, TARGET_NSIG); > + > if (sig < 1 || sig > TARGET_NSIG || sig == TARGET_SIGKILL || sig == > TARGET_SIGSTOP) { > return -TARGET_EINVAL; > } > @@ -847,6 +863,13 @@ int do_sigaction(int sig, const struct target_sigaction > *act, > > /* we update the host linux signal state */ > host_sig = target_to_host_signal(sig); > + trace_signal_do_sigaction_host(host_sig, TARGET_NSIG); > + if (host_sig > SIGRTMAX) { > + /* we don't have enough host signals to map all target signals */ > + qemu_log_mask(LOG_UNIMP, "Unsupported target signal #%d, > ignored\n", > + sig); > + return 0; > + } > if (host_sig != SIGSEGV && host_sig != SIGBUS) { > sigfillset(&act1.sa_mask); > act1.sa_flags = SA_SIGINFO; > diff --git a/linux-user/trace-events b/linux-user/trace-events > index f6de1b8befc0..0296133daeb6 100644 > --- a/linux-user/trace-events > +++ b/linux-user/trace-events > @@ -1,6 +1,9 @@ > # See docs/devel/tracing.txt for syntax documentation. > > # signal.c > +signal_table_init(int i) "number of unavailable signals: %d" > +signal_do_sigaction_guest(int sig, int max) "target signal %d (MAX %d)" > +signal_do_sigaction_host(int sig, int max) "host signal %d (MAX %d)" > # */signal.c > user_setup_frame(void *env, uint64_t frame_addr) "env=%p > frame_addr=0x%"PRIx64 > user_setup_rt_frame(void *env, uint64_t frame_addr) "env=%p > frame_addr=0x%"PRIx64 > -- > 2.24.1 >
On Tue, 4 Feb 2020 at 17:11, Laurent Vivier <laurent@vivier.eu> wrote: > > Some RT signals can be in use by glibc, > it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32). > > So SIGRTMIN cannot be mapped to TARGET_SIGRTMIN. > > Instead of swapping only SIGRTMIN and SIGRTMAX, map all the > range [TARGET_SIGRTMIN ... TARGET_SIGRTMAX - X] to > [__SIGRTMIN + X ... SIGRTMAX ] > (SIGRTMIN is __SIGRTMIN + X). > > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > --- In general I think this is a good approach to trying to deal with this long-standing issue in a pragmatic and not too complicated way, so thanks for writing this patchset. I have some fairly minor comments on the code below. > > Notes: > v2: ignore error when target sig <= TARGET_NSIG but host sig > SIGRTMAX > replace i, j by target_sig, host_sig > update signal_table_init() trace message > > linux-user/signal.c | 37 ++++++++++++++++++++++++++++++------- > linux-user/trace-events | 3 +++ > 2 files changed, 33 insertions(+), 7 deletions(-) > > diff --git a/linux-user/signal.c b/linux-user/signal.c > index c1e664f97a7c..e7e5581a016f 100644 > --- a/linux-user/signal.c > +++ b/linux-user/signal.c > @@ -498,18 +498,23 @@ static int core_dump_signal(int sig) > > static void signal_table_init(void) > { > - int host_sig, target_sig; > + int host_sig, target_sig, count; > > /* > - * Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with > - * host libpthread signals. This assumes no one actually uses SIGRTMAX :-/ > - * To fix this properly we need to do manual signal delivery multiplexed > - * over a single host signal. > + * some RT signals can be in use by glibc, > + * it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32) > */ > - host_to_target_signal_table[__SIGRTMIN] = __SIGRTMAX; > - host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN; > + for (host_sig = SIGRTMIN; host_sig <= SIGRTMAX; host_sig++) { > + target_sig = host_sig - SIGRTMIN + TARGET_SIGRTMIN; > + if (target_sig <= TARGET_NSIG) { > + host_to_target_signal_table[host_sig] = target_sig; > + } > + } So the effect of this is that we now support target signals starting from TARGET_SIGRTMIN and going up until we run out of host realtime signals that the host libc hasn't reserved ? That seems reasonable, since glibc at least uses only the lower 2 rt signals and probably nobody's using the upper ones. But this would be a good place to have a comment explaining the limitation (and that if it needed to be fixed we'd have to multiplex guest signals onto a single host signal). You could also mention that attempts to configure the "missing" signals via sigaction will be silently ignored. > /* generate signal conversion tables */ > + for (target_sig = 1; target_sig <= TARGET_NSIG; target_sig++) { > + target_to_host_signal_table[target_sig] = _NSIG; /* poison */ > + } > for (host_sig = 1; host_sig < _NSIG; host_sig++) { > if (host_to_target_signal_table[host_sig] == 0) { > host_to_target_signal_table[host_sig] = host_sig; > @@ -519,6 +524,15 @@ static void signal_table_init(void) > target_to_host_signal_table[target_sig] = host_sig; > } > } > + > + if (TRACE_SIGNAL_TABLE_INIT_BACKEND_DSTATE()) { This isn't the right way to conditionalize expensive stuff that's only used in trace events. You want to use trace_event_get_state_backends() (see docs/devel/tracing.txt for details). > + for (target_sig = 1, count = 0; target_sig <= TARGET_NSIG; target_sig++) { > + if (target_to_host_signal_table[target_sig] == _NSIG) { > + count++; > + } > + } > + trace_signal_table_init(count); > + } > } > > void signal_init(void) > @@ -817,6 +831,8 @@ int do_sigaction(int sig, const struct target_sigaction *act, > int host_sig; > int ret = 0; > > + trace_signal_do_sigaction_guest(sig, TARGET_NSIG); > + > if (sig < 1 || sig > TARGET_NSIG || sig == TARGET_SIGKILL || sig == TARGET_SIGSTOP) { > return -TARGET_EINVAL; > } > @@ -847,6 +863,13 @@ int do_sigaction(int sig, const struct target_sigaction *act, > > /* we update the host linux signal state */ > host_sig = target_to_host_signal(sig); > + trace_signal_do_sigaction_host(host_sig, TARGET_NSIG); > + if (host_sig > SIGRTMAX) { > + /* we don't have enough host signals to map all target signals */ > + qemu_log_mask(LOG_UNIMP, "Unsupported target signal #%d, ignored\n", > + sig); > + return 0; We should have a comment here mentioning why we don't return an error code here (and explicitly noting that the Go runtime is the major one which we don't want to upset). > + } > if (host_sig != SIGSEGV && host_sig != SIGBUS) { > sigfillset(&act1.sa_mask); > act1.sa_flags = SA_SIGINFO; > diff --git a/linux-user/trace-events b/linux-user/trace-events > index f6de1b8befc0..0296133daeb6 100644 > --- a/linux-user/trace-events > +++ b/linux-user/trace-events > @@ -1,6 +1,9 @@ > # See docs/devel/tracing.txt for syntax documentation. > > # signal.c > +signal_table_init(int i) "number of unavailable signals: %d" > +signal_do_sigaction_guest(int sig, int max) "target signal %d (MAX %d)" > +signal_do_sigaction_host(int sig, int max) "host signal %d (MAX %d)" > # */signal.c > user_setup_frame(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64 > user_setup_rt_frame(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64 thanks -- PMM
Thank you Peter, I will address you comments and send a new version of the series. Laurent Le 11/02/2020 à 18:05, Peter Maydell a écrit : > On Tue, 4 Feb 2020 at 17:11, Laurent Vivier <laurent@vivier.eu> wrote: >> >> Some RT signals can be in use by glibc, >> it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32). >> >> So SIGRTMIN cannot be mapped to TARGET_SIGRTMIN. >> >> Instead of swapping only SIGRTMIN and SIGRTMAX, map all the >> range [TARGET_SIGRTMIN ... TARGET_SIGRTMAX - X] to >> [__SIGRTMIN + X ... SIGRTMAX ] >> (SIGRTMIN is __SIGRTMIN + X). >> >> Signed-off-by: Laurent Vivier <laurent@vivier.eu> >> --- > > In general I think this is a good approach to trying to deal > with this long-standing issue in a pragmatic and not too > complicated way, so thanks for writing this patchset. I have > some fairly minor comments on the code below. > >> >> Notes: >> v2: ignore error when target sig <= TARGET_NSIG but host sig > SIGRTMAX >> replace i, j by target_sig, host_sig >> update signal_table_init() trace message >> >> linux-user/signal.c | 37 ++++++++++++++++++++++++++++++------- >> linux-user/trace-events | 3 +++ >> 2 files changed, 33 insertions(+), 7 deletions(-) >> >> diff --git a/linux-user/signal.c b/linux-user/signal.c >> index c1e664f97a7c..e7e5581a016f 100644 >> --- a/linux-user/signal.c >> +++ b/linux-user/signal.c >> @@ -498,18 +498,23 @@ static int core_dump_signal(int sig) >> >> static void signal_table_init(void) >> { >> - int host_sig, target_sig; >> + int host_sig, target_sig, count; >> >> /* >> - * Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with >> - * host libpthread signals. This assumes no one actually uses SIGRTMAX :-/ >> - * To fix this properly we need to do manual signal delivery multiplexed >> - * over a single host signal. >> + * some RT signals can be in use by glibc, >> + * it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32) >> */ >> - host_to_target_signal_table[__SIGRTMIN] = __SIGRTMAX; >> - host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN; >> + for (host_sig = SIGRTMIN; host_sig <= SIGRTMAX; host_sig++) { >> + target_sig = host_sig - SIGRTMIN + TARGET_SIGRTMIN; >> + if (target_sig <= TARGET_NSIG) { >> + host_to_target_signal_table[host_sig] = target_sig; >> + } >> + } > > So the effect of this is that we now support target signals > starting from TARGET_SIGRTMIN and going up until we run out > of host realtime signals that the host libc hasn't reserved ? > That seems reasonable, since glibc at least uses only the > lower 2 rt signals and probably nobody's using the upper ones. > But this would be a good place to have a comment explaining > the limitation (and that if it needed to be fixed we'd have > to multiplex guest signals onto a single host signal). You > could also mention that attempts to configure the "missing" > signals via sigaction will be silently ignored. > >> /* generate signal conversion tables */ >> + for (target_sig = 1; target_sig <= TARGET_NSIG; target_sig++) { >> + target_to_host_signal_table[target_sig] = _NSIG; /* poison */ >> + } >> for (host_sig = 1; host_sig < _NSIG; host_sig++) { >> if (host_to_target_signal_table[host_sig] == 0) { >> host_to_target_signal_table[host_sig] = host_sig; >> @@ -519,6 +524,15 @@ static void signal_table_init(void) >> target_to_host_signal_table[target_sig] = host_sig; >> } >> } >> + >> + if (TRACE_SIGNAL_TABLE_INIT_BACKEND_DSTATE()) { > > This isn't the right way to conditionalize expensive stuff > that's only used in trace events. You want to use > trace_event_get_state_backends() (see docs/devel/tracing.txt > for details). > >> + for (target_sig = 1, count = 0; target_sig <= TARGET_NSIG; target_sig++) { >> + if (target_to_host_signal_table[target_sig] == _NSIG) { >> + count++; >> + } >> + } >> + trace_signal_table_init(count); >> + } >> } >> >> void signal_init(void) >> @@ -817,6 +831,8 @@ int do_sigaction(int sig, const struct target_sigaction *act, >> int host_sig; >> int ret = 0; >> >> + trace_signal_do_sigaction_guest(sig, TARGET_NSIG); >> + >> if (sig < 1 || sig > TARGET_NSIG || sig == TARGET_SIGKILL || sig == TARGET_SIGSTOP) { >> return -TARGET_EINVAL; >> } >> @@ -847,6 +863,13 @@ int do_sigaction(int sig, const struct target_sigaction *act, >> >> /* we update the host linux signal state */ >> host_sig = target_to_host_signal(sig); >> + trace_signal_do_sigaction_host(host_sig, TARGET_NSIG); >> + if (host_sig > SIGRTMAX) { >> + /* we don't have enough host signals to map all target signals */ >> + qemu_log_mask(LOG_UNIMP, "Unsupported target signal #%d, ignored\n", >> + sig); >> + return 0; > > We should have a comment here mentioning why we don't return > an error code here (and explicitly noting that the Go runtime > is the major one which we don't want to upset). > >> + } >> if (host_sig != SIGSEGV && host_sig != SIGBUS) { >> sigfillset(&act1.sa_mask); >> act1.sa_flags = SA_SIGINFO; >> diff --git a/linux-user/trace-events b/linux-user/trace-events >> index f6de1b8befc0..0296133daeb6 100644 >> --- a/linux-user/trace-events >> +++ b/linux-user/trace-events >> @@ -1,6 +1,9 @@ >> # See docs/devel/tracing.txt for syntax documentation. >> >> # signal.c >> +signal_table_init(int i) "number of unavailable signals: %d" >> +signal_do_sigaction_guest(int sig, int max) "target signal %d (MAX %d)" >> +signal_do_sigaction_host(int sig, int max) "host signal %d (MAX %d)" >> # */signal.c >> user_setup_frame(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64 >> user_setup_rt_frame(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64 > > thanks > -- PMM >
diff --git a/linux-user/signal.c b/linux-user/signal.c index c1e664f97a7c..e7e5581a016f 100644 --- a/linux-user/signal.c +++ b/linux-user/signal.c @@ -498,18 +498,23 @@ static int core_dump_signal(int sig) static void signal_table_init(void) { - int host_sig, target_sig; + int host_sig, target_sig, count; /* - * Nasty hack: Reverse SIGRTMIN and SIGRTMAX to avoid overlap with - * host libpthread signals. This assumes no one actually uses SIGRTMAX :-/ - * To fix this properly we need to do manual signal delivery multiplexed - * over a single host signal. + * some RT signals can be in use by glibc, + * it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32) */ - host_to_target_signal_table[__SIGRTMIN] = __SIGRTMAX; - host_to_target_signal_table[__SIGRTMAX] = __SIGRTMIN; + for (host_sig = SIGRTMIN; host_sig <= SIGRTMAX; host_sig++) { + target_sig = host_sig - SIGRTMIN + TARGET_SIGRTMIN; + if (target_sig <= TARGET_NSIG) { + host_to_target_signal_table[host_sig] = target_sig; + } + } /* generate signal conversion tables */ + for (target_sig = 1; target_sig <= TARGET_NSIG; target_sig++) { + target_to_host_signal_table[target_sig] = _NSIG; /* poison */ + } for (host_sig = 1; host_sig < _NSIG; host_sig++) { if (host_to_target_signal_table[host_sig] == 0) { host_to_target_signal_table[host_sig] = host_sig; @@ -519,6 +524,15 @@ static void signal_table_init(void) target_to_host_signal_table[target_sig] = host_sig; } } + + if (TRACE_SIGNAL_TABLE_INIT_BACKEND_DSTATE()) { + for (target_sig = 1, count = 0; target_sig <= TARGET_NSIG; target_sig++) { + if (target_to_host_signal_table[target_sig] == _NSIG) { + count++; + } + } + trace_signal_table_init(count); + } } void signal_init(void) @@ -817,6 +831,8 @@ int do_sigaction(int sig, const struct target_sigaction *act, int host_sig; int ret = 0; + trace_signal_do_sigaction_guest(sig, TARGET_NSIG); + if (sig < 1 || sig > TARGET_NSIG || sig == TARGET_SIGKILL || sig == TARGET_SIGSTOP) { return -TARGET_EINVAL; } @@ -847,6 +863,13 @@ int do_sigaction(int sig, const struct target_sigaction *act, /* we update the host linux signal state */ host_sig = target_to_host_signal(sig); + trace_signal_do_sigaction_host(host_sig, TARGET_NSIG); + if (host_sig > SIGRTMAX) { + /* we don't have enough host signals to map all target signals */ + qemu_log_mask(LOG_UNIMP, "Unsupported target signal #%d, ignored\n", + sig); + return 0; + } if (host_sig != SIGSEGV && host_sig != SIGBUS) { sigfillset(&act1.sa_mask); act1.sa_flags = SA_SIGINFO; diff --git a/linux-user/trace-events b/linux-user/trace-events index f6de1b8befc0..0296133daeb6 100644 --- a/linux-user/trace-events +++ b/linux-user/trace-events @@ -1,6 +1,9 @@ # See docs/devel/tracing.txt for syntax documentation. # signal.c +signal_table_init(int i) "number of unavailable signals: %d" +signal_do_sigaction_guest(int sig, int max) "target signal %d (MAX %d)" +signal_do_sigaction_host(int sig, int max) "host signal %d (MAX %d)" # */signal.c user_setup_frame(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64 user_setup_rt_frame(void *env, uint64_t frame_addr) "env=%p frame_addr=0x%"PRIx64
Some RT signals can be in use by glibc, it's why SIGRTMIN (34) is generally greater than __SIGRTMIN (32). So SIGRTMIN cannot be mapped to TARGET_SIGRTMIN. Instead of swapping only SIGRTMIN and SIGRTMAX, map all the range [TARGET_SIGRTMIN ... TARGET_SIGRTMAX - X] to [__SIGRTMIN + X ... SIGRTMAX ] (SIGRTMIN is __SIGRTMIN + X). Signed-off-by: Laurent Vivier <laurent@vivier.eu> --- Notes: v2: ignore error when target sig <= TARGET_NSIG but host sig > SIGRTMAX replace i, j by target_sig, host_sig update signal_table_init() trace message linux-user/signal.c | 37 ++++++++++++++++++++++++++++++------- linux-user/trace-events | 3 +++ 2 files changed, 33 insertions(+), 7 deletions(-)