Re: [PATCH v1] linux-user: Add option to run `execve`d programs through QEMU

["Alex Bennée" <alex.bennee@linaro.org>]

Noah Goldstein <goldstein.w.n@gmail.com> writes:

> The new option '-qemu-children' makes it so that on `execve` the child
> process will be launch by the same `qemu` executable that is currently
> running along with its current commandline arguments.
>
> The motivation for the change is to make it so that plugins running
> through `qemu` can continue to run on children.  Why not just
> `binfmt`?: Plugins can be desirable regardless of system/architecture
> emulation, and can sometimes be useful for elf files that can run
> natively. Enabling `binfmt` for all natively runnable elf files may
> not be desirable.

Broadly I don't see anything wrong with the patch. However my principle
concern is how test it. Currently nothing in check-tcg ensures this
mechanism works and stresses the argument copying.

>
> Signed-off-by: Noah Goldstein <goldstein.w.n@gmail.com>
> ---
>  linux-user/main.c           | 22 ++++++++++++++++++++++
>  linux-user/syscall.c        | 20 +++++++++++++++-----
>  linux-user/user-internals.h |  4 ++++
>  3 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 8143a0d4b0..dfb303a1f2 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -81,6 +81,10 @@ unsigned long mmap_min_addr;
>  uintptr_t guest_base;
>  bool have_guest_base;
>  
> +bool qemu_dup_for_children;
> +int qemu_argc;
> +char ** qemu_argv;
> +
>  /*
>   * Used to implement backwards-compatibility for the `-strace`, and
>   * QEMU_STRACE options. Without this, the QEMU_LOG can be overwritten by
> @@ -451,6 +455,11 @@ static void handle_arg_jitdump(const char *arg)
>      perf_enable_jitdump();
>  }
>  
> +static void handle_arg_qemu_children(const char *arg)
> +{
> +    qemu_dup_for_children = true;
> +}
> +
>  static QemuPluginList plugins = QTAILQ_HEAD_INITIALIZER(plugins);
>  
>  #ifdef CONFIG_PLUGIN
> @@ -526,6 +535,10 @@ static const struct qemu_argument arg_table[] = {
>       "",           "Generate a /tmp/perf-${pid}.map file for perf"},
>      {"jitdump",    "QEMU_JITDUMP",     false, handle_arg_jitdump,
>       "",           "Generate a jit-${pid}.dump file for perf"},
> +    {"qemu-children",
> +                   "QEMU_CHILDREN",    false, handle_arg_qemu_children,
> +     "",           "Run child processes (created with execve) with qemu "
> +                   "(as instantiated for the parent)"},
>      {NULL, NULL, false, NULL, NULL, NULL}
>  };
>  
> @@ -729,6 +742,15 @@ int main(int argc, char **argv, char **envp)
>  
>      optind = parse_args(argc, argv);
>  
> +    if (qemu_dup_for_children) {
> +        int i;
> +        qemu_argc = optind;
> +        qemu_argv = g_new0(char *, qemu_argc);
> +        for (i = 0; i < optind; ++i) {
> +            qemu_argv[i] = strdup(argv[i]);
> +        }
> +    }
> +
>      qemu_set_log_filename_flags(last_log_filename,
>                                  last_log_mask | (enable_strace * LOG_STRACE),
>                                  &error_fatal);
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9d5415674d..732ef89054 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8459,13 +8459,14 @@ static int do_execv(CPUArchState *cpu_env, int dirfd,
>                      abi_long pathname, abi_long guest_argp,
>                      abi_long guest_envp, int flags, bool is_execveat)
>  {
> -    int ret;
> +    int ret, argp_offset;
>      char **argp, **envp;
>      int argc, envc;
>      abi_ulong gp;
>      abi_ulong addr;
>      char **q;
>      void *p;
> +    bool through_qemu = !is_execveat && qemu_dup_for_children;
>  
>      argc = 0;
>  
> @@ -8489,10 +8490,11 @@ static int do_execv(CPUArchState *cpu_env, int dirfd,
>          envc++;
>      }
>  
> -    argp = g_new0(char *, argc + 1);
> +    argp_offset = through_qemu ? qemu_argc : 0;
> +    argp = g_new0(char *, argc + argp_offset + 1);
>      envp = g_new0(char *, envc + 1);
>  
> -    for (gp = guest_argp, q = argp; gp; gp += sizeof(abi_ulong), q++) {
> +    for (gp = guest_argp, q = argp + argp_offset; gp; gp += sizeof(abi_ulong), q++) {
>          if (get_user_ual(addr, gp)) {
>              goto execve_efault;
>          }
> @@ -8537,9 +8539,17 @@ static int do_execv(CPUArchState *cpu_env, int dirfd,
>      }
>  
>      const char *exe = p;
> -    if (is_proc_myself(p, "exe")) {
> +    if (through_qemu) {
> +        int i;
> +        for (i = 0; i < argp_offset; ++i) {
> +            argp[i] = qemu_argv[i];
> +        }
> +        exe = qemu_argv[0];
> +    }
> +    else if (is_proc_myself(p, "exe")) {
>          exe = exec_path;
>      }
> +
>      ret = is_execveat
>          ? safe_execveat(dirfd, exe, argp, envp, flags)
>          : safe_execve(exe, argp, envp);
> @@ -8553,7 +8563,7 @@ execve_efault:
>      ret = -TARGET_EFAULT;
>  
>  execve_end:
> -    for (gp = guest_argp, q = argp; *q; gp += sizeof(abi_ulong), q++) {
> +    for (gp = guest_argp, q = argp + argp_offset; *q; gp += sizeof(abi_ulong), q++) {
>          if (get_user_ual(addr, gp) || !addr) {
>              break;
>          }
> diff --git a/linux-user/user-internals.h b/linux-user/user-internals.h
> index 5c7f173ceb..0719e65ff4 100644
> --- a/linux-user/user-internals.h
> +++ b/linux-user/user-internals.h
> @@ -30,6 +30,10 @@ void stop_all_tasks(void);
>  extern const char *qemu_uname_release;
>  extern unsigned long mmap_min_addr;
>  
> +extern bool qemu_dup_for_children;
> +extern int qemu_argc;
> +extern char ** qemu_argv;
> +
>  typedef struct IOCTLEntry IOCTLEntry;
>  
>  typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro