Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES, allow dynamic growth of env/argv space

[Peter Maydell]

On 15 August 2015 at 19:26, Stefan Brüns <address@hidden> wrote:
> qemu currently limits the space for the evironment and arguments to
> 32 * PAGE_SIZE. Linux limits the argument space to 1/4 of the stack size.
> A program trying to detect this with a getrlimit(RLIMIT_STACK) syscall
> will typically get a much larger limit than qemus current 128kB.
>
> The current limit causes "Argument list too long" errors.
>
> Signed-off-by: Stefan Brüns <address@hidden>

Thanks for this bug fix; it definitely seems like a good idea.
I have a few review comments below.

> ---
>  linux-user/elfload.c   | 29 ++++++++++++++++++-----------
>  linux-user/linuxload.c |  7 ++++---
>  linux-user/qemu.h      | 11 ++---------
>  linux-user/syscall.c   |  4 ++++
>  4 files changed, 28 insertions(+), 23 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 1788368..be8f4d6 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1365,11 +1365,13 @@ 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,

This should have a space after the 'argc,'.
(If you run scripts/checkpatch.pl you'll find it catches this
and other minor style errors.)

> +                                  struct linux_binprm *bprm)
>  {
>      char *tmp, *tmp1, *pag = NULL;
>      int len, offset = 0;
> +    void **page = bprm->page;
> +    abi_ulong p = bprm->p;
>
>      if (!p) {
>          return 0;       /* bullet-proofing */
> @@ -1383,8 +1385,13 @@ static abi_ulong copy_elf_strings(int argc,char ** 
> argv, void **page,
>          tmp1 = tmp;
>          while (*tmp++);
>          len = tmp - tmp1;
> -        if (p < len) {  /* this shouldn't happen - 128kB */
> -            return 0;
> +        if (p < len) {

Since this looks almost but not quite like a standard reallocate-larger,
a comment here would be helpful I think:
     /* Reallocate the page array to add extra zero entries at the start */

> +            bprm->page = (void**)calloc(bprm->n_arg_pages + 32, 
> sizeof(void*));

Prefer
    bprm->page = g_new0(void *, bprm->n_arg_pages + 32);

> +            memcpy(&bprm->page[32], page, sizeof(void*) * bprm->n_arg_pages);
> +            free(page);

   g_free(page);

> +            page = bprm->page;
> +            bprm->n_arg_pages += 32;
> +            p += 32 * TARGET_PAGE_SIZE;

I think we have enough repetitions of '32' here to merit a #define.

But having said all that, I wonder if it would be better to
precalculate how big a page array we need and just do the
allocation once, rather than having this complicated code to
handle a reallocate-and-fix-up-everything. In particular this
is basically just adding string lengths for filename, argv
and envp together. load_flt_binary() already wants that information,
so it might be better to have loader_exec() calculate this
and fill in new bprm->argv_strlen and bprm->envp_strlen values
for the callees to use.

>          }
>          while (len) {
>              --p; --tmp; --len;
> @@ -1423,8 +1430,8 @@ static abi_ulong setup_arg_pages(abi_ulong p, struct 
> linux_binprm *bprm,
>      /* 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 < bprm->n_arg_pages * TARGET_PAGE_SIZE) {
> +        size = bprm->n_arg_pages * TARGET_PAGE_SIZE;
>      }
>      guard = TARGET_PAGE_SIZE;
>      if (guard < qemu_real_host_page_size) {
> @@ -1442,10 +1449,10 @@ 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;
> +    stack_base = info->stack_limit + size - bprm->n_arg_pages * 
> TARGET_PAGE_SIZE;
>      p += stack_base;
>
> -    for (i = 0 ; i < MAX_ARG_PAGES ; i++) {
> +    for (i = 0; i < bprm->n_arg_pages; i++) {
>          if (bprm->page[i]) {
>              info->rss++;
>              /* FIXME - check return value of memcpy_to_target() for failure 
> */
> @@ -2211,9 +2218,9 @@ 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);
> +    bprm->p = copy_elf_strings(1, &bprm->filename, bprm);
> +    bprm->p = copy_elf_strings(bprm->envc, bprm->envp, bprm);
> +    bprm->p = copy_elf_strings(bprm->argc, bprm->argv, bprm);
>      if (!bprm->p) {
>          fprintf(stderr, "%s: %s\n", bprm->filename, strerror(E2BIG));
>          exit(-1);
> diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c
> index 506e837..59943ea 100644
> --- a/linux-user/linuxload.c
> +++ b/linux-user/linuxload.c
> @@ -137,8 +137,9 @@ 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->n_arg_pages = 32;
> +    bprm->p = bprm->n_arg_pages * TARGET_PAGE_SIZE - sizeof(unsigned int);
> +    bprm->page = (void**)calloc(bprm->n_arg_pages, sizeof(void*));

Rather than calloc, prefer
    bprm->page = g_new0(void *, bprm->n_arg_pages);

>      bprm->fd = fdexec;
>      bprm->filename = (char *)filename;
>      bprm->argc = count(argv);
> @@ -173,7 +174,7 @@ int loader_exec(int fdexec, const char *filename, char 
> **argv, char **envp,
>      }
>
>      /* Something went wrong, return the inode and free the argument pages*/
> -    for (i=0 ; i<MAX_ARG_PAGES ; i++) {
> +    for (i = 0; i < bprm->n_arg_pages; i++) {
>          g_free(bprm->page[i]);
>      }

You should g_free() bprm->page too here now, since we now allocate it.

>      return(retval);
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 8012cc2..cf4aa73 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -144,14 +144,6 @@ void stop_all_tasks(void);
>  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.  */
>  #define BPRM_BUF_SIZE  1024
> @@ -162,7 +154,8 @@ extern unsigned long mmap_min_addr;
>   */
>  struct linux_binprm {
>          char buf[BPRM_BUF_SIZE] __attribute__((aligned));
> -        void *page[MAX_ARG_PAGES];
> +        void **page;
> +        int n_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..6fa5fb4 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5799,12 +5799,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>              }
>              *q = NULL;
>
> +/* This check is bogus. MAX_ARG_PAGES is a thing of the past. Trust the host
> +execve to set errno appropriately */
> +#if 0
>              /* 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;
>              }
> +#endif

You should just delete this code now it's not required; we don't
need to leave #if-0'd out obsolete code in the tree.

>              if (!(p = lock_user_string(arg1)))
>                  goto execve_efault;
>              ret = get_errno(execve(p, argp, envp));
> --
> 2.1.4

thanks
-- PMM