Re: [Qemu-devel] [PATCH 13/34] linux-user: Fix signal before blocking system calls race and SA_RESTART

[Peter Maydell]

On 6 September 2015 at 00:57, Timothy E Baldwin
<address@hidden> wrote:

This is definitely the right design for fixing this race. Comments
below are mainly about structure, documentation and minor nits.

> If a signal is delivered immediately before a blocking system calls the

"call"

> handler will only be called after the system call returns, which may be a
> long time later or never.
>
> This is fixed by using a function (safe_syscall_base) that checks  if a guest

double space ("  ")

> signal is pending prior to making a system call, and if so does not call the
> system call and returns -TARGET_ERESTARTSYS. If a signal is received between

"but instead returns".

> the check and the system call host_signal_handler() rewinds execution to

"then host_signal_handler()"

> before the check. If the system call returns an error a guest error code
> is returned.
>
> safe_syscall_base is implemented in assembly language with initially
> only a x86-64 version, a fall back is provided keep the old behaviour.

"version. A fall back is provided until other host architectures are
converted to use this mechanism."

>
> Also setting the SA_RESTART flag would result in host system calls being
> restarted and the guest signal handler only being called when they have
> completed. This is also fixed.

Fixed only for host archs using safe_syscall, I assume?

> This commit contains general infrastructure and safe_syscall_base for x86-64.
>
> Signed-off-by: Timothy Edward Baldwin <address@hidden>
> ---
>  configure                        |  13 +++++
>  linux-user/Makefile.objs         |   3 +-
>  linux-user/qemu.h                |  17 +++++++
>  linux-user/safe_syscall/x86_64.S |  34 +++++++++++++
>  linux-user/signal.c              |  12 +++++
>  linux-user/syscall.c             | 107 
> +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 185 insertions(+), 1 deletion(-)
>  create mode 100644 linux-user/safe_syscall/x86_64.S
>
> diff --git a/configure b/configure
> index 21c4089..d7c115a 100755
> --- a/configure
> +++ b/configure
> @@ -5219,6 +5219,19 @@ if test "$gcov" = "yes" ; then
>    echo "GCOV=$gcov_tool" >> $config_host_mak
>  fi
>
> +# Which safe_syscall_base implmentation?

"implementation"

> +if test "$linux" = "yes" ; then
> +  case "$cpu" in
> +  x86_64)
> +    echo "SAFE_SYSCALL_BASE=safe_syscall/x86_64.o" >> $config_host_mak
> +    echo "CONFIG_SAFE_SYSCALL=y" >> $config_host_mak
> +    ;;
> +  *)
> +    echo "SAFE_SYSCALL_BASE=" >> $config_host_mak
> +    ;;
> +  esac
> +fi

I can't help feeling there ought to be a cleaner way than this,
but I can't think of one, so this will work. It would probably
be nice to factor out the "write safe_syscall/something.o to
config file" part, because that will get repetitive when we
get more architectures in this switch.

