[PATCH 0/5] linux-user changes to run docker

[PATCH 0/5] linux-user changes to run docker

[YAMAMOTO Takashi]

These patches, along with a few more hacks [1] I didn't include
in this patchset, allowed me to run arm64 and armv7 version of
dind image on amd64.

[1] https://github.com/yamt/qemu/tree/linux-user-for-docker

You can find my test setup here:
https://github.com/yamt/garbage/tree/master/binfmt-aarch64-install

YAMAMOTO Takashi (5):
  linux-user: handle /proc/self/exe for execve
  linux-uesr: make exec_path realpath
  linux-user: Fix the execfd case of /proc/self/exe open
  linux-user: dup the execfd on start up
  linux-user: Implement pivot_root

 linux-user/main.c    | 14 +++++++++++++-
 linux-user/qemu.h    |  2 ++
 linux-user/syscall.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 55 insertions(+), 4 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

[PATCH 1/5] linux-user: handle /proc/self/exe for execve

[YAMAMOTO Takashi]

It seems somehow common to execve /proc/self/exe in docker
or golang community these days.
At least, moby "reexec" and runc "libcontainer" do that.

Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
---
 linux-user/syscall.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index c9f812091c..a2b03ecb8b 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8470,6 +8470,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
 #endif
     case TARGET_NR_execve:
         {
+            const char *path;
             char **argp, **envp;
             int argc, envc;
             abi_ulong gp;
@@ -8537,7 +8538,11 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
              * before the execve completes and makes it the other
              * program's problem.
              */
-            ret = get_errno(safe_execve(p, argp, envp));
+            path = p;
+            if (is_proc_myself(path, "exe")) {
+                path = exec_path;
+            }
+            ret = get_errno(safe_execve(path, argp, envp));
             unlock_user(p, arg1, 0);
 
             goto execve_end;
-- 
2.21.1 (Apple Git-122.3)

Re: [PATCH 1/5] linux-user: handle /proc/self/exe for execve

[Alex Bennée]

YAMAMOTO Takashi <yamamoto@midokura.com> writes:

> It seems somehow common to execve /proc/self/exe in docker
> or golang community these days.
> At least, moby "reexec" and runc "libcontainer" do that.
>
> Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
> ---
>  linux-user/syscall.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index c9f812091c..a2b03ecb8b 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8470,6 +8470,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>  #endif
>      case TARGET_NR_execve:
>          {
> +            const char *path;
>              char **argp, **envp;
>              int argc, envc;
>              abi_ulong gp;
> @@ -8537,7 +8538,11 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>               * before the execve completes and makes it the other
>               * program's problem.
>               */
> -            ret = get_errno(safe_execve(p, argp, envp));
> +            path = p;
> +            if (is_proc_myself(path, "exe")) {
> +                path = exec_path;
> +            }

This still relies on binfmt_misc kicking in to ensure the binary is
re-executed with qemu right?

> +            ret = get_errno(safe_execve(path, argp, envp));
>              unlock_user(p, arg1, 0);
>  
>              goto execve_end;


-- 
Alex Bennée

Re: [PATCH 1/5] linux-user: handle /proc/self/exe for execve

[Takashi Yamamoto]

On Mon, May 24, 2021 at 7:53 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> YAMAMOTO Takashi <yamamoto@midokura.com> writes:
>
> > It seems somehow common to execve /proc/self/exe in docker
> > or golang community these days.
> > At least, moby "reexec" and runc "libcontainer" do that.
> >
> > Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
> > ---
> >  linux-user/syscall.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index c9f812091c..a2b03ecb8b 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -8470,6 +8470,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
> >  #endif
> >      case TARGET_NR_execve:
> >          {
> > +            const char *path;
> >              char **argp, **envp;
> >              int argc, envc;
> >              abi_ulong gp;
> > @@ -8537,7 +8538,11 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
> >               * before the execve completes and makes it the other
> >               * program's problem.
> >               */
> > -            ret = get_errno(safe_execve(p, argp, envp));
> > +            path = p;
> > +            if (is_proc_myself(path, "exe")) {
> > +                path = exec_path;
> > +            }
>
> This still relies on binfmt_misc kicking in to ensure the binary is
> re-executed with qemu right?

right.

>
> > +            ret = get_errno(safe_execve(path, argp, envp));
> >              unlock_user(p, arg1, 0);
> >
> >              goto execve_end;
>
>
> --
> Alex Bennée

[PATCH 2/5] linux-uesr: make exec_path realpath

[YAMAMOTO Takashi]

Otherwise, it can be easily fooled by the user app using chdir().

Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
---
 linux-user/main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 4dfc47ad3b..1f9f4e3820 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -55,6 +55,7 @@
 #endif
 
 char *exec_path;
+char exec_path_store[PATH_MAX];
 
 int singlestep;
 static const char *argv0;
@@ -610,7 +611,10 @@ static int parse_args(int argc, char **argv)
         exit(EXIT_FAILURE);
     }
 
-    exec_path = argv[optind];
+    exec_path = realpath(argv[optind], exec_path_store);
+    if (exec_path == NULL) {
+        exec_path = argv[optind];
+    }
 
     return optind;
 }
-- 
2.21.1 (Apple Git-122.3)

Re: [PATCH 2/5] linux-uesr: make exec_path realpath

[Alex Bennée]

YAMAMOTO Takashi <yamamoto@midokura.com> writes:

> Otherwise, it can be easily fooled by the user app using chdir().
>
> Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
> ---
>  linux-user/main.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 4dfc47ad3b..1f9f4e3820 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -55,6 +55,7 @@
>  #endif
>  
>  char *exec_path;
> +char exec_path_store[PATH_MAX];

Is there any point in keeping this as a static path rather than just
allocating it off the heap?

>  
>  int singlestep;
>  static const char *argv0;
> @@ -610,7 +611,10 @@ static int parse_args(int argc, char **argv)
>          exit(EXIT_FAILURE);
>      }
>  
> -    exec_path = argv[optind];
> +    exec_path = realpath(argv[optind], exec_path_store);
> +    if (exec_path == NULL) {
> +        exec_path = argv[optind];
> +    }

  exec_path = realpath(argv[optind], NULL)
  exec_path = exec_path ? exec_path : argv[optind];

