| Message ID | 5824aebefdadb9beb24cda3fab0398931bedbfb1.1378383549.git.minovotn@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 05/09/2013 14:19, Michal Novotny ha scritto: > This is the patch to introduce SIGILL handler to be able to trigger > SIGSEGV signal in qemu. This has been written to help debugging > state when qemu crashes by SIGSEGV as a simple reproducer to > emulate such situation in case of need. What's wrong with "kill -11" or, within gdb, "j *0x1234"? Why do you need a SIGILL handler for this? In fact, SIGILL is a pretty bad choice: QEMU includes a JIT compiler, so a SIGILL is a relatively common thing to happen while debugging it. Also: (1) there is a known bug in qemu-thread-posix.c, which should not block SIGILL, SIGBUS, SIGSEGV, SIGFPE and SIGSYS. Without fixing that, this trick will only work for the iothread and not for the VCPU threads. If you can produce a patch for this, it would be very nice. > > + int *p = NULL; > + > + *p = 0xDEADBEEF; (2) This is undefined behavior. You probably want something like "volatile int *p = (volatile int *)(intptr_t)4;" instead. Paolo
On 09/05/13 15:26, Paolo Bonzini wrote: > Il 05/09/2013 14:19, Michal Novotny ha scritto: >> This is the patch to introduce SIGILL handler to be able to trigger >> SIGSEGV signal in qemu. This has been written to help debugging >> state when qemu crashes by SIGSEGV as a simple reproducer to >> emulate such situation in case of need. > > What's wrong with "kill -11" or, within gdb, "j *0x1234"? Why do you > need a SIGILL handler for this? In fact, SIGILL is a pretty bad choice: > QEMU includes a JIT compiler, so a SIGILL is a relatively common thing > to happen while debugging it. > > Also: > > (1) there is a known bug in qemu-thread-posix.c, which should not block > SIGILL, SIGBUS, SIGSEGV, SIGFPE and SIGSYS. Without fixing that, this > trick will only work for the iothread and not for the VCPU threads. If > you can produce a patch for this, it would be very nice. > >> >> + int *p = NULL; >> + >> + *p = 0xDEADBEEF; > > (2) This is undefined behavior. You probably want something like > "volatile int *p = (volatile int *)(intptr_t)4;" instead. What's wrong with raise(SIGSEGV)? I don't understand the motivation BTW -- what sense does it make to turn SIGILL into SIGSEGV? If someone just wants to force a "coredump due to signal" interactively, SIGQUIT was invented *exactly* for that. You can even send it from the controlling terminal directly, with Ctrl-\. (More precisely, by entering QUIT character, see eg. the stty manual.) (Also, in this specific case it would be no problem if all but one threads blocked SIGQUIT -- the terminal driver or the "kill" utility would generate the signal for the entire process, not a specific thread, and then the signal would be delivered to some thread among those threads that are not blocking the signal.) Laszlo
On Thu, Sep 5, 2013 at 7:20 AM, Michal Novotny <minovotn@redhat.com> wrote: > This is the patch to introduce SIGILL handler to be able to trigger > SIGSEGV signal in qemu. This has been written to help debugging > state when qemu crashes by SIGSEGV as a simple reproducer to > emulate such situation in case of need. > > Signed-off-by: Michal Novotny <minovotn@redhat.com> > --- > vl.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/vl.c b/vl.c > index 7e04641..3966271 100644 > --- a/vl.c > +++ b/vl.c > @@ -2897,6 +2897,26 @@ static int object_create(QemuOpts *opts, void *opaque) > return 0; > } > > +#ifdef CONFIG_POSIX > +static void signal_handler(int signal) > +{ > + int *p = NULL; > + > + *p = 0xDEADBEEF; I won't repeat the questions from Paolo and Lazlo (I share their confusion) but will simply add that you cannot rely on NULL address accessing causing a SEGV. Even with all the use of volatile in the world, there's no guarantee this is going to crash. Regards, Anthony Liguori > +} > + > +static void setup_signal_handlers(void) > +{ > + struct sigaction action; > + > + memset(&action, 0, sizeof(action)); > + sigfillset(&action.sa_mask); > + action.sa_handler = signal_handler; > + action.sa_flags = 0; > + sigaction(SIGILL, &action, NULL); > +} > +#endif > + > int main(int argc, char **argv, char **envp) > { > int i; > @@ -2945,6 +2965,10 @@ int main(int argc, char **argv, char **envp) > #endif > } > > +#ifdef CONFIG_POSIX > + setup_signal_handlers(); > +#endif > + > module_call_init(MODULE_INIT_QOM); > > qemu_add_opts(&qemu_drive_opts); > -- > 1.7.11.7 >
On 09/05/2013 04:50 PM, Anthony Liguori wrote: >> + int *p = NULL; >> + >> + *p = 0xDEADBEEF; > > I won't repeat the questions from Paolo and Lazlo (I share their > confusion) but will simply add that you cannot rely on NULL address > accessing causing a SEGV. Even with all the use of volatile in the > world, there's no guarantee this is going to crash. If you want to guarantee that a write would cause a SEGV, then you have to use mmap(MAP_ANONYMOUS|MAP_PRIVATE) + mprotect(PROT_NONE) to get a valid unwritable pointer that will reliably fault, rather than hoping that NULL (or any other low-valued intptr_t cast to void*) is sufficiently protected. But I also echo the question: why is raise() insufficient?
On 09/06/2013 12:50 AM, Anthony Liguori wrote: > On Thu, Sep 5, 2013 at 7:20 AM, Michal Novotny <minovotn@redhat.com> wrote: >> This is the patch to introduce SIGILL handler to be able to trigger >> SIGSEGV signal in qemu. This has been written to help debugging >> state when qemu crashes by SIGSEGV as a simple reproducer to >> emulate such situation in case of need. >> >> Signed-off-by: Michal Novotny <minovotn@redhat.com> >> --- >> vl.c | 24 ++++++++++++++++++++++++ >> 1 file changed, 24 insertions(+) >> >> diff --git a/vl.c b/vl.c >> index 7e04641..3966271 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2897,6 +2897,26 @@ static int object_create(QemuOpts *opts, void *opaque) >> return 0; >> } >> >> +#ifdef CONFIG_POSIX >> +static void signal_handler(int signal) >> +{ >> + int *p = NULL; >> + >> + *p = 0xDEADBEEF; > I won't repeat the questions from Paolo and Lazlo (I share their > confusion) but will simply add that you cannot rely on NULL address > accessing causing a SEGV. Even with all the use of volatile in the > world, there's no guarantee this is going to crash. > > Regards, > > Anthony Liguori The idea was to trigger SIGSEGV (working at least at test conditions) to find out current qemu state. Of course, using gdb is also an option. Please ignore this patch, it was rather one purpose patch used in testing... Thanks, Michal > >> +} >> + >> +static void setup_signal_handlers(void) >> +{ >> + struct sigaction action; >> + >> + memset(&action, 0, sizeof(action)); >> + sigfillset(&action.sa_mask); >> + action.sa_handler = signal_handler; >> + action.sa_flags = 0; >> + sigaction(SIGILL, &action, NULL); >> +} >> +#endif >> + >> int main(int argc, char **argv, char **envp) >> { >> int i; >> @@ -2945,6 +2965,10 @@ int main(int argc, char **argv, char **envp) >> #endif >> } >> >> +#ifdef CONFIG_POSIX >> + setup_signal_handlers(); >> +#endif >> + >> module_call_init(MODULE_INIT_QOM); >> >> qemu_add_opts(&qemu_drive_opts); >> -- >> 1.7.11.7 >>
diff --git a/vl.c b/vl.c index 7e04641..3966271 100644 --- a/vl.c +++ b/vl.c @@ -2897,6 +2897,26 @@ static int object_create(QemuOpts *opts, void *opaque) return 0; } +#ifdef CONFIG_POSIX +static void signal_handler(int signal) +{ + int *p = NULL; + + *p = 0xDEADBEEF; +} + +static void setup_signal_handlers(void) +{ + struct sigaction action; + + memset(&action, 0, sizeof(action)); + sigfillset(&action.sa_mask); + action.sa_handler = signal_handler; + action.sa_flags = 0; + sigaction(SIGILL, &action, NULL); +} +#endif + int main(int argc, char **argv, char **envp) { int i; @@ -2945,6 +2965,10 @@ int main(int argc, char **argv, char **envp) #endif } +#ifdef CONFIG_POSIX + setup_signal_handlers(); +#endif + module_call_init(MODULE_INIT_QOM); qemu_add_opts(&qemu_drive_opts);
This is the patch to introduce SIGILL handler to be able to trigger SIGSEGV signal in qemu. This has been written to help debugging state when qemu crashes by SIGSEGV as a simple reproducer to emulate such situation in case of need. Signed-off-by: Michal Novotny <minovotn@redhat.com> --- vl.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)