Re: [Qemu-devel] [PATCH v7 62/74] linux-user: Split out rt_sigqueueinfo, rt_tgsigqueueinfo

[Aleksandar Markovic <aleksandar.m.mail@gmail.com>]

On May 19, 2019 11:25 PM, "Richard Henderson" <richard.henderson@linaro.org>
wrote:
>
> This does drop the (questionable) siginfo_t printing.
> But since we already do not handle more important things
> in this area like sigset_t, this does not feel a loss.
>

What is the most questionable here is the reasoning “we don't handle one
thing, therefore let's drop another”.

If you find siginfo_t printing questionable, then provide a better one, not
remove it altogether.

I find that the current printing is useful in many circumstances, there is
a lot of energy of former contributors invested into it, and should not be
deleted just like that. It does feel like a loss.

Aleksandar

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/syscall-defs.h    |   2 +
>  linux-user/strace.c          | 138 -----------------------------------
>  linux-user/syscall-sig.inc.c |  30 ++++++++
>  linux-user/syscall.c         |  26 -------
>  linux-user/strace.list       |   6 --
>  5 files changed, 32 insertions(+), 170 deletions(-)
>
> diff --git a/linux-user/syscall-defs.h b/linux-user/syscall-defs.h
> index 24289ed413..11851535e1 100644
> --- a/linux-user/syscall-defs.h
> +++ b/linux-user/syscall-defs.h
> @@ -196,8 +196,10 @@ SYSCALL_DEF(rt_sigaction, ARG_SIGNAL, ARG_PTR,
ARG_PTR, ARG_DEC);
>  #endif
>  SYSCALL_DEF(rt_sigpending, ARG_PTR, ARG_DEC);
>  SYSCALL_DEF(rt_sigprocmask, ARG_SIGPROCMASKHOW, ARG_PTR, ARG_PTR,
ARG_DEC);
> +SYSCALL_DEF(rt_sigqueueinfo, ARG_DEC, ARG_SIGNAL, ARG_PTR);
>  SYSCALL_DEF(rt_sigsuspend, ARG_PTR, ARG_DEC);
>  SYSCALL_DEF(rt_sigtimedwait, ARG_PTR, ARG_PTR, ARG_PTR, ARG_DEC);
> +SYSCALL_DEF(rt_tgsigqueueinfo, ARG_DEC, ARG_DEC, ARG_SIGNAL, ARG_PTR);
>  #if !defined(SYSCALL_TABLE) || defined(TARGET_NR_semctl)
>  SYSCALL_DEF(semctl, ARG_DEC, ARG_DEC, ARG_DEC, ARG_HEX);
>  #endif
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 886663af2e..2e70a3910c 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -167,93 +167,6 @@ static void print_si_code(int arg)
>      gemu_log("%s", codename);
>  }
>
> -static void get_target_siginfo(target_siginfo_t *tinfo,
> -                                const target_siginfo_t *info)
> -{
> -    abi_ulong sival_ptr;
> -
> -    int sig;
> -    int si_errno;
> -    int si_code;
> -    int si_type;
> -
> -    __get_user(sig, &info->si_signo);
> -    __get_user(si_errno, &tinfo->si_errno);
> -    __get_user(si_code, &info->si_code);
> -
> -    tinfo->si_signo = sig;
> -    tinfo->si_errno = si_errno;
> -    tinfo->si_code = si_code;
> -
> -    /* Ensure we don't leak random junk to the guest later */
> -    memset(tinfo->_sifields._pad, 0, sizeof(tinfo->_sifields._pad));
> -
> -    /* This is awkward, because we have to use a combination of
> -     * the si_code and si_signo to figure out which of the union's
> -     * members are valid. (Within the host kernel it is always possible
> -     * to tell, but the kernel carefully avoids giving userspace the
> -     * high 16 bits of si_code, so we don't have the information to
> -     * do this the easy way...) We therefore make our best guess,
> -     * bearing in mind that a guest can spoof most of the si_codes
> -     * via rt_sigqueueinfo() if it likes.
> -     *
> -     * Once we have made our guess, we record it in the top 16 bits of
> -     * the si_code, so that print_siginfo() later can use it.
> -     * print_siginfo() will strip these top bits out before printing
> -     * the si_code.
> -     */
> -
> -    switch (si_code) {
> -    case SI_USER:
> -    case SI_TKILL:
> -    case SI_KERNEL:
> -        /* Sent via kill(), tkill() or tgkill(), or direct from the
kernel.
> -         * These are the only unspoofable si_code values.
> -         */
> -        __get_user(tinfo->_sifields._kill._pid,
&info->_sifields._kill._pid);
> -        __get_user(tinfo->_sifields._kill._uid,
&info->_sifields._kill._uid);
> -        si_type = QEMU_SI_KILL;
> -        break;
> -    default:
> -        /* Everything else is spoofable. Make best guess based on signal
*/
> -        switch (sig) {
> -        case TARGET_SIGCHLD:
> -            __get_user(tinfo->_sifields._sigchld._pid,
> -                       &info->_sifields._sigchld._pid);
> -            __get_user(tinfo->_sifields._sigchld._uid,
> -                       &info->_sifields._sigchld._uid);
> -            __get_user(tinfo->_sifields._sigchld._status,
> -                       &info->_sifields._sigchld._status);
> -            __get_user(tinfo->_sifields._sigchld._utime,
> -                       &info->_sifields._sigchld._utime);
> -            __get_user(tinfo->_sifields._sigchld._stime,
> -                       &info->_sifields._sigchld._stime);
> -            si_type = QEMU_SI_CHLD;
> -            break;
> -        case TARGET_SIGIO:
> -            __get_user(tinfo->_sifields._sigpoll._band,
> -                       &info->_sifields._sigpoll._band);
> -            __get_user(tinfo->_sifields._sigpoll._fd,
> -                       &info->_sifields._sigpoll._fd);
> -            si_type = QEMU_SI_POLL;
> -            break;
> -        default:
> -            /* Assume a sigqueue()/mq_notify()/rt_sigqueueinfo() source.
*/
> -            __get_user(tinfo->_sifields._rt._pid,
&info->_sifields._rt._pid);
> -            __get_user(tinfo->_sifields._rt._uid,
&info->_sifields._rt._uid);
> -            /* XXX: potential problem if 64 bit */
> -            __get_user(sival_ptr,
&info->_sifields._rt._sigval.sival_ptr);
> -            tinfo->_sifields._rt._sigval.sival_ptr = sival_ptr;
> -
> -            si_type = QEMU_SI_RT;
> -            break;
> -        }
> -        break;
> -    }
> -
> -    tinfo->si_code = deposit32(si_code, 16, 16, si_type);
> -}
> -
>  static void print_siginfo(const target_siginfo_t *tinfo)
>  {
>      /* Print a target_siginfo_t in the format desired for printing
> @@ -1585,57 +1498,6 @@ print_fstat(const struct syscallname *name,
>  #define print_fstat64     print_fstat
>  #endif
>
> -#ifdef TARGET_NR_rt_sigqueueinfo
> -static void
> -print_rt_sigqueueinfo(const struct syscallname *name,
> -    abi_long arg0, abi_long arg1, abi_long arg2,
> -    abi_long arg3, abi_long arg4, abi_long arg5)
> -{
> -    void *p;
> -    target_siginfo_t uinfo;
> -
> -    print_syscall_prologue(name);
> -    print_raw_param("%d", arg0, 0);
> -    print_signal(arg1, 0);
> -    p = lock_user(VERIFY_READ, arg2, sizeof(target_siginfo_t), 1);
> -    if (p) {
> -        get_target_siginfo(&uinfo, p);
> -        print_siginfo(&uinfo);
> -
> -        unlock_user(p, arg2, 0);
> -    } else {
> -        print_pointer(arg2, 1);
> -    }
> -    print_syscall_epilogue(name);
> -}
> -#endif
> -
> -#ifdef TARGET_NR_rt_tgsigqueueinfo
> -static void
> -print_rt_tgsigqueueinfo(const struct syscallname *name,
> -    abi_long arg0, abi_long arg1, abi_long arg2,
> -    abi_long arg3, abi_long arg4, abi_long arg5)
> -{
> -    void *p;
> -    target_siginfo_t uinfo;
> -
> -    print_syscall_prologue(name);
> -    print_raw_param("%d", arg0, 0);
> -    print_raw_param("%d", arg1, 0);
> -    print_signal(arg2, 0);
> -    p = lock_user(VERIFY_READ, arg3, sizeof(target_siginfo_t), 1);
> -    if (p) {
> -        get_target_siginfo(&uinfo, p);
> -        print_siginfo(&uinfo);
> -
> -        unlock_user(p, arg3, 0);
> -    } else {
> -        print_pointer(arg3, 1);
> -    }
> -    print_syscall_epilogue(name);
> -}
> -#endif
> -
>  #ifdef TARGET_NR_syslog
>  static void
>  print_syslog_action(abi_ulong arg, int last)
> diff --git a/linux-user/syscall-sig.inc.c b/linux-user/syscall-sig.inc.c
> index 5f2c0ba499..774346838b 100644
> --- a/linux-user/syscall-sig.inc.c
> +++ b/linux-user/syscall-sig.inc.c
> @@ -191,6 +191,21 @@ SYSCALL_IMPL(rt_sigprocmask)
>      return ret;
>  }
>
> +SYSCALL_IMPL(rt_sigqueueinfo)
> +{
> +    siginfo_t uinfo;
> +    void *p;
> +
> +    p = lock_user(VERIFY_READ, arg3, sizeof(target_siginfo_t), 1);
> +    if (!p) {
> +        return -TARGET_EFAULT;
> +    }
> +    target_to_host_siginfo(&uinfo, p);
> +    unlock_user(p, arg3, 0);
> +
> +    return get_errno(sys_rt_sigqueueinfo(arg1, arg2, &uinfo));
> +}
> +
>  SYSCALL_IMPL(rt_sigsuspend)
>  {
>      CPUState *cpu = ENV_GET_CPU(cpu_env);
> @@ -252,6 +267,21 @@ SYSCALL_IMPL(rt_sigtimedwait)
>      return ret;
>  }
>
> +SYSCALL_IMPL(rt_tgsigqueueinfo)
> +{
> +    siginfo_t uinfo;
> +    void *p;
> +
> +    p = lock_user(VERIFY_READ, arg4, sizeof(target_siginfo_t), 1);
> +    if (!p) {
> +        return -TARGET_EFAULT;
> +    }
> +    target_to_host_siginfo(&uinfo, p);
> +    unlock_user(p, arg4, 0);
> +
> +    return get_errno(sys_rt_tgsigqueueinfo(arg1, arg2, arg3, &uinfo));
> +}
> +
>  #ifdef TARGET_NR_sigaction
>  SYSCALL_IMPL(sigaction)
>  {
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 8a05d3e32a..e489d12103 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -4240,32 +4240,6 @@ static abi_long do_syscall1(void *cpu_env, int
num, abi_long arg1,
>      void *p;
>
>      switch(num) {
> -    case TARGET_NR_rt_sigqueueinfo:
> -        {
> -            siginfo_t uinfo;
> -
> -            p = lock_user(VERIFY_READ, arg3, sizeof(target_siginfo_t),
1);
> -            if (!p) {
> -                return -TARGET_EFAULT;
> -            }
> -            target_to_host_siginfo(&uinfo, p);
> -            unlock_user(p, arg3, 0);
> -            ret = get_errno(sys_rt_sigqueueinfo(arg1, arg2, &uinfo));
> -        }
> -        return ret;
> -    case TARGET_NR_rt_tgsigqueueinfo:
> -        {
> -            siginfo_t uinfo;
> -
> -            p = lock_user(VERIFY_READ, arg4, sizeof(target_siginfo_t),
1);
> -            if (!p) {
> -                return -TARGET_EFAULT;
> -            }
> -            target_to_host_siginfo(&uinfo, p);
> -            unlock_user(p, arg4, 0);
> -            ret = get_errno(sys_rt_tgsigqueueinfo(arg1, arg2, arg3,
&uinfo));
> -        }
> -        return ret;
>  #ifdef TARGET_NR_sigreturn
>      case TARGET_NR_sigreturn:
>          if (block_signals()) {
> diff --git a/linux-user/strace.list b/linux-user/strace.list
> index 0b2c057673..57445a8d81 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -926,15 +926,9 @@
>  #ifdef TARGET_NR_rmdir
>  { TARGET_NR_rmdir, "rmdir" , NULL, NULL, NULL },
>  #endif
> -#ifdef TARGET_NR_rt_sigqueueinfo
> -{ TARGET_NR_rt_sigqueueinfo, "rt_sigqueueinfo" , NULL,
print_rt_sigqueueinfo, NULL },
> -#endif
>  #ifdef TARGET_NR_rt_sigreturn
>  { TARGET_NR_rt_sigreturn, "rt_sigreturn" , NULL, NULL, NULL },
>  #endif
> -#ifdef TARGET_NR_rt_tgsigqueueinfo
> -{ TARGET_NR_rt_tgsigqueueinfo, "rt_tgsigqueueinfo" , NULL,
print_rt_tgsigqueueinfo, NULL },
> -#endif
>  #ifdef TARGET_NR_sched_getaffinity
>  { TARGET_NR_sched_getaffinity, "sched_getaffinity" , NULL, NULL, NULL },
>  #endif
> --
> 2.17.1
>
>