> +
>  # use included Linux headers
>  if test "$linux" = "yes" ; then
>    mkdir -p linux-headers
> diff --git a/linux-user/Makefile.objs b/linux-user/Makefile.objs
> index fd50217..56f9937 100644
> --- a/linux-user/Makefile.objs
> +++ b/linux-user/Makefile.objs
> @@ -1,5 +1,6 @@
>  obj-y = main.o syscall.o strace.o mmap.o signal.o \
> -       elfload.o linuxload.o uaccess.o uname.o
> +       elfload.o linuxload.o uaccess.o uname.o \
> +       $(SAFE_SYSCALL_BASE)
>
>  obj-$(TARGET_HAS_BFLT) += flatload.o
>  obj-$(TARGET_I386) += vm86.o
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 8012cc2..57c7e43 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -216,8 +216,25 @@ unsigned long init_guest_space(unsigned long host_start,
>
>  #include "qemu/log.h"
>
> +/* safe_syscall.S */
> +
> +/* Call a system call if guest signal not pending.
> + * Returns -TARGET_ESIGRETURN if signal pending.

The code seems to return -TARGET_ERESTARTSYS...

> + * Returns guest error code on error.
> + */

Doc-comment style comments for functions are preferred over
ad-hoc ones (see eg bitops.h extract32 &c for example syntax).

Worth emphasising that this is going to directly call a host
system call (ie will not go via glibc).

We should also actually mention the most important thing
about the API -- that there is no race between signals arriving
and taking the syscall, so either we return -TARGET_ERESTARTSYS,
or we enter the syscall (which then will return EINTR/etc as
appropriate when the host signal arrives).

Saying which syscalls need to be implemented using this and
which don't would also be good. (I think the distinction is
that anything that can block has to go via this, whereas
syscalls which can't block can be implemented via glibc
functions if that's easier. Is that right?)

> +
> +extern long safe_syscall_base(volatile int *pending, long number, ...);
> +#define safe_syscall(...) \
> +    safe_syscall_base(&((TaskState *)thread_cpu->opaque)->signal_pending, \
> +    __VA_ARGS__)
> +
> +/* For host_signal_handler() */
> +extern char safe_syscall_start[];
> +extern char safe_syscall_end[];
> +
>  /* syscall.c */
>  int host_to_target_waitstatus(int status);
> +abi_long convert_syscall_return_value(abi_long ret);

Any new global function should have a doc comment.

>  /* strace.c */
>  void print_syscall(int num,
> diff --git a/linux-user/safe_syscall/x86_64.S 
> b/linux-user/safe_syscall/x86_64.S
> new file mode 100644
> index 0000000..2e32563
> --- /dev/null
> +++ b/linux-user/safe_syscall/x86_64.S
> @@ -0,0 +1,34 @@
> +#include "errno_defs.h"

New source files need a comment at the top which gives the
copyright and license information and a brief description of
what the file is.

> +
> +        .global safe_syscall_base
> +        .global safe_syscall_start
> +        .global safe_syscall_end
> +        .type   safe_syscall_base, @function
> +
> +safe_syscall_base:

This definitely needs a comment describing the ABI on entry,
at a minimum.

> +        push    %rbp
> +        mov     %rsi, %rax // Move syscall number
> +        mov     %rdi, %rbp // Move signal_pending pointer
> +
> +        mov     %rdx, %rdi // Move syscall arguments
> +        mov     %rcx, %rsi
> +        mov     %r8,  %rdx
> +        mov     %r9,  %r10
> +        mov     16(%rsp), %r8
> +        mov     24(%rsp), %r9
> +
> +safe_syscall_start:
> +        testl   $1, (%rbp) // Check signal_pending
> +        jnz     return_ERESTARTSYS
> +        syscall
> +safe_syscall_end:

These labels are deep magic and need a suitable comment to
let the reader know that weird things can happen to the
flow control in this function that are not apparent at
first glance.

(Consider that most people trying to add support for
a new target architecture will probably do so by
cribbing from this existing implementation; we should
document what those readers need to know to get it right.)

> +        pop     %rbp
> +        mov     %rax, %rdi
> +        jmp     convert_syscall_return_value
> +
> +return_ERESTARTSYS:
> +        mov     $-TARGET_ERESTARTSYS, %rax
> +        pop     %rbp
> +        ret
> +
> +        .size   safe_syscall_base, .-safe_syscall_base
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 5cf75a4..09551ba 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -569,6 +569,10 @@ int queue_signal(CPUArchState *env, int sig, 
> target_siginfo_t *info)
>      }
>  }
>
> +#if defined(__x86_64__)
> +#define PC_SIG (uc->uc_mcontext.gregs[REG_RIP])
> +#endif

This is going to turn into an ifdef ladder for every host
architecture we support. We have too many ifdef ladders
already -- can you find a way to structure this so per-arch
defines and functions live in their own files and are
pulled in correctly by the build system, please?

(One possibility that comes to mind is adding another
directory to the include path, which is how we handle
linux-headers and tcg/ backends. That would probably also
let you automatically #include a linux-user/host-arch/safe_syscall.S
somehow without having to put something special in configure
for that part. I haven't thought the details through too
carefully here yet.)

