| Message ID | 20230705090056.610699-1-pierrick.bouvier@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] linux-user/syscall: Implement execve without execveat | expand |
05.07.2023 12:00, Pierrick Bouvier wrote: ... > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 08162cc966..4945ddd7f2 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -659,6 +659,7 @@ safe_syscall4(pid_t, wait4, pid_t, pid, int *, status, int, options, \ > #endif > safe_syscall5(int, waitid, idtype_t, idtype, id_t, id, siginfo_t *, infop, \ > int, options, struct rusage *, rusage) > +safe_syscall3(int, execve, const char *, filename, char **, argv, char **, envp) > safe_syscall5(int, execveat, int, dirfd, const char *, filename, > char **, argv, char **, envp, int, flags) > #if defined(TARGET_NR_select) || defined(TARGET_NR__newselect) || \ > @@ -8615,9 +8616,9 @@ ssize_t do_guest_readlink(const char *pathname, char *buf, size_t bufsiz) > return ret; > } > > -static int do_execveat(CPUArchState *cpu_env, int dirfd, > - abi_long pathname, abi_long guest_argp, > - abi_long guest_envp, int flags) > +static int do_execv(CPUArchState *cpu_env, int dirfd, > + abi_long pathname, abi_long guest_argp, > + abi_long guest_envp, int flags, bool is_execveat) > { > int ret; > char **argp, **envp; > @@ -8696,11 +8697,14 @@ static int do_execveat(CPUArchState *cpu_env, int dirfd, > goto execve_efault; > } > > + const char* exe = p; > if (is_proc_myself(p, "exe")) { > - ret = get_errno(safe_execveat(dirfd, exec_path, argp, envp, flags)); > - } else { > - ret = get_errno(safe_execveat(dirfd, p, argp, envp, flags)); > + exe = exec_path; > } > + ret = is_execveat ? > + safe_execveat(dirfd, exe, argp, envp, flags): > + safe_execve(exe, argp, envp); > + ret = get_errno(ret); This one has 2 issues reported by checkpatch.pl: $ git show | ./scripts/checkpatch.pl - ERROR: "foo* bar" should be "foo *bar" #161: FILE: linux-user/syscall.c:8700: + const char* exe = p; ERROR: spaces required around that ':' (ctx:VxE) #169: FILE: linux-user/syscall.c:8705: + safe_execveat(dirfd, exe, argp, envp, flags): ^ total: 2 errors, 0 warnings, 47 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. As I mentioned in the v1, I don't remember offhand how the arithmetic if should be styled in qemu. At the very best, the v2 variant is difficult to read because ":" is too close to ";" visually, an extra space before it will make it more explicit. Other than that, I'm fine with this version. With the checkpatch issues fixed, Reviewed-by: Michael Tokarev <mjt@tls.msk.ru> Thanks, /mjt
On 5/7/23 12:26, Michael Tokarev wrote: > 05.07.2023 12:00, Pierrick Bouvier wrote: > ... >> @@ -8696,11 +8697,14 @@ static int do_execveat(CPUArchState *cpu_env, >> int dirfd, >> goto execve_efault; >> } >> + const char* exe = p; >> if (is_proc_myself(p, "exe")) { >> - ret = get_errno(safe_execveat(dirfd, exec_path, argp, envp, >> flags)); >> - } else { >> - ret = get_errno(safe_execveat(dirfd, p, argp, envp, flags)); >> + exe = exec_path; >> } >> + ret = is_execveat ? >> + safe_execveat(dirfd, exe, argp, envp, flags): >> + safe_execve(exe, argp, envp); >> + ret = get_errno(ret); > ERROR: spaces required around that ':' (ctx:VxE) > #169: FILE: linux-user/syscall.c:8705: > + safe_execveat(dirfd, exe, argp, envp, flags): > ^ > > total: 2 errors, 0 warnings, 47 lines checked > > Your patch has style problems, please review. If any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. > > As I mentioned in the v1, I don't remember offhand how the arithmetic if > should be styled in qemu. At the very best, the v2 variant is difficult > to read because ":" is too close to ";" visually, an extra space before > it will make it more explicit. KISS, alternatively: ret = is_execveat ? safe_execveat(dirfd, exe, argp, envp, flags) : safe_execve(exe, argp, envp);
On 7/5/23 12:26, Michael Tokarev wrote: > 05.07.2023 12:00, Pierrick Bouvier wrote: > ... >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index 08162cc966..4945ddd7f2 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -659,6 +659,7 @@ safe_syscall4(pid_t, wait4, pid_t, pid, int *, status, int, options, \ >> #endif >> safe_syscall5(int, waitid, idtype_t, idtype, id_t, id, siginfo_t *, infop, \ >> int, options, struct rusage *, rusage) >> +safe_syscall3(int, execve, const char *, filename, char **, argv, char **, envp) >> safe_syscall5(int, execveat, int, dirfd, const char *, filename, >> char **, argv, char **, envp, int, flags) >> #if defined(TARGET_NR_select) || defined(TARGET_NR__newselect) || \ >> @@ -8615,9 +8616,9 @@ ssize_t do_guest_readlink(const char *pathname, char *buf, size_t bufsiz) >> return ret; >> } >> >> -static int do_execveat(CPUArchState *cpu_env, int dirfd, >> - abi_long pathname, abi_long guest_argp, >> - abi_long guest_envp, int flags) >> +static int do_execv(CPUArchState *cpu_env, int dirfd, >> + abi_long pathname, abi_long guest_argp, >> + abi_long guest_envp, int flags, bool is_execveat) >> { >> int ret; >> char **argp, **envp; >> @@ -8696,11 +8697,14 @@ static int do_execveat(CPUArchState *cpu_env, int dirfd, >> goto execve_efault; >> } >> >> + const char* exe = p; >> if (is_proc_myself(p, "exe")) { >> - ret = get_errno(safe_execveat(dirfd, exec_path, argp, envp, flags)); >> - } else { >> - ret = get_errno(safe_execveat(dirfd, p, argp, envp, flags)); >> + exe = exec_path; >> } >> + ret = is_execveat ? >> + safe_execveat(dirfd, exe, argp, envp, flags): >> + safe_execve(exe, argp, envp); >> + ret = get_errno(ret); > > > This one has 2 issues reported by checkpatch.pl: > > $ git show | ./scripts/checkpatch.pl - > > ERROR: "foo* bar" should be "foo *bar" > #161: FILE: linux-user/syscall.c:8700: > + const char* exe = p; > > ERROR: spaces required around that ':' (ctx:VxE) > #169: FILE: linux-user/syscall.c:8705: > + safe_execveat(dirfd, exe, argp, envp, flags): > ^ > > total: 2 errors, 0 warnings, 47 lines checked > > Your patch has style problems, please review. If any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. > > As I mentioned in the v1, I don't remember offhand how the arithmetic if > should be styled in qemu. At the very best, the v2 variant is difficult > to read because ":" is too close to ";" visually, an extra space before > it will make it more explicit. > Sorry about it. I'll fix it, and use style advised by Philippe. > Other than that, I'm fine with this version. > > With the checkpatch issues fixed, > > Reviewed-by: Michael Tokarev <mjt@tls.msk.ru> > > Thanks, > > /mjt
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 08162cc966..4945ddd7f2 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -659,6 +659,7 @@ safe_syscall4(pid_t, wait4, pid_t, pid, int *, status, int, options, \ #endif safe_syscall5(int, waitid, idtype_t, idtype, id_t, id, siginfo_t *, infop, \ int, options, struct rusage *, rusage) +safe_syscall3(int, execve, const char *, filename, char **, argv, char **, envp) safe_syscall5(int, execveat, int, dirfd, const char *, filename, char **, argv, char **, envp, int, flags) #if defined(TARGET_NR_select) || defined(TARGET_NR__newselect) || \ @@ -8615,9 +8616,9 @@ ssize_t do_guest_readlink(const char *pathname, char *buf, size_t bufsiz) return ret; } -static int do_execveat(CPUArchState *cpu_env, int dirfd, - abi_long pathname, abi_long guest_argp, - abi_long guest_envp, int flags) +static int do_execv(CPUArchState *cpu_env, int dirfd, + abi_long pathname, abi_long guest_argp, + abi_long guest_envp, int flags, bool is_execveat) { int ret; char **argp, **envp; @@ -8696,11 +8697,14 @@ static int do_execveat(CPUArchState *cpu_env, int dirfd, goto execve_efault; } + const char* exe = p; if (is_proc_myself(p, "exe")) { - ret = get_errno(safe_execveat(dirfd, exec_path, argp, envp, flags)); - } else { - ret = get_errno(safe_execveat(dirfd, p, argp, envp, flags)); + exe = exec_path; } + ret = is_execveat ? + safe_execveat(dirfd, exe, argp, envp, flags): + safe_execve(exe, argp, envp); + ret = get_errno(ret); unlock_user(p, pathname, 0); @@ -9251,9 +9255,9 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, return ret; #endif case TARGET_NR_execveat: - return do_execveat(cpu_env, arg1, arg2, arg3, arg4, arg5); + return do_execv(cpu_env, arg1, arg2, arg3, arg4, arg5, true); case TARGET_NR_execve: - return do_execveat(cpu_env, AT_FDCWD, arg1, arg2, arg3, 0); + return do_execv(cpu_env, AT_FDCWD, arg1, arg2, arg3, 0, false); case TARGET_NR_chdir: if (!(p = lock_user_string(arg1))) return -TARGET_EFAULT;
Support for execveat syscall was implemented in 55bbe4 and is available since QEMU 8.0.0. It relies on host execveat, which is widely available on most of Linux kernels today. However, this change breaks qemu-user self emulation, if "host" qemu version is less than 8.0.0. Indeed, it does not implement yet execveat. This strange use case happens with most of distribution today having binfmt support. With a concrete failing example: $ qemu-x86_64-7.2 qemu-x86_64-8.0 /bin/bash -c /bin/ls /bin/bash: line 1: /bin/ls: Function not implemented -> not implemented means execve returned ENOSYS qemu-user-static 7.2 and 8.0 can be conveniently grabbed from debian packages qemu-user-static* [1]. One usage of this is running wine-arm64 from linux-x64 (details [2]). This is by updating qemu embedded in docker image that we ran into this issue. The solution to update host qemu is not always possible. Either it's complicated or ask you to recompile it, or simply is not accessible (GitLab CI, GitHub Actions). Thus, it could be worth to implement execve without relying on execveat, which is the goal of this patch. This patch was tested with example presented in this commit message. [1] http://ftp.us.debian.org/debian/pool/main/q/qemu/ [1] https://www.linaro.org/blog/emulate-windows-on-arm/ Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> --- linux-user/syscall.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)