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

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

[Stefan Brüns]

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 <stefan.bruens@rwth-aachen.de>
---
 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,
+                                  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) {
+            bprm->page = (void**)calloc(bprm->n_arg_pages + 32, sizeof(void*));
+            memcpy(&bprm->page[32], page, sizeof(void*) * bprm->n_arg_pages);
+            free(page);
+            page = bprm->page;
+            bprm->n_arg_pages += 32;
+            p += 32 * TARGET_PAGE_SIZE;
         }
         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*));
     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]);
     }
     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
             if (!(p = lock_user_string(arg1)))
                 goto execve_efault;
             ret = get_errno(execve(p, argp, envp));
-- 
2.1.4

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 <stefan.bruens@rwth-aachen.de> 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 <stefan.bruens@rwth-aachen.de>

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

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

[Stefan Brüns]

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(-)

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));
-- 
2.1.4

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

[Stefan Bruens]

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

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

[Peter Maydell]

On 23 August 2015 at 00:47, Stefan Brüns <stefan.bruens@rwth-aachen.de> 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>

This doesn't seem to compile:

  CC    arm-linux-user/linux-user/elfload.o
/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/elfload.c: In
function ‘load_elf_binary’:
/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/elfload.c:2210:5:
error: passing argument 3 of ‘copy_elf_strings’ from incompatible
pointer type [-Werror]
     bprm->p = copy_elf_strings(1, &bprm->filename, scratch, bprm->p,
info->stack_limit);
     ^
/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/elfload.c:1368:18:
note: expected ‘char *’ but argument is of type ‘void **’
 static abi_ulong copy_elf_strings(int argc,char ** argv, char* scratch,
                  ^
/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/elfload.c:2211:5:
error: passing argument 3 of ‘copy_elf_strings’ from incompatible
pointer type [-Werror]
     bprm->p = copy_elf_strings(bprm->envc, bprm->envp, scratch,
bprm->p, info->stack_limit);
     ^
/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/elfload.c:1368:18:
note: expected ‘char *’ but argument is of type ‘void **’
 static abi_ulong copy_elf_strings(int argc,char ** argv, char* scratch,
                  ^
/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/elfload.c:2212:5:
error: passing argument 3 of ‘copy_elf_strings’ from incompatible
pointer type [-Werror]
     bprm->p = copy_elf_strings(bprm->argc, bprm->argv, scratch,
bprm->p, info->stack_limit);
     ^
/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/elfload.c:1368:18:
note: expected ‘char *’ but argument is of type ‘void **’
 static abi_ulong copy_elf_strings(int argc,char ** argv, char* scratch,
                  ^
cc1: all warnings being treated as errors

It also has a number of checkpatch.pl warnings.

thanks
-- PMM

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

[Stefan Brüns]

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.

v2: fixed scratch pointer type, fixed checkpatch issues.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 linux-user/elfload.c   | 110 ++++++++++++++++++++++++-------------------------
 linux-user/linuxload.c |   7 +---
 linux-user/qemu.h      |   7 ----
 linux-user/syscall.c   |   6 ---
 4 files changed, 56 insertions(+), 74 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 9c999ac..991dd35 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1365,66 +1365,70 @@ 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 +1446,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
@@ -2196,6 +2189,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;
+    char *scratch;
 
     info->start_mmap = (abi_ulong)ELF_START_MMAP;
     info->mmap = 0;
@@ -2209,18 +2203,24 @@ 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(char, 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);
+    g_free(scratch);
+
     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);
-
     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));
-- 
2.1.4

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

[Stefan Bruens]

On Thursday 27 August 2015 21:35:41 Stefan Brüns wrote:

PING!

> 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.
> 
> v2: fixed scratch pointer type, fixed checkpatch issues.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

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

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

[Peter Maydell]

On 30 August 2015 at 16:52, Stefan Bruens <stefan.bruens@rwth-aachen.de> wrote:
> On Thursday 27 August 2015 21:35:41 Stefan Brüns wrote:
>
> PING!

Hi. This is on my to-review list, but so are a lot of
other things. It might take me a week or so to get to it.