what situations would we expect realpath to fail and in those cases is
sticking to argv[optind] safe?

>  
>      return optind;
>  }


-- 
Alex Bennée

Re: [PATCH 2/5] linux-uesr: make exec_path realpath

[Takashi Yamamoto]

On Mon, May 24, 2021 at 7:59 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> YAMAMOTO Takashi <yamamoto@midokura.com> writes:
>
> > Otherwise, it can be easily fooled by the user app using chdir().
> >
> > Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
> > ---
> >  linux-user/main.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index 4dfc47ad3b..1f9f4e3820 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -55,6 +55,7 @@
> >  #endif
> >
> >  char *exec_path;
> > +char exec_path_store[PATH_MAX];
>
> Is there any point in keeping this as a static path rather than just
> allocating it off the heap?

it's just the simplest given the api of realpath().
do you mean it's better to use malloc? why?

>
> >
> >  int singlestep;
> >  static const char *argv0;
> > @@ -610,7 +611,10 @@ static int parse_args(int argc, char **argv)
> >          exit(EXIT_FAILURE);
> >      }
> >
> > -    exec_path = argv[optind];
> > +    exec_path = realpath(argv[optind], exec_path_store);
> > +    if (exec_path == NULL) {
> > +        exec_path = argv[optind];
> > +    }
>
>   exec_path = realpath(argv[optind], NULL)
>   exec_path = exec_path ? exec_path : argv[optind];
>
> what situations would we expect realpath to fail and in those cases is
> sticking to argv[optind] safe?

i don't have any particular case in my mind.
i guess it rarely fails and it might be simpler to just bail out on the failure.

>
> >
> >      return optind;
> >  }
>
>
> --
> Alex Bennée

Re: [PATCH 2/5] linux-uesr: make exec_path realpath

[Takashi Yamamoto]

On Tue, May 25, 2021 at 7:59 AM Takashi Yamamoto <yamamoto@midokura.com> wrote:
>
> On Mon, May 24, 2021 at 7:59 PM Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> >
> > YAMAMOTO Takashi <yamamoto@midokura.com> writes:
> >
> > > Otherwise, it can be easily fooled by the user app using chdir().
> > >
> > > Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
> > > ---
> > >  linux-user/main.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/linux-user/main.c b/linux-user/main.c
> > > index 4dfc47ad3b..1f9f4e3820 100644
> > > --- a/linux-user/main.c
> > > +++ b/linux-user/main.c
> > > @@ -55,6 +55,7 @@
> > >  #endif
> > >
> > >  char *exec_path;
> > > +char exec_path_store[PATH_MAX];
> >
> > Is there any point in keeping this as a static path rather than just
> > allocating it off the heap?
>
> it's just the simplest given the api of realpath().
> do you mean it's better to use malloc? why?
>
> >
> > >
> > >  int singlestep;
> > >  static const char *argv0;
> > > @@ -610,7 +611,10 @@ static int parse_args(int argc, char **argv)
> > >          exit(EXIT_FAILURE);
> > >      }
> > >
> > > -    exec_path = argv[optind];
> > > +    exec_path = realpath(argv[optind], exec_path_store);
> > > +    if (exec_path == NULL) {
> > > +        exec_path = argv[optind];
> > > +    }
> >
> >   exec_path = realpath(argv[optind], NULL)
> >   exec_path = exec_path ? exec_path : argv[optind];
> >
> > what situations would we expect realpath to fail and in those cases is
> > sticking to argv[optind] safe?
>
> i don't have any particular case in my mind.
> i guess it rarely fails and it might be simpler to just bail out on the failure.

i recalled why i did this way.
it was actually necessary for some apps. eg. runc
it executes an unlinked binary via /proc/self/fd.
i'll clean it up a bit and add comments.

>
> >
> > >
> > >      return optind;
> > >  }
> >
> >
> > --
> > Alex Bennée

[PATCH 3/5] linux-user: Fix the execfd case of /proc/self/exe open

[YAMAMOTO Takashi]

