Re: [Qemu-devel] [PATCH v1 03/17] threads: add infrastructure to process sigsegv

[Peter Xu]

On Wed, Jul 18, 2018 at 06:41:46PM +0300, Denis Plotnikov wrote:
> Allows to define sigsegv handler temporary for all threads.
> This is useful to implement copy-on-write logic while
> linux usefaultfd doesn't support write-protected faults.
> In the future, switch to using WP userfaultfd when it's
> available.
> 
> Signed-off-by: Denis Plotnikov <address@hidden>
> ---
>  include/qemu/thread.h    |  5 ++++
>  util/qemu-thread-posix.c | 52 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index 9910f49b3a..d6fed833fa 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -210,4 +210,9 @@ void qemu_lockcnt_inc_and_unlock(QemuLockCnt *lockcnt);
>   */
>  unsigned qemu_lockcnt_count(QemuLockCnt *lockcnt);
>  
> +
> +typedef void (*sigsegv_handler)(int signum, siginfo_t *siginfo, void 
> *sigctx);
> +void sigsegv_user_handler_set(sigsegv_handler handler);
> +void sigsegv_user_handler_reset(void);
> +
>  #endif
> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index 7306475899..5424b7106d 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -489,6 +489,47 @@ static void qemu_thread_set_name(QemuThread *thread, 
> const char *name)
>  #endif
>  }
>  
> +static sigsegv_handler sigsegv_user_handler;
> +
> +void sigsegv_user_handler_set(sigsegv_handler handler)
> +{
> +    assert(handler);

We can also assert !sigsegv_user_handler here to make sure there won't
accidentally be two users that want to specify a SIGSEGV handler in
parallel.

> +    atomic_set(&sigsegv_user_handler, handler);
> +}
> +
> +static sigsegv_handler sigsegv_user_handler_get(void)
> +{
> +    return atomic_read(&sigsegv_user_handler);
> +}
> +
> +void sigsegv_user_handler_reset(void)
> +{
> +    atomic_set(&sigsegv_user_handler, NULL);
> +}
> +
> +static void sigsegv_default_handler(int signum, siginfo_t *siginfo, void 
> *sigctx)
> +{
> +    sigsegv_handler handler = sigsegv_user_handler_get();
> +
> +    if (!handler) {
> +        /*
> +         * remove the sigsegv handler if it's not set by user
> +         * this will lead to re-raising the error without a handler
> +         * and exiting from the program with "Segmentation fault"
> +         */
> +        int err;
> +        struct sigaction act;
> +        memset(&act, 0, sizeof(act));
> +        act.sa_flags = SA_RESETHAND;

I'm not sure whether this is the correct way to use SA_RESETHAND.

  SA_RESETHAND
        Restore the signal action to the default upon entry to the
        signal handler.  This flag is meaningful only when
        establishing a signal han‐ dler.  SA_ONESHOT is an obsolete,
        nonstandard synonym for this flag.

My understanding is that it's used as a "oneshot" pattern, but here
you didn't really specify any handler.  My wild guess is that the
handler is actually specified to SIG_DFL (which is, 0) after you
bzero-ed the sigaction, hence got what we want.

I'm not sure whether we need this complexity - could we just unset the
handler outside itself when reset?

> +        err = sigaction(SIGSEGV, &act, NULL);
> +        if (err) {
> +            error_exit(err, __func__);
> +        }
> +    } else {
> +        handler(signum, siginfo, sigctx);

Here not sure whether we can return something from the user handler
showing that whether it correctly handled the SIGSEGV signal,
otherwise how could we identify it's an expected event or a real
SIGSEGV fault?

> +    }
> +}
> +
>  void qemu_thread_create(QemuThread *thread, const char *name,
>                         void *(*start_routine)(void*),
>                         void *arg, int mode)
> @@ -496,14 +537,25 @@ void qemu_thread_create(QemuThread *thread, const char 
> *name,
>      sigset_t set, oldset;
>      int err;
>      pthread_attr_t attr;
> +    struct sigaction act;
>  
>      err = pthread_attr_init(&attr);
>      if (err) {
>          error_exit(err, __func__);
>      }
>  
> +    memset(&act, 0, sizeof(act));
> +    act.sa_flags = SA_SIGINFO;
> +    act.sa_sigaction = sigsegv_default_handler;
> +    err = sigaction(SIGSEGV, &act, NULL);
> +    if (err) {
> +        error_exit(err, __func__);
> +    }
> +
>      /* Leave signal handling to the iothread.  */
>      sigfillset(&set);
> +    /* ...all but SIGSEGV */
> +    sigdelset(&set, SIGSEGV);

Why we need to enable SIGSEGV on all threads explicitly?  Could we
just keep the old behavior just like what the other signals do?

>      pthread_sigmask(SIG_SETMASK, &set, &oldset);
>      err = pthread_create(&thread->thread, &attr, start_routine, arg);
>      if (err)
> -- 
> 2.17.0
> 
> 

Regards,

-- 
Peter Xu