thanks
-- PMM

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

[Peter Maydell]

On 27 August 2015 at 20:35, Stefan Brüns <stefan.bruens@rwth-aachen.de> 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.
>
> v2: fixed scratch pointer type, fixed checkpatch issues.

This sort of 'v1 to v2 diffs' comment should go below the '---'
line, so it doesn't end up in the final git commit message.

>
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>  linux-user/elfload.c   | 110 ++++++++++++++++++++++++-------------------------
>  linux-user/linuxload.c |   7 +---
>  linux-user/qemu.h      |   7 ----
>  linux-user/syscall.c   |   6 ---
>  4 files changed, 56 insertions(+), 74 deletions(-)

Still doesn't compile:

/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c: In
function ‘loader_exec’:
/home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c:138:9:
error: unused variable ‘i’ [-Werror=unused-variable]
     int i;
         ^

Mostly this looks good; I have a few review comments below.

> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 9c999ac..991dd35 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c

> +            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);

Why bother to special case the 1 byte case?

>              }
> -            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

This sounds like a minimum, not a maximum, so the name is very
misleading.

The comment says "limit to 1/4 of stack size" but the code doesn't
seem to do anything like that.

Please move the #define to top-level in the file, not hidden
inside a function definition.

>
> -    /* 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 +1446,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++;

I think your patch has lost the calculation of info->rss.

(We don't actually use info->rss anywhere, though, so if you wanted
to add a patch in front of this one explicitly removing it as useless
that would be an OK way to handle this.)

> -            /* 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

> 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;

Nothing actually uses this value -- both the elfload and the flatload code
paths now either ignore bprm->p or set it themselves. It would be
better to delete this and also the dead assignment "p = bprm->p" at
the top of load_flt_binary().

>      bprm->fd = fdexec;
>      bprm->filename = (char *)filename;
>      bprm->argc = count(argv);

thanks
-- PMM

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

[Brüns, Stefan]

On Tuesday, September 01, 2015 15:29:26 you wrote:
> On 27 August 2015 at 20:35, Stefan Brüns <stefan.bruens@rwth-aachen.de> 
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.
> > 
> > v2: fixed scratch pointer type, fixed checkpatch issues.
> 
> This sort of 'v1 to v2 diffs' comment should go below the '---'
> line, so it doesn't end up in the final git commit message.
> 
> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > ---
> > 
> >  linux-user/elfload.c   | 110
> >  ++++++++++++++++++++++++------------------------- linux-user/linuxload.c
> >  |   7 +---
> >  linux-user/qemu.h      |   7 ----
> >  linux-user/syscall.c   |   6 ---
> >  4 files changed, 56 insertions(+), 74 deletions(-)
> 
> Still doesn't compile:
> 
> /home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c: In
> function ‘loader_exec’:
> /home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c:138:9:
> error: unused variable ‘i’ [-Werror=unused-variable]
>      int i;
>          ^
> 
> Mostly this looks good; I have a few review comments below.
> 
> > diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> > index 9c999ac..991dd35 100644
> > --- a/linux-user/elfload.c
> > +++ b/linux-user/elfload.c
> > 
> > +            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);
> 
> Why bother to special case the 1 byte case?

Taken from the old code. Probably not worth the extra code, will remove it.
 
> >              }
> > 
> > -            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
> 
> This sounds like a minimum, not a maximum, so the name is very
> misleading.
> 
> The comment says "limit to 1/4 of stack size" but the code doesn't
> seem to do anything like that.
> 
> Please move the #define to top-level in the file, not hidden
> inside a function definition.

MAX_ARG_PAGES is the name used in old kernels (and current, when there is
no MMU), so it is useful to keep this name (maybe in a comment).

What about:
---
/* Older linux kernels provide up to MAX_ARG_PAGES (default: 32) of
 * argument/environment space. Newer kernels (>2.6.33) provide up to
 * 1/4th of stack size, but guarantee at least 32 pages for backwards
 * compatibility. */
#define STACK_LOWER_LIMIT (32 * TARGET_PAGE_SIZE)
---