It's problematic to return AT_EXECFD as it is because the user app
would close it.
This patch opens it via /proc/self/fd instead.

Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
---
 linux-user/syscall.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a2b03ecb8b..14a63518e2 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8118,7 +8118,17 @@ static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags,
 
     if (is_proc_myself(pathname, "exe")) {
         int execfd = qemu_getauxval(AT_EXECFD);
-        return execfd ? execfd : safe_openat(dirfd, exec_path, flags, mode);
+        if (execfd) {
+            char filename[PATH_MAX];
+            int ret;
+
+            snprintf(filename, sizeof(filename), "/proc/self/fd/%d", execfd);
+            ret = safe_openat(dirfd, filename, flags, mode);
+            if (ret != -1) {
+                return ret;
+            }
+        }
+        return safe_openat(dirfd, exec_path, flags, mode);
     }
 
     for (fake_open = fakes; fake_open->filename; fake_open++) {
-- 
2.21.1 (Apple Git-122.3)

[PATCH 4/5] linux-user: dup the execfd on start up

[YAMAMOTO Takashi]

So that it can be used for other purposes (e.g. syscall.c)
after the elf loader closed it.

Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
---
 linux-user/main.c    | 8 ++++++++
 linux-user/qemu.h    | 2 ++
 linux-user/syscall.c | 5 ++---
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 1f9f4e3820..86ddba8b62 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -56,6 +56,7 @@
 
 char *exec_path;
 char exec_path_store[PATH_MAX];
+int exec_fd = -1;
 
 int singlestep;
 static const char *argv0;
@@ -833,6 +834,13 @@ int main(int argc, char **argv, char **envp)
     cpu->opaque = ts;
     task_settid(ts);
 
+    /*
+     * dup execfd to a global so that it can be used after loader_exec
+     * closes it.
+     */
+
+    exec_fd = dup(execfd);
+
     ret = loader_exec(execfd, exec_path, target_argv, target_environ, regs,
         info, &bprm);
     if (ret != 0) {
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 3b0b6b75fe..ee4e9a1779 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -160,6 +160,8 @@ typedef struct TaskState {
 } __attribute__((aligned(16))) TaskState;
 
 extern char *exec_path;
+extern int exec_fd;
+
 void init_task_state(TaskState *ts);
 void task_settid(TaskState *);
 void stop_all_tasks(void);
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 14a63518e2..2947e79dc0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8117,12 +8117,11 @@ static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags,
     };
 
     if (is_proc_myself(pathname, "exe")) {
-        int execfd = qemu_getauxval(AT_EXECFD);
-        if (execfd) {
+        if (exec_fd != -1) {
             char filename[PATH_MAX];
             int ret;
 
-            snprintf(filename, sizeof(filename), "/proc/self/fd/%d", execfd);
+            snprintf(filename, sizeof(filename), "/proc/self/fd/%d", exec_fd);
             ret = safe_openat(dirfd, filename, flags, mode);
             if (ret != -1) {
                 return ret;
-- 
2.21.1 (Apple Git-122.3)

[PATCH 5/5] linux-user: Implement pivot_root

[YAMAMOTO Takashi]

Used by runc.

Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
---
 linux-user/syscall.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2947e79dc0..e739921e86 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -35,6 +35,7 @@
 #include <sys/prctl.h>
 #include <sys/resource.h>
 #include <sys/swap.h>
+#include <sys/syscall.h>
 #include <linux/capability.h>
 #include <sched.h>
 #include <sys/timex.h>
@@ -8254,6 +8255,11 @@ static int host_to_target_cpu_mask(const unsigned long *host_mask,
     return 0;
 }
 
+static int pivot_root(const char *new_root, const char *put_old)
+{
+    return syscall(SYS_pivot_root, new_root, put_old);
+}
+
 /* This is an internal helper for do_syscall so that it is easier
  * to have a single return point, so that actions, such as logging
  * of syscall results, can be performed.
@@ -13222,6 +13228,23 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
         return ret;
 #endif
 
+#if defined(TARGET_NR_pivot_root)
+    case TARGET_NR_pivot_root:
+        {
+            void *p2;
+            p = lock_user_string(arg1); /* new_root */
+            p2 = lock_user_string(arg2); /* put_old */
+            if (!p || !p2) {
+                ret = -TARGET_EFAULT;
+            } else {
+                ret = get_errno(pivot_root(p, p2));
+            }
+            unlock_user(p2, arg2, 0);
+            unlock_user(p, arg1, 0);
+        }
+        return ret;
+#endif
+
     default:
         qemu_log_mask(LOG_UNIMP, "Unsupported syscall: %d\n", num);
         return -TARGET_ENOSYS;
-- 
2.21.1 (Apple Git-122.3)

Re: [PATCH 5/5] linux-user: Implement pivot_root

[Laurent Vivier]

Le 24/05/2021 à 06:54, YAMAMOTO Takashi a écrit :
> Used by runc.
> 
> Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
> ---
>  linux-user/syscall.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 2947e79dc0..e739921e86 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -35,6 +35,7 @@
>  #include <sys/prctl.h>
>  #include <sys/resource.h>
>  #include <sys/swap.h>
> +#include <sys/syscall.h>

we don't need that include, see below

>  #include <linux/capability.h>
>  #include <sched.h>
>  #include <sys/timex.h>
> @@ -8254,6 +8255,11 @@ static int host_to_target_cpu_mask(const unsigned long *host_mask,
>      return 0;
>  }
>  
> +static int pivot_root(const char *new_root, const char *put_old)
> +{
> +    return syscall(SYS_pivot_root, new_root, put_old);
> +}

Better to use:

#if defined(TARGET_NR_pivot_root) && defined(__NR_pivot_root)
_syscall2(int, pivot_root, const char *, new_root, const char *, put_old)
#endif

> +
>  /* This is an internal helper for do_syscall so that it is easier
>   * to have a single return point, so that actions, such as logging
>   * of syscall results, can be performed.
> @@ -13222,6 +13228,23 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>          return ret;
>  #endif
>  
> +#if defined(TARGET_NR_pivot_root)
> +    case TARGET_NR_pivot_root:
> +        {
> +            void *p2;
> +            p = lock_user_string(arg1); /* new_root */
> +            p2 = lock_user_string(arg2); /* put_old */
> +            if (!p || !p2) {
> +                ret = -TARGET_EFAULT;
> +            } else {
> +                ret = get_errno(pivot_root(p, p2));
> +            }
> +            unlock_user(p2, arg2, 0);
> +            unlock_user(p, arg1, 0);
> +        }
> +        return ret;
> +#endif
> +
>      default:
>          qemu_log_mask(LOG_UNIMP, "Unsupported syscall: %d\n", num);
>          return -TARGET_ENOSYS;
> 

Thanks,
Laurent

Re: [PATCH 5/5] linux-user: Implement pivot_root

[Takashi Yamamoto]

On Wed, May 26, 2021 at 5:22 AM Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 24/05/2021 à 06:54, YAMAMOTO Takashi a écrit :
> > Used by runc.
> >
> > Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
> > ---
> >  linux-user/syscall.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 2947e79dc0..e739921e86 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -35,6 +35,7 @@
> >  #include <sys/prctl.h>
> >  #include <sys/resource.h>
> >  #include <sys/swap.h>
> > +#include <sys/syscall.h>
>
> we don't need that include, see below
>
> >  #include <linux/capability.h>
> >  #include <sched.h>
> >  #include <sys/timex.h>
> > @@ -8254,6 +8255,11 @@ static int host_to_target_cpu_mask(const unsigned long *host_mask,
> >      return 0;
> >  }
> >
> > +static int pivot_root(const char *new_root, const char *put_old)
> > +{
> > +    return syscall(SYS_pivot_root, new_root, put_old);
> > +}
>
> Better to use:
>
> #if defined(TARGET_NR_pivot_root) && defined(__NR_pivot_root)
> _syscall2(int, pivot_root, const char *, new_root, const char *, put_old)
> #endif

ok. i haven't noticed the _syscall2 macro in this file. thank you.

>
> > +
> >  /* This is an internal helper for do_syscall so that it is easier
> >   * to have a single return point, so that actions, such as logging
> >   * of syscall results, can be performed.
> > @@ -13222,6 +13228,23 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
> >          return ret;
> >  #endif
> >
> > +#if defined(TARGET_NR_pivot_root)
> > +    case TARGET_NR_pivot_root:
> > +        {
> > +            void *p2;
> > +            p = lock_user_string(arg1); /* new_root */
> > +            p2 = lock_user_string(arg2); /* put_old */
> > +            if (!p || !p2) {
> > +                ret = -TARGET_EFAULT;
> > +            } else {
> > +                ret = get_errno(pivot_root(p, p2));
> > +            }
> > +            unlock_user(p2, arg2, 0);
> > +            unlock_user(p, arg1, 0);
> > +        }
> > +        return ret;
> > +#endif
> > +
> >      default:
> >          qemu_log_mask(LOG_UNIMP, "Unsupported syscall: %d\n", num);
> >          return -TARGET_ENOSYS;
> >
>
> Thanks,
> Laurent
>

Re: [PATCH 0/5] linux-user changes to run docker

[Alex Bennée]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]


YAMAMOTO Takashi <yamamoto@midokura.com> writes:

> These patches, along with a few more hacks [1] I didn't include
> in this patchset, allowed me to run arm64 and armv7 version of
> dind image on amd64.
>
> [1] https://github.com/yamt/qemu/tree/linux-user-for-docker

Might be worth posting those patches next time (even if they have a RFC
or !MERGE in the title for now). I had a little noodle around with
testing and quickly found a few holes. It would be nice if we could have
a unit test to cover these various bits as I fear it will easily break
again. Feel free to use the following as a basis if you want:


[-- Attachment #2: /proc/self test --]
[-- Type: text/x-diff, Size: 3695 bytes --]

From 5d331e84f3e8763921a619647a46bc8b4c9f3207 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>
Date: Mon, 24 May 2021 10:49:55 +0100
Subject: [PATCH 1/2] tests/tcg: simple test for /proc/self behaviour
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/multiarch/self.c | 114 +++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)
 create mode 100644 tests/tcg/multiarch/self.c

diff --git a/tests/tcg/multiarch/self.c b/tests/tcg/multiarch/self.c
new file mode 100644
index 0000000000..f6ea145d16
--- /dev/null
+++ b/tests/tcg/multiarch/self.c
@@ -0,0 +1,114 @@
+/*
+ * /proc/self checks
+ *
+ * Copyright (c) 2021 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#define _GNU_SOURCE
+#include <stdarg.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <assert.h>
+#include <fcntl.h>
+
+static void error1(const char *filename, int line, const char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    fprintf(stderr, "%s:%d: ", filename, line);
+    vfprintf(stderr, fmt, ap);
+    fprintf(stderr, "\n");
+    va_end(ap);
+    exit(1);
+}
+
+static int __chk_error(const char *filename, int line, int ret)
+{
+    if (ret < 0) {
+        error1(filename, line, "%m (ret=%d, errno=%d/%s)",
+               ret, errno, strerror(errno));
+    }
+    return ret;
+}
+
+#define error(fmt, ...) error1(__FILE__, __LINE__, fmt, ## __VA_ARGS__)
+
+#define chk_error(ret) __chk_error(__FILE__, __LINE__, (ret))
+
+#define PATH_MAX 1024
+
+static void check_self_exe(struct stat *self)
+{
+    struct stat statbuf;
+    struct stat linkbuf;
+    chk_error(stat("/proc/self/exe", &statbuf));
+    chk_error(lstat("/proc/self/exe", &linkbuf));
+    assert(statbuf.st_ino != linkbuf.st_ino);
+    assert(statbuf.st_ino == self->st_ino);
+}
+
+static void check_mypid(struct stat *self)
+{
+    pid_t me = getpid();
+    char path[PATH_MAX];
+    struct stat statbuf;
+    struct stat linkbuf;
+
+    snprintf(&path[0], PATH_MAX, "/proc/%d/exe", me);
+
+    chk_error(stat(path, &statbuf));
+    chk_error(lstat(path, &linkbuf));
+    assert(statbuf.st_ino != linkbuf.st_ino);
+    assert(statbuf.st_ino == self->st_ino);
+}
+
+static void check_noncanon(struct stat *self)
+{
+    struct stat statbuf;
+    int fd_slash, fd_dot;
+
+    fd_slash = openat(AT_FDCWD, "/proc///self/exe", O_PATH);
+    chk_error(fstat(fd_slash, &statbuf));
+    assert(statbuf.st_ino == self->st_ino);
+    close(fd_slash);
+
+    fd_dot = openat(AT_FDCWD, "/proc/./self/exe", O_PATH);
+    chk_error(fstat(fd_dot, &statbuf));
+    assert(statbuf.st_ino == self->st_ino);
+    close(fd_dot);
+}
+
+int main(int argc, char **argv)
+{
+    struct stat self;
+
+    chk_error(stat(argv[0], &self));
+    printf("I am %s (%d/%lu)\n", argv[0], argc,
+           (long unsigned int) self.st_ino);
+
+    check_self_exe(&self);
+    check_mypid(&self);
+    check_noncanon(&self);
+
+#if 0
+    if (argc == 2) {
+        printf("I think I execve'd myself\n");
+    } else {
+        char *new_argv[3] = { argv[0], "again", NULL };
+        chk_error(execve("/proc/self/exe", new_argv, NULL));
+        /* should never return */
+        return -1;
+    }
+#endif
+
+    return 0;
+}

