All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES, allow dynamic growth of env/argv space
       [not found] <1439663161-13070-1-git-send-email-stefan.bruens@rwth-aachen.de>
@ 2015-08-15 18:26 ` Stefan Brüns
  2015-08-19 21:57   ` Peter Maydell
  0 siblings, 1 reply; 20+ messages in thread
From: Stefan Brüns @ 2015-08-15 18:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: Riku Voipio, 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES, allow dynamic growth of env/argv space
  2015-08-15 18:26 ` [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES, allow dynamic growth of env/argv space Stefan Brüns
@ 2015-08-19 21:57   ` Peter Maydell
  2015-08-22 23:47     ` [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit Stefan Brüns
  2015-08-23  0:31     ` [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES, allow dynamic growth of env/argv space Stefan Bruens
  0 siblings, 2 replies; 20+ messages in thread
From: Peter Maydell @ 2015-08-19 21:57 UTC (permalink / raw)
  To: Stefan Brüns; +Cc: Riku Voipio, QEMU Developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit
  2015-08-19 21:57   ` Peter Maydell
@ 2015-08-22 23:47     ` Stefan Brüns
  2015-08-27 16:10       ` Stefan Bruens
  2015-08-27 17:57       ` Peter Maydell
  2015-08-23  0:31     ` [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES, allow dynamic growth of env/argv space Stefan Bruens
  1 sibling, 2 replies; 20+ messages in thread
From: Stefan Brüns @ 2015-08-22 23:47 UTC (permalink / raw)
  To: QEMU Developers; +Cc: 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES, allow dynamic growth of env/argv space
  2015-08-19 21:57   ` Peter Maydell
  2015-08-22 23:47     ` [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit Stefan Brüns
@ 2015-08-23  0:31     ` Stefan Bruens
  1 sibling, 0 replies; 20+ messages in thread
From: Stefan Bruens @ 2015-08-23  0:31 UTC (permalink / raw)
  To: Qemu-devel

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit
  2015-08-22 23:47     ` [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit Stefan Brüns
@ 2015-08-27 16:10       ` Stefan Bruens
  2015-08-27 17:57       ` Peter Maydell
  1 sibling, 0 replies; 20+ messages in thread
From: Stefan Bruens @ 2015-08-27 16:10 UTC (permalink / raw)
  To: qemu-devel

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit
  2015-08-22 23:47     ` [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit Stefan Brüns
  2015-08-27 16:10       ` Stefan Bruens
@ 2015-08-27 17:57       ` Peter Maydell
  2015-08-27 19:35         ` Stefan Brüns
  1 sibling, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2015-08-27 17:57 UTC (permalink / raw)
  To: Stefan Brüns; +Cc: QEMU Developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit
  2015-08-27 17:57       ` Peter Maydell
@ 2015-08-27 19:35         ` Stefan Brüns
  2015-08-30 15:52           ` Stefan Bruens
  2015-09-01 14:29           ` Peter Maydell
  0 siblings, 2 replies; 20+ messages in thread
From: Stefan Brüns @ 2015-08-27 19:35 UTC (permalink / raw)
  To: QEMU Developers; +Cc: Peter Maydell, 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit
  2015-08-27 19:35         ` Stefan Brüns
@ 2015-08-30 15:52           ` Stefan Bruens
  2015-08-30 16:37             ` Peter Maydell
  2015-09-01 14:29           ` Peter Maydell
  1 sibling, 1 reply; 20+ messages in thread
From: Stefan Bruens @ 2015-08-30 15:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit
  2015-08-30 15:52           ` Stefan Bruens
@ 2015-08-30 16:37             ` Peter Maydell
  0 siblings, 0 replies; 20+ messages in thread
From: Peter Maydell @ 2015-08-30 16:37 UTC (permalink / raw)
  To: Stefan Bruens; +Cc: QEMU Developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit
  2015-08-27 19:35         ` Stefan Brüns
  2015-08-30 15:52           ` Stefan Bruens
@ 2015-09-01 14:29           ` Peter Maydell
  2015-09-01 17:27             ` Brüns, Stefan
  1 sibling, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2015-09-01 14:29 UTC (permalink / raw)
  To: Stefan Brüns; +Cc: QEMU Developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit
  2015-09-01 14:29           ` Peter Maydell
@ 2015-09-01 17:27             ` Brüns, Stefan
  2015-09-01 18:42               ` Peter Maydell
  0 siblings, 1 reply; 20+ messages in thread
From: Brüns, Stefan @ 2015-09-01 17:27 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit
  2015-09-01 17:27             ` Brüns, Stefan
@ 2015-09-01 18:42               ` Peter Maydell
  2015-09-02  1:15                 ` Stefan Bruens
                                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Peter Maydell @ 2015-09-01 18:42 UTC (permalink / raw)
  To: Brüns, Stefan; +Cc: QEMU Developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit
  2015-09-01 18:42               ` Peter Maydell
@ 2015-09-02  1:15                 ` Stefan Bruens
  2015-09-02  1:38                 ` [Qemu-devel] [PATCH 1/2] linux-user: remove unused image_info members Stefan Brüns
       [not found]                 ` <1441157933-12459-1-git-send-email-stefan.bruens@rwth-aachen.de>
  2 siblings, 0 replies; 20+ messages in thread
From: Stefan Bruens @ 2015-09-02  1:15 UTC (permalink / raw)
  To: Peter Maydell, qemu-devel

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH 1/2] linux-user: remove unused image_info members
  2015-09-01 18:42               ` Peter Maydell
  2015-09-02  1:15                 ` Stefan Bruens
@ 2015-09-02  1:38                 ` Stefan Brüns
  2015-09-03 17:19                   ` Peter Maydell
       [not found]                 ` <1441157933-12459-1-git-send-email-stefan.bruens@rwth-aachen.de>
  2 siblings, 1 reply; 20+ messages in thread
From: Stefan Brüns @ 2015-09-02  1:38 UTC (permalink / raw)
  To: QEMU Developers; +Cc: Peter Maydell, 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH 2/2] linux-user: remove MAX_ARG_PAGES limit
       [not found]                 ` <1441157933-12459-1-git-send-email-stefan.bruens@rwth-aachen.de>
@ 2015-09-02  1:38                   ` Stefan Brüns
  2015-09-03 17:27                     ` Peter Maydell
  0 siblings, 1 reply; 20+ messages in thread
From: Stefan Brüns @ 2015-09-02  1:38 UTC (permalink / raw)
  To: QEMU Developers; +Cc: Peter Maydell, 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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] linux-user: remove unused image_info members
  2015-09-02  1:38                 ` [Qemu-devel] [PATCH 1/2] linux-user: remove unused image_info members Stefan Brüns
@ 2015-09-03 17:19                   ` Peter Maydell
  0 siblings, 0 replies; 20+ messages in thread
From: Peter Maydell @ 2015-09-03 17:19 UTC (permalink / raw)
  To: Stefan Brüns; +Cc: QEMU Developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] linux-user: remove MAX_ARG_PAGES limit
  2015-09-02  1:38                   ` [Qemu-devel] [PATCH 2/2] linux-user: remove MAX_ARG_PAGES limit Stefan Brüns
@ 2015-09-03 17:27                     ` Peter Maydell
  2015-09-14 19:37                       ` Stefan Bruens
  0 siblings, 1 reply; 20+ messages in thread
From: Peter Maydell @ 2015-09-03 17:27 UTC (permalink / raw)
  To: Stefan Brüns; +Cc: QEMU Developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] linux-user: remove MAX_ARG_PAGES limit
  2015-09-03 17:27                     ` Peter Maydell
@ 2015-09-14 19:37                       ` Stefan Bruens
  2015-09-14 20:33                         ` Peter Maydell
  2015-09-28 13:30                         ` Riku Voipio
  0 siblings, 2 replies; 20+ messages in thread