The 1/4th is not enforced, as our stack has to be big enough to hold args/env 
coming from the kernel, but is no problem if we provide more space.

Thinking about it, maybe we should provide some extra space, as qemu-linux-
user puts some more stuff (e.g. auxv) between env/args and user stack.
 
> > -    /* 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 +1446,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++;
> 
> I think your patch has lost the calculation of info->rss.
> 
> (We don't actually use info->rss anywhere, though, so if you wanted
> to add a patch in front of this one explicitly removing it as useless
> that would be an OK way to handle this.)

Will add an explicit removal patch. info->mmap is not used either, ok to 
remove both in one go?

> > -            /* 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
> > 
> > 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;
> 
> Nothing actually uses this value -- both the elfload and the flatload code
> paths now either ignore bprm->p or set it themselves. It would be
> better to delete this and also the dead assignment "p = bprm->p" at
> the top of load_flt_binary().

OK to do this in a followup patch?

> >      bprm->fd = fdexec;
> >      bprm->filename = (char *)filename;
> >      bprm->argc = count(argv);
> 
> thanks
> -- PMM

Kind regards, Stefan

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

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

[Peter Maydell]

On 1 September 2015 at 18:27, Brüns, Stefan
<Stefan.Bruens@rwth-aachen.de> wrote:
> On Tuesday, September 01, 2015 15:29:26 you wrote:
>> On 27 August 2015 at 20:35, Stefan Brüns <stefan.bruens@rwth-aachen.de>
> 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.
>> >
>> > v2: fixed scratch pointer type, fixed checkpatch issues.
>>
>> This sort of 'v1 to v2 diffs' comment should go below the '---'
>> line, so it doesn't end up in the final git commit message.
>>
>> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
>> > ---
>> >
>> >  linux-user/elfload.c   | 110
>> >  ++++++++++++++++++++++++------------------------- linux-user/linuxload.c
>> >  |   7 +---
>> >  linux-user/qemu.h      |   7 ----
>> >  linux-user/syscall.c   |   6 ---
>> >  4 files changed, 56 insertions(+), 74 deletions(-)
>>
>> Still doesn't compile:
>>
>> /home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c: In
>> function ‘loader_exec’:
>> /home/petmay01/linaro/qemu-from-laptop/qemu/linux-user/linuxload.c:138:9:
>> error: unused variable ‘i’ [-Werror=unused-variable]
>>      int i;
>>          ^
>>
>> Mostly this looks good; I have a few review comments below.
>>
>> > diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> > index 9c999ac..991dd35 100644
>> > --- a/linux-user/elfload.c
>> > +++ b/linux-user/elfload.c
>> >
>> > +            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);
>>
>> Why bother to special case the 1 byte case?
>
> Taken from the old code. Probably not worth the extra code, will remove it.

Oops, I thought I'd checked the diff to check it wasn't already
present, but I must have misread it.

>> > +    /* 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
>>
>> This sounds like a minimum, not a maximum, so the name is very
>> misleading.
>>
>> The comment says "limit to 1/4 of stack size" but the code doesn't
>> seem to do anything like that.
>>
>> Please move the #define to top-level in the file, not hidden
>> inside a function definition.
>
> MAX_ARG_PAGES is the name used in old kernels (and current, when there is
> no MMU), so it is useful to keep this name (maybe in a comment).
>
> What about:
> ---
> /* Older linux kernels provide up to MAX_ARG_PAGES (default: 32) of
>  * argument/environment space. Newer kernels (>2.6.33) provide up to
>  * 1/4th of stack size, but guarantee at least 32 pages for backwards
>  * compatibility. */
> #define STACK_LOWER_LIMIT (32 * TARGET_PAGE_SIZE)
> ---

OK. (Incidentally I prefer the trailing "*/" of a multiline comment
on its own line.)

> The 1/4th is not enforced, as our stack has to be big enough to hold args/env
> coming from the kernel, but is no problem if we provide more space.

That's OK, but then we should either (a) just not mention the 1/4
limit in the comment or (b) mention it and say we don't enforce
the limit in QEMU.