base-commit: d36f3ecdc70af8941053cca8347daca757be2865
-- 
2.20.1


[-- Attachment #3: Type: text/plain, Size: 644 bytes --]



> You can find my test setup here:
> https://github.com/yamt/garbage/tree/master/binfmt-aarch64-install
>
> YAMAMOTO Takashi (5):
>   linux-user: handle /proc/self/exe for execve
>   linux-uesr: make exec_path realpath
>   linux-user: Fix the execfd case of /proc/self/exe open
>   linux-user: dup the execfd on start up
>   linux-user: Implement pivot_root
>
>  linux-user/main.c    | 14 +++++++++++++-
>  linux-user/qemu.h    |  2 ++
>  linux-user/syscall.c | 43 ++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 55 insertions(+), 4 deletions(-)

I also had a go at cleaning up is_proc_self and Daniel greatly
simplified it.


[-- Attachment #4: glib-ify is_proc_self --]
[-- Type: text/x-diff, Size: 2841 bytes --]

From fe342309661e3fe8b9e192e6df6ef84267207dac Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>
Date: Mon, 24 May 2021 12:19:18 +0100
Subject: [PATCH 2/2] linux-user: glib-ify is_proc_myself
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

For the cost of a couple of heap allocations we can reduce the code
complexity to a couple of string compares. While we are at it make the
function a bool return and fixup the fake_open function prototypes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use danpb's suggestion
---
 linux-user/syscall.c | 31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e739921e86..8af48b5f1f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7987,33 +7987,16 @@ static int open_self_auxv(void *cpu_env, int fd)
     return 0;
 }
 
-static int is_proc_myself(const char *filename, const char *entry)
-{
-    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
-        filename += strlen("/proc/");
-        if (!strncmp(filename, "self/", strlen("self/"))) {
-            filename += strlen("self/");
-        } else if (*filename >= '1' && *filename <= '9') {
-            char myself[80];
-            snprintf(myself, sizeof(myself), "%d/", getpid());
-            if (!strncmp(filename, myself, strlen(myself))) {
-                filename += strlen(myself);
-            } else {
-                return 0;
-            }
-        } else {
-            return 0;
-        }
-        if (!strcmp(filename, entry)) {
-            return 1;
-        }
-    }
-    return 0;
+static bool is_proc_myself(const char *filename, const char *entry)
+{
+    g_autofree char *procself = g_strdup_printf("/proc/self/%s", entry);
+    g_autofree char *procpid = g_strdup_printf("/proc/%d/%s", getpid(), entry);
+    return g_str_equal(filename, procself) || g_str_equal(filename, procpid);
 }
 
 #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN) || \
     defined(TARGET_SPARC) || defined(TARGET_M68K) || defined(TARGET_HPPA)
-static int is_proc(const char *filename, const char *entry)
+static bool is_proc(const char *filename, const char *entry)
 {
     return strcmp(filename, entry) == 0;
 }
@@ -8097,7 +8080,7 @@ static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags,
     struct fake_open {
         const char *filename;
         int (*fill)(void *cpu_env, int fd);
-        int (*cmp)(const char *s1, const char *s2);
+        bool (*cmp)(const char *s1, const char *s2);
     };
     const struct fake_open *fake_open;
     static const struct fake_open fakes[] = {
-- 
2.20.1


[-- Attachment #5: Type: text/plain, Size: 23 bytes --]



-- 
Alex Bennée

Re: [PATCH 0/5] linux-user changes to run docker

[Takashi Yamamoto]

On Tue, May 25, 2021 at 2:49 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> YAMAMOTO Takashi <yamamoto@midokura.com> writes:
>
> > These patches, along with a few more hacks [1] I didn't include
> > in this patchset, allowed me to run arm64 and armv7 version of
> > dind image on amd64.
> >
> > [1] https://github.com/yamt/qemu/tree/linux-user-for-docker
>
> Might be worth posting those patches next time (even if they have a RFC
> or !MERGE in the title for now).

ok.

> I had a little noodle around with
> testing and quickly found a few holes. It would be nice if we could have
> a unit test to cover these various bits as I fear it will easily break
> again. Feel free to use the following as a basis if you want:

frankly, i feel it's enough to cover the cases which are actually used
by real apps.
is "/proc/./self/exe" etc used in the field?

>
>
>
> > You can find my test setup here:
> > https://github.com/yamt/garbage/tree/master/binfmt-aarch64-install
> >
> > YAMAMOTO Takashi (5):
> >   linux-user: handle /proc/self/exe for execve
> >   linux-uesr: make exec_path realpath
> >   linux-user: Fix the execfd case of /proc/self/exe open
> >   linux-user: dup the execfd on start up
> >   linux-user: Implement pivot_root
> >
> >  linux-user/main.c    | 14 +++++++++++++-
> >  linux-user/qemu.h    |  2 ++
> >  linux-user/syscall.c | 43 ++++++++++++++++++++++++++++++++++++++++---
> >  3 files changed, 55 insertions(+), 4 deletions(-)
>
> I also had a go at cleaning up is_proc_self and Daniel greatly
> simplified it.

thank you for the info.
unfortunately the approach seems incompatible with what i want to do
eventually. (handle non-self cases as well)

>
>
>
> --
> Alex Bennée

Re: [PATCH 0/5] linux-user changes to run docker

[Takashi Yamamoto]

On Tue, May 25, 2021 at 8:22 AM Takashi Yamamoto <yamamoto@midokura.com> wrote:
>
> On Tue, May 25, 2021 at 2:49 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> >
> > YAMAMOTO Takashi <yamamoto@midokura.com> writes:
> >
> > > These patches, along with a few more hacks [1] I didn't include
> > > in this patchset, allowed me to run arm64 and armv7 version of
> > > dind image on amd64.
> > >
> > > [1] https://github.com/yamt/qemu/tree/linux-user-for-docker
> >
> > Might be worth posting those patches next time (even if they have a RFC
> > or !MERGE in the title for now).
>
> ok.

while RFC is mentioned in eg. git format-patch --help,
i couldn't find what !MERGE is.
can you provide a reference?

is there a nice way to express that some patches in a post are meant
for application and the others are RFC?

>
> > I had a little noodle around with
> > testing and quickly found a few holes. It would be nice if we could have
> > a unit test to cover these various bits as I fear it will easily break
> > again. Feel free to use the following as a basis if you want:
>
> frankly, i feel it's enough to cover the cases which are actually used
> by real apps.
> is "/proc/./self/exe" etc used in the field?
>
> >
> >
> >
> > > You can find my test setup here:
> > > https://github.com/yamt/garbage/tree/master/binfmt-aarch64-install
> > >
> > > YAMAMOTO Takashi (5):
> > >   linux-user: handle /proc/self/exe for execve
> > >   linux-uesr: make exec_path realpath
> > >   linux-user: Fix the execfd case of /proc/self/exe open
> > >   linux-user: dup the execfd on start up
> > >   linux-user: Implement pivot_root
> > >
> > >  linux-user/main.c    | 14 +++++++++++++-
> > >  linux-user/qemu.h    |  2 ++
> > >  linux-user/syscall.c | 43 ++++++++++++++++++++++++++++++++++++++++---
> > >  3 files changed, 55 insertions(+), 4 deletions(-)
> >
> > I also had a go at cleaning up is_proc_self and Daniel greatly
> > simplified it.
>
> thank you for the info.
> unfortunately the approach seems incompatible with what i want to do
> eventually. (handle non-self cases as well)
>
> >
> >
> >
> > --
> > Alex Bennée

Re: [PATCH 0/5] linux-user changes to run docker

[Alex Bennée]

Takashi Yamamoto <yamamoto@midokura.com> writes:

> On Tue, May 25, 2021 at 8:22 AM Takashi Yamamoto <yamamoto@midokura.com> wrote:
>>
>> On Tue, May 25, 2021 at 2:49 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>> >
>> >
>> > YAMAMOTO Takashi <yamamoto@midokura.com> writes:
>> >
>> > > These patches, along with a few more hacks [1] I didn't include
>> > > in this patchset, allowed me to run arm64 and armv7 version of
>> > > dind image on amd64.
>> > >
>> > > [1] https://github.com/yamt/qemu/tree/linux-user-for-docker
>> >
>> > Might be worth posting those patches next time (even if they have a RFC
>> > or !MERGE in the title for now).
>>
>> ok.
>
> while RFC is mentioned in eg. git format-patch --help,
> i couldn't find what !MERGE is.
> can you provide a reference?

It's usually just an annotation to the subject line of the commit, e.g:

  foo/bar: hacky fix to frobulator (!MERGE)

  rest of commit message

or something like:

  baz/quack: invert the tachyon beam (WIP)

  reason for the fix.

  [AJB: still WIP as this breaks foo]

AFAIK the only subject lines supported by the tooling are the squash:
and fixup: prefixes.

> is there a nice way to express that some patches in a post are meant
> for application and the others are RFC?

Aside from a description in the cover letter not really. The main reason
to include patches that aren't ready for merging is to show where your
work is going so the full context of earlier changes can be seen. Having
an ALL CAPS tag in the subject line is just handy for the maintainer
when scanning what might get cherry picked. Obviously if a patch totally
breaks the build it's not worth including as it just makes review harder
when giving the patches a spin so you should exercise your judgement.

-- 
Alex Bennée

Re: [PATCH 0/5] linux-user changes to run docker

[Takashi Yamamoto]

On Thu, May 27, 2021 at 10:25 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Takashi Yamamoto <yamamoto@midokura.com> writes:
>
> > On Tue, May 25, 2021 at 8:22 AM Takashi Yamamoto <yamamoto@midokura.com> wrote:
> >>
> >> On Tue, May 25, 2021 at 2:49 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> >> >
> >> >
> >> > YAMAMOTO Takashi <yamamoto@midokura.com> writes:
> >> >
> >> > > These patches, along with a few more hacks [1] I didn't include
> >> > > in this patchset, allowed me to run arm64 and armv7 version of
> >> > > dind image on amd64.
> >> > >
> >> > > [1] https://github.com/yamt/qemu/tree/linux-user-for-docker
> >> >
> >> > Might be worth posting those patches next time (even if they have a RFC
> >> > or !MERGE in the title for now).
> >>
> >> ok.
> >
> > while RFC is mentioned in eg. git format-patch --help,
> > i couldn't find what !MERGE is.
> > can you provide a reference?
>
> It's usually just an annotation to the subject line of the commit, e.g:
>
>   foo/bar: hacky fix to frobulator (!MERGE)
>
>   rest of commit message
>
> or something like:
>
>   baz/quack: invert the tachyon beam (WIP)
>
>   reason for the fix.
>
>   [AJB: still WIP as this breaks foo]
>
> AFAIK the only subject lines supported by the tooling are the squash:
> and fixup: prefixes.
>
> > is there a nice way to express that some patches in a post are meant
> > for application and the others are RFC?
>
> Aside from a description in the cover letter not really. The main reason
> to include patches that aren't ready for merging is to show where your
> work is going so the full context of earlier changes can be seen. Having
> an ALL CAPS tag in the subject line is just handy for the maintainer
> when scanning what might get cherry picked. Obviously if a patch totally
> breaks the build it's not worth including as it just makes review harder
> when giving the patches a spin so you should exercise your judgement.

ok. thank you for the explanation.

>
> --
> Alex Bennée