From: Stefan Bruens @ 2015-09-14 19:37 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] linux-user: remove MAX_ARG_PAGES limit
  2015-09-14 19:37                       ` Stefan Bruens
@ 2015-09-14 20:33                         ` Peter Maydell
  2015-09-28 13:30                         ` Riku Voipio
  1 sibling, 0 replies; 20+ messages in thread
From: Peter Maydell @ 2015-09-14 20:33 UTC (permalink / raw)
  To: Stefan Bruens; +Cc: Riku Voipio, QEMU Developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] linux-user: remove MAX_ARG_PAGES limit
  2015-09-14 19:37                       ` Stefan Bruens
  2015-09-14 20:33                         ` Peter Maydell
@ 2015-09-28 13:30                         ` Riku Voipio
  1 sibling, 0 replies; 20+ messages in thread
From: Riku Voipio @ 2015-09-28 13:30 UTC (permalink / raw)
  To: Stefan Bruens; +Cc: Peter Maydell, QEMU Developers

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2015-09-28 13:30 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1439663161-13070-1-git-send-email-stefan.bruens@rwth-aachen.de>
2015-08-15 18:26 ` [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES, allow dynamic growth of env/argv space Stefan Brüns
2015-08-19 21:57   ` Peter Maydell
2015-08-22 23:47     ` [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES limit Stefan Brüns
2015-08-27 16:10       ` Stefan Bruens
2015-08-27 17:57       ` Peter Maydell
2015-08-27 19:35         ` Stefan Brüns
2015-08-30 15:52           ` Stefan Bruens
2015-08-30 16:37             ` Peter Maydell
2015-09-01 14:29           ` Peter Maydell
2015-09-01 17:27             ` Brüns, Stefan
2015-09-01 18:42               ` Peter Maydell
2015-09-02  1:15                 ` Stefan Bruens
2015-09-02  1:38                 ` [Qemu-devel] [PATCH 1/2] linux-user: remove unused image_info members Stefan Brüns
2015-09-03 17:19                   ` Peter Maydell
     [not found]                 ` <1441157933-12459-1-git-send-email-stefan.bruens@rwth-aachen.de>
2015-09-02  1:38                   ` [Qemu-devel] [PATCH 2/2] linux-user: remove MAX_ARG_PAGES limit Stefan Brüns
2015-09-03 17:27                     ` Peter Maydell
2015-09-14 19:37                       ` Stefan Bruens
2015-09-14 20:33                         ` Peter Maydell
2015-09-28 13:30                         ` Riku Voipio
2015-08-23  0:31     ` [Qemu-devel] [PATCH] linux-user: remove MAX_ARG_PAGES, allow dynamic growth of env/argv space Stefan Bruens

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.