>> I think your patch has lost the calculation of info->rss.
>>
>> (We don't actually use info->rss anywhere, though, so if you wanted
>> to add a patch in front of this one explicitly removing it as useless
>> that would be an OK way to handle this.)
>
> Will add an explicit removal patch. info->mmap is not used either, ok to
> remove both in one go?

Yes.

>> > --- 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;
>>
>> Nothing actually uses this value -- both the elfload and the flatload code
>> paths now either ignore bprm->p or set it themselves. It would be
>> better to delete this and also the dead assignment "p = bprm->p" at
>> the top of load_flt_binary().
>
> OK to do this in a followup patch?

If you want to remove the dead assignment in its own patch
I would do that before this patch, rather than after.

thanks
-- PMM

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

[Stefan Bruens]

On Tuesday 01 September 2015 19:42:26 you wrote:
> >> > --- 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;
> >> 
> >> Nothing actually uses this value -- both the elfload and the flatload
> >> code
> >> paths now either ignore bprm->p or set it themselves. It would be
> >> better to delete this and also the dead assignment "p = bprm->p" at
> >> the top of load_flt_binary().
> > 
> > OK to do this in a followup patch?
> 
> If you want to remove the dead assignment in its own patch
> I would do that before this patch, rather than after.

Before is not really possible, as it depends on the reordering.

Regards, Stefan

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

[Qemu-devel] [PATCH 1/2] linux-user: remove unused image_info members

[Stefan Brüns]

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 linux-user/elfload.c | 3 ---
 linux-user/qemu.h    | 2 --
 2 files changed, 5 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 9c999ac..e44f989 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1447,7 +1447,6 @@ static abi_ulong setup_arg_pages(abi_ulong p, struct linux_binprm *bprm,
 
     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]);
@@ -2198,8 +2197,6 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
     char *elf_interpreter = NULL;
 
     info->start_mmap = (abi_ulong)ELF_START_MMAP;
-    info->mmap = 0;
-    info->rss = 0;
 
     load_elf_image(bprm->filename, bprm->fd, info,
                    &elf_interpreter, bprm->buf);
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 8012cc2..7d0009e 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -36,8 +36,6 @@ struct image_info {
         abi_ulong       start_brk;
         abi_ulong       brk;
         abi_ulong       start_mmap;
-        abi_ulong       mmap;
-        abi_ulong       rss;
         abi_ulong       start_stack;
         abi_ulong       stack_limit;
         abi_ulong       entry;
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/2] linux-user: remove unused image_info members

[Peter Maydell]

On 2 September 2015 at 02:38, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

By the way, for future submissions of patch series with more than
one patch in them, please can you include a cover letter? (ie
what git produces for git format-patch --cover-letter). Some of
our tooling for processing patches doesn't really handle multipatch
series which don't have a cover letter.

thanks
-- PMM

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

[Stefan Brüns]

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>
---
v2: fixed scratch pointer type, fixed checkpatch issues.
v3: removed special case for single byte memcpy
    replace MAX_ARG_PAGES with STACK_LOWER_LIMIT

 linux-user/elfload.c   | 110 ++++++++++++++++++++++++-------------------------
 linux-user/flatload.c  |   2 +-
 linux-user/linuxload.c |   7 ----
 linux-user/qemu.h      |   7 ----
 linux-user/syscall.c   |   6 ---
 5 files changed, 56 insertions(+), 76 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index e44f989..7bd5759 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;
-            }
-            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);
+            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(scratch + offset, tmp, bytes_to_copy);
+
+            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,
+/* Older linux kernels provide up to MAX_ARG_PAGES (default: 32) of
+ * argument/environment space. Newer kernels (>2.6.33) allow more,
+ * dependent on stack size, but guarantee at least 32 pages for
+ * backwards compatibility.
+ */
+#define STACK_LOWER_LIMIT (32 * TARGET_PAGE_SIZE)
+
+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;
 