> +
>  static void host_signal_handler(int host_signum, siginfo_t *info,
>                                  void *puc)
>  {
> @@ -591,6 +595,14 @@ static void host_signal_handler(int host_signum, 
> siginfo_t *info,
>  #if defined(DEBUG_SIGNAL)
>      fprintf(stderr, "qemu: got signal %d\n", sig);
>  #endif
> +
> +    struct ucontext *uc = puc;
> +#ifdef CONFIG_SAFE_SYSCALL
> +    if (PC_SIG > (uintptr_t)safe_syscall_start
> +            && PC_SIG < (uintptr_t)safe_syscall_end)
> +        PC_SIG = (uintptr_t)safe_syscall_start;

Is this going to work for SPARC, with its pc/npc distinction?
Or do we need a more complex abstraction?

> +#endif
> +
>      host_to_target_siginfo_noswap(&tinfo, info);
>      if (queue_signal(env, sig, &tinfo) == 1) {
>          /* interrupt the virtual CPU as soon as possible */
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 80b8fa8..b9ad7b6 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -599,6 +599,113 @@ char *target_strerror(int err)
>      return strerror(target_to_host_errno(err));
>  }
>
> +abi_long convert_syscall_return_value(abi_long ret) {
> +    if (is_error(ret)) {
> +        ret = host_to_target_errno(ret);
> +    }
> +    return ret;
> +}
> +
> +#if defined(CONFIG_SAFE_SYSCALL) && defined(TARGET_USE_ERESTARTSYS)
> +
> +#define safe_syscall0(type, name) \
> +static type safe_##name (void) \
> +{ \
> +    return safe_syscall(__NR_##name); \
> +}
> +
> +#define safe_syscall1(type, name, type1, arg1) \
> +static type safe_##name (type1 arg1) \
> +{ \
> +    return safe_syscall(__NR_##name, arg1); \
> +}
> +
> +#define safe_syscall2(type, name, type1, arg1, type2, arg2) \
> +static type safe_##name (type1 arg1, type2 arg2) \
> +{ \
> +    return safe_syscall(__NR_##name, arg1, arg2); \
> +}
> +
> +#define safe_syscall3(type, name, type1, arg1, type2, arg2, type3, arg3) \
> +static type safe_##name (type1 arg1, type2 arg2, type3 arg3) \
> +{ \
> +    return safe_syscall(__NR_##name, arg1, arg2, arg3); \
> +}
> +
> +#define safe_syscall4(type, name, type1, arg1, type2, arg2, type3, arg3, \
> +    type4, arg4) \
> +static type safe_##name (type1 arg1, type2 arg2, type3 arg3, type4 arg4) \
> +{ \
> +    return safe_syscall(__NR_##name, arg1, arg2, arg3, arg4); \
> +}
> +
> +#define safe_syscall5(type, name, type1, arg1, type2, arg2, type3, arg3, \
> +    type4, arg4, type5, arg5) \
> +static type safe_##name (type1 arg1, type2 arg2, type3 arg3, type4 arg4, \
> +    type5 arg5) \
> +{ \
> +    return safe_syscall(__NR_##name, arg1, arg2, arg3, arg4, arg5); \
> +}
> +
> +#define safe_syscall6(type, name, type1, arg1, type2, arg2, type3, arg3, \
> +    type4, arg4, type5, arg5, type6, arg6) \
> +static type safe_##name (type1 arg1, type2 arg2, type3 arg3, type4 arg4, \
> +    type5 arg5, type6 arg6) \
> +{ \
> +    return safe_syscall(__NR_##name, arg1, arg2, arg3, arg4, arg5, arg6); \
> +}
> +
> +#else
> +
> +#define safe_syscall0(type, name) \
> +static type safe_##name (void) \
> +{ \
> +    return get_errno(name()); \
> +}

Should these fallbacks call the syscall, not the libc function?

> +#define safe_syscall1(type, name, type1, arg1) \
> +static type safe_##name (type1 arg1) \
> +{ \
> +    return get_errno(name(arg1)); \
> +}
> +
> +#define safe_syscall2(type, name, type1, arg1, type2, arg2) \
> +static type safe_##name (type1 arg1, type2 arg2) \
> +{ \
> +    return get_errno(name(arg1, arg2)); \
> +}
> +
> +#define safe_syscall3(type, name, type1, arg1, type2, arg2, type3, arg3) \
> +static type safe_##name (type1 arg1, type2 arg2, type3 arg3) \
> +{ \
> +    return get_errno(name(arg1, arg2, arg3)); \
> +}
> +
> +#define safe_syscall4(type, name, type1, arg1, type2, arg2, type3, arg3, \
> +    type4, arg4) \
> +static type safe_##name (type1 arg1, type2 arg2, type3 arg3, type4 arg4) \
> +{ \
> +    return get_errno(name(arg1, arg2, arg3, arg4)); \
> +}
> +
> +#define safe_syscall5(type, name, type1, arg1, type2, arg2, type3, arg3, \
> +    type4, arg4, type5, arg5) \
> +static type safe_##name (type1 arg1, type2 arg2, type3 arg3, type4 arg4, \
> +    type5 arg5) \
> +{ \
> +    return get_errno(name(arg1, arg2, arg3, arg4, arg5)); \
> +}
> +
> +#define safe_syscall6(type, name, type1, arg1, type2, arg2, type3, arg3, \
> +    type4, arg4, type5, arg5, type6, arg6) \
> +static type safe_##name (type1 arg1, type2 arg2, type3 arg3, type4 arg4, \
> +    type5 arg5, type6 arg6)  \
> +{ \
> +    return get_errno(name(arg1, arg2, arg3, arg4, arg5, arg6)); \
> +}
> +
> +#endif
> +
>  static inline int host_to_target_sock_type(int host_type)
>  {
>      int target_type;
> --
> 2.1.4

thanks
-- PMM