Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit

[Stefan Bruens <stefan.bruens@rwth-aachen.de>]

On Sunday 23 August 2015 01:47:52 Stefan Brüns wrote:
> Instead of creating a temporary copy for the whole environment and
> the arguments, directly copy everything to the target stack.
> 
> For this to work, we have to change the order of stack creation and
> copying the arguments.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>  linux-user/elfload.c   | 105
> +++++++++++++++++++++++-------------------------- linux-user/linuxload.c | 
>  7 +---
>  linux-user/qemu.h      |   7 ----
>  linux-user/syscall.c   |   6 ---
>  4 files changed, 51 insertions(+), 74 deletions(-)

Any comments are very appreciated.

Kind regards,

Stefan

> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 1788368..62feaf7 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1365,66 +1365,69 @@ static bool elf_check_ehdr(struct elfhdr *ehdr)
>   * to be put directly into the top of new user memory.
>   *
>   */
> -static abi_ulong copy_elf_strings(int argc,char ** argv, void **page,
> -                                  abi_ulong p)
> +static abi_ulong copy_elf_strings(int argc,char ** argv, char* scratch,
> +                                  abi_ulong p, abi_ulong stack_limit)
>  {
> -    char *tmp, *tmp1, *pag = NULL;
> -    int len, offset = 0;
> +    char *tmp;
> +    int len, offset;
> +    abi_ulong top = p;
> 
>      if (!p) {
>          return 0;       /* bullet-proofing */
>      }
> +
> +    offset = ((p - 1) % TARGET_PAGE_SIZE) + 1;
> +
>      while (argc-- > 0) {
>          tmp = argv[argc];
>          if (!tmp) {
>              fprintf(stderr, "VFS: argc is wrong");
>              exit(-1);
>          }
> -        tmp1 = tmp;
> -        while (*tmp++);
> -        len = tmp - tmp1;
> -        if (p < len) {  /* this shouldn't happen - 128kB */
> +        len = strlen(tmp) + 1;
> +        tmp += len;
> +
> +        if (len > (p - stack_limit)) {
>              return 0;
>          }
>          while (len) {
> -            --p; --tmp; --len;
> -            if (--offset < 0) {
> -                offset = p % TARGET_PAGE_SIZE;
> -                pag = (char *)page[p/TARGET_PAGE_SIZE];
> -                if (!pag) {
> -                    pag = g_try_malloc0(TARGET_PAGE_SIZE);
> -                    page[p/TARGET_PAGE_SIZE] = pag;
> -                    if (!pag)
> -                        return 0;
> -                }
> -            }
> -            if (len == 0 || offset == 0) {
> -                *(pag + offset) = *tmp;
> +            int bytes_to_copy = (len > offset) ? offset : len;
> +            tmp -= bytes_to_copy;
> +            p -= bytes_to_copy;
> +            offset -= bytes_to_copy;
> +            len -= bytes_to_copy;
> +
> +            if (bytes_to_copy == 1) {
> +                *(scratch + offset) = *tmp;
> +            } else {
> +                memcpy_fromfs(scratch + offset, tmp, bytes_to_copy);
>              }
> -            else {
> -                int bytes_to_copy = (len > offset) ? offset : len;
> -                tmp -= bytes_to_copy;
> -                p -= bytes_to_copy;
> -                offset -= bytes_to_copy;
> -                len -= bytes_to_copy;
> -                memcpy_fromfs(pag + offset, tmp, bytes_to_copy + 1);
> +
> +            if (offset == 0) {
> +                memcpy_to_target(p, scratch, top - p);
> +                top = p;
> +                offset = TARGET_PAGE_SIZE;
>              }
>          }
>      }
> +    if (offset)
> +        memcpy_to_target(p, scratch + offset, top - p);
> +
>      return p;
>  }
> 
> -static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm,
> +static abi_ulong setup_arg_pages(struct linux_binprm *bprm,
>                                   struct image_info *info)
>  {
> -    abi_ulong stack_base, size, error, guard;
> -    int i;
> +    abi_ulong size, error, guard;
> +
> +    /* Linux kernel limits argument/environment space to 1/4th of stack
> size, +       but also has a floor of 32 pages. Mimic this behaviour.  */ +
>    #define MAX_ARG_PAGES 32
> 
> -    /* Create enough stack to hold everything.  If we don't use
> -       it for args, we'll use it for something else.  */
>      size = guest_stack_size;
> -    if (size < MAX_ARG_PAGES*TARGET_PAGE_SIZE) {
> -        size = MAX_ARG_PAGES*TARGET_PAGE_SIZE;
> +    if (size < MAX_ARG_PAGES * TARGET_PAGE_SIZE) {
> +        size = MAX_ARG_PAGES * TARGET_PAGE_SIZE;
>      }
>      guard = TARGET_PAGE_SIZE;
>      if (guard < qemu_real_host_page_size) {
> @@ -1442,19 +1445,8 @@ static abi_ulong setup_arg_pages(abi_ulong p, struct
> linux_binprm *bprm, target_mprotect(error, guard, PROT_NONE);
> 
>      info->stack_limit = error + guard;
> -    stack_base = info->stack_limit + size - MAX_ARG_PAGES*TARGET_PAGE_SIZE;
> -    p += stack_base;
> -
> -    for (i = 0 ; i < MAX_ARG_PAGES ; i++) {
> -        if (bprm->page[i]) {
> -            info->rss++;
> -            /* FIXME - check return value of memcpy_to_target() for failure
> */ -            memcpy_to_target(stack_base, bprm->page[i],
> TARGET_PAGE_SIZE); -            g_free(bprm->page[i]);
> -        }
> -        stack_base += TARGET_PAGE_SIZE;
> -    }
> -    return p;
> +
> +    return info->stack_limit + size - sizeof(void*);
>  }
> 
>  /* Map and zero the bss.  We need to explicitly zero any fractional pages
> @@ -2198,6 +2190,7 @@ int load_elf_binary(struct linux_binprm *bprm, struct
> image_info *info) struct image_info interp_info;
>      struct elfhdr elf_ex;
>      char *elf_interpreter = NULL;
> +    void **scratch;
> 
>      info->start_mmap = (abi_ulong)ELF_START_MMAP;
>      info->mmap = 0;
> @@ -2211,17 +2204,19 @@ int load_elf_binary(struct linux_binprm *bprm,
> struct image_info *info) when we load the interpreter.  */
>      elf_ex = *(struct elfhdr *)bprm->buf;
> 
> -    bprm->p = copy_elf_strings(1, &bprm->filename, bprm->page, bprm->p);
> -    bprm->p = copy_elf_strings(bprm->envc,bprm->envp,bprm->page,bprm->p);
> -    bprm->p = copy_elf_strings(bprm->argc,bprm->argv,bprm->page,bprm->p);
> +    /* Do this so that we can load the interpreter, if need be.  We will
> +       change some of these later */
> +    bprm->p = setup_arg_pages(bprm, info);
> +
> +    scratch = g_new0(void *, TARGET_PAGE_SIZE);
> +    bprm->p = copy_elf_strings(1, &bprm->filename, scratch, bprm->p,
> info->stack_limit); +    bprm->p = copy_elf_strings(bprm->envc, bprm->envp,
> scratch, bprm->p, info->stack_limit); +    bprm->p =
> copy_elf_strings(bprm->argc, bprm->argv, scratch, bprm->p,
> info->stack_limit); if (!bprm->p) {
>          fprintf(stderr, "%s: %s\n", bprm->filename, strerror(E2BIG));
>          exit(-1);
>      }
> -
> -    /* Do this so that we can load the interpreter, if need be.  We will
> -       change some of these later */
> -    bprm->p = setup_arg_pages(bprm->p, bprm, info);
> +    g_free(scratch);
> 
>      if (elf_interpreter) {
>          load_elf_interp(elf_interpreter, &interp_info, bprm->buf);
> diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c
> index 506e837..09df934 100644
> --- a/linux-user/linuxload.c
> +++ b/linux-user/linuxload.c
> @@ -137,8 +137,7 @@ int loader_exec(int fdexec, const char *filename, char
> **argv, char **envp, int retval;
>      int i;
> 
> -    bprm->p = TARGET_PAGE_SIZE*MAX_ARG_PAGES-sizeof(unsigned int);
> -    memset(bprm->page, 0, sizeof(bprm->page));
> +    bprm->p = 0;
>      bprm->fd = fdexec;
>      bprm->filename = (char *)filename;
>      bprm->argc = count(argv);
> @@ -172,9 +171,5 @@ int loader_exec(int fdexec, const char *filename, char
> **argv, char **envp, return retval;
>      }
> 
> -    /* Something went wrong, return the inode and free the argument pages*/
> -    for (i=0 ; i<MAX_ARG_PAGES ; i++) {
> -        g_free(bprm->page[i]);
> -    }
>      return(retval);
>  }
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 8012cc2..04c315b 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -145,12 +145,6 @@ extern const char *qemu_uname_release;
>  extern unsigned long mmap_min_addr;
> 
>  /* ??? See if we can avoid exposing so much of the loader internals.  */
> -/*
> - * MAX_ARG_PAGES defines the number of pages allocated for arguments
> - * and envelope for the new program. 32 should suffice, this gives
> - * a maximum env+arg of 128kB w/4KB pages!
> - */
> -#define MAX_ARG_PAGES 33
> 
>  /* Read a good amount of data initially, to hopefully get all the
>     program headers loaded.  */
> @@ -162,7 +156,6 @@ extern unsigned long mmap_min_addr;
>   */
>  struct linux_binprm {
>          char buf[BPRM_BUF_SIZE] __attribute__((aligned));
> -        void *page[MAX_ARG_PAGES];
>          abi_ulong p;
>  	int fd;
>          int e_uid, e_gid;
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f62c698..7d4e60e 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5799,12 +5799,6 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
> arg1, }
>              *q = NULL;
> 
> -            /* This case will not be caught by the host's execve() if its
> -               page size is bigger than the target's. */
> -            if (total_size > MAX_ARG_PAGES * TARGET_PAGE_SIZE) {
> -                ret = -TARGET_E2BIG;
> -                goto execve_end;
> -            }
>              if (!(p = lock_user_string(arg1)))
>                  goto execve_efault;
>              ret = get_errno(execve(p, argp, envp));

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019
work: +49 2405 49936-424