-    /* 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 < STACK_LOWER_LIMIT) {
+        size = STACK_LOWER_LIMIT;
     }
     guard = TARGET_PAGE_SIZE;
     if (guard < qemu_real_host_page_size) {
@@ -1442,18 +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]) {
-            /* 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
@@ -2195,6 +2188,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;
+    char *scratch;
 
     info->start_mmap = (abi_ulong)ELF_START_MMAP;
 
@@ -2206,18 +2200,24 @@ 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(char, 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);
+    g_free(scratch);
+
     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);
-
     if (elf_interpreter) {
         load_elf_interp(elf_interpreter, &interp_info, bprm->buf);
 
diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index 566a7a8..ceacb98 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -707,7 +707,7 @@ static int load_flat_shared_library(int id, struct lib_info *libs)
 int load_flt_binary(struct linux_binprm *bprm, struct image_info *info)
 {
     struct lib_info libinfo[MAX_SHARED_LIBS];
-    abi_ulong p = bprm->p;
+    abi_ulong p;
     abi_ulong stack_len;
     abi_ulong start_addr;
     abi_ulong sp;
diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c
index 506e837..dbaf0ec 100644
--- a/linux-user/linuxload.c
+++ b/linux-user/linuxload.c
@@ -135,10 +135,7 @@ int loader_exec(int fdexec, const char *filename, char **argv, char **envp,
              struct linux_binprm *bprm)
 {
     int retval;
-    int i;
 
-    bprm->p = TARGET_PAGE_SIZE*MAX_ARG_PAGES-sizeof(unsigned int);
-    memset(bprm->page, 0, sizeof(bprm->page));
     bprm->fd = fdexec;
     bprm->filename = (char *)filename;
     bprm->argc = count(argv);
@@ -172,9 +169,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 7d0009e..0341df8 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -143,12 +143,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.  */
@@ -160,7 +154,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));
-- 
2.1.4

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

[Peter Maydell]

On 2 September 2015 at 02:38, Stefan Brüns <stefan.bruens@rwth-aachen.de> 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>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks for working through the code review process.

thanks
-- PMM

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

[Stefan Bruens]

On Thursday 03 September 2015 18:27:20 Peter Maydell wrote:
> On 2 September 2015 at 02:38, Stefan Brüns <stefan.bruens@rwth-aachen.de> 
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>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Thanks for working through the code review process.
> 
> thanks
> -- PMM

Hi Peter,

is there anything that needs to be done before picking up these two patches?

Kind regards,

Stefan

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

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

[Peter Maydell]

On 14 September 2015 at 20:37, Stefan Bruens
<stefan.bruens@rwth-aachen.de> wrote:
> On Thursday 03 September 2015 18:27:20 Peter Maydell wrote:
>> On 2 September 2015 at 02:38, Stefan Brüns <stefan.bruens@rwth-aachen.de>
> 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>
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> Thanks for working through the code review process.
>>
>> thanks
>> -- PMM
>
> Hi Peter,
>
> is there anything that needs to be done before picking up these two patches?

Nope. It's just waiting on Riku to pick them up (I've cc'd him;
it's usually a good idea to cc the relevant submaintainer
listed in the MAINTAINERS file). Riku doesn't generally have
much time to do QEMU related work these days though so there's
sometimes a lag on linux-user patches getting into master.

thanks
-- PMM

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

[Riku Voipio]

On Mon, Sep 14, 2015 at 09:37:10PM +0200, Stefan Bruens wrote:
> On Thursday 03 September 2015 18:27:20 Peter Maydell wrote:
> > On 2 September 2015 at 02:38, Stefan Brüns <stefan.bruens@rwth-aachen.de> 
> 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>
> > 
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > 
> > Thanks for working through the code review process.
> > 
> > thanks
> > -- PMM
> 
> Hi Peter,
> 
> is there anything that needs to be done before picking up these two patches?

Hi,

Both patches applied to linux-user tree, thanks

Riku

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

[Stefan Bruens]

On Wednesday 19 August 2015 22:57:53 you wrote:
[...]
> 
> 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.

I have completely reworked the patch. There is no longer any need for the page 
array, the environment gets directly copied to the target stack (although it 
uses a scratch buffer, to avoid frequent calls to the locking 
memcpy_to_target).

Kind regards,

Stefan

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