[PATCH] linux-user: Implement faccessat2

[PATCH] linux-user: Implement faccessat2

[WANG Xuerui]

User space has been preferring this syscall for a while, due to its
closer match with C semantics, and newer platforms such as LoongArch
apparently have libc implementations that don't fallback to faccessat
so normal access checks are failing without the emulation in place.

Tested by successfully emerging several packages within a Gentoo loong
stage3 chroot, emulated on amd64 with help of static qemu-loongarch64.

Reported-by: Andreas K. Hüttel <dilfridge@gentoo.org>
Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
---
 linux-user/strace.list | 3 +++
 linux-user/syscall.c   | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/linux-user/strace.list b/linux-user/strace.list
index a87415bf3d..3df2184580 100644
--- a/linux-user/strace.list
+++ b/linux-user/strace.list
@@ -178,6 +178,9 @@
 #ifdef TARGET_NR_faccessat
 { TARGET_NR_faccessat, "faccessat" , NULL, print_faccessat, NULL },
 #endif
+#ifdef TARGET_NR_faccessat2
+{ TARGET_NR_faccessat2, "faccessat2" , NULL, print_faccessat, NULL },
+#endif
 #ifdef TARGET_NR_fadvise64
 { TARGET_NR_fadvise64, "fadvise64" , NULL, NULL, NULL },
 #endif
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2e954d8dbd..a81f0b65b9 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9110,6 +9110,15 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
         unlock_user(p, arg2, 0);
         return ret;
 #endif
+#if defined(TARGET_NR_faccessat2) && defined(__NR_faccessat2)
+    case TARGET_NR_faccessat2:
+        if (!(p = lock_user_string(arg2))) {
+            return -TARGET_EFAULT;
+        }
+        ret = get_errno(faccessat(arg1, p, arg3, arg4));
+        unlock_user(p, arg2, 0);
+        return ret;
+#endif
 #ifdef TARGET_NR_nice /* not on alpha */
     case TARGET_NR_nice:
         return get_errno(nice(arg1));
-- 
2.38.0

Re: [PATCH] linux-user: Implement faccessat2

[Helge Deller]

On 10/9/22 08:08, WANG Xuerui wrote:
> User space has been preferring this syscall for a while, due to its
> closer match with C semantics, and newer platforms such as LoongArch
> apparently have libc implementations that don't fallback to faccessat
> so normal access checks are failing without the emulation in place.
>
> Tested by successfully emerging several packages within a Gentoo loong
> stage3 chroot, emulated on amd64 with help of static qemu-loongarch64.
>
> Reported-by: Andreas K. Hüttel <dilfridge@gentoo.org>
> Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
> ---
>   linux-user/strace.list | 3 +++
>   linux-user/syscall.c   | 9 +++++++++
>   2 files changed, 12 insertions(+)

There were two similar approaches from Richard and me:
https://lore.kernel.org/qemu-devel/20220729201414.29869-1-richard.henderson@linaro.org/#t
and
https://lore.kernel.org/qemu-devel/YzLdcnL6x646T61W@p100/

> diff --git a/linux-user/strace.list b/linux-user/strace.list
> index a87415bf3d..3df2184580 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -178,6 +178,9 @@
>   #ifdef TARGET_NR_faccessat
>   { TARGET_NR_faccessat, "faccessat" , NULL, print_faccessat, NULL },
>   #endif
> +#ifdef TARGET_NR_faccessat2
> +{ TARGET_NR_faccessat2, "faccessat2" , NULL, print_faccessat, NULL },
> +#endif

You are missing that part (from my patch):

--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -1931,7 +1931,7 @@ print_execv(CPUArchState *cpu_env, const struct syscallname *name,
  }
  #endif

-#ifdef TARGET_NR_faccessat
+#if defined(TARGET_NR_faccessat) || defined(TARGET_NR_faccessat2)

otherwise if TARGET_NR_faccessat isn't defined, you won't have
the function print_faccessat() in strace.c defined.


>   #ifdef TARGET_NR_fadvise64
>   { TARGET_NR_fadvise64, "fadvise64" , NULL, NULL, NULL },
>   #endif
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 2e954d8dbd..a81f0b65b9 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -9110,6 +9110,15 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
>           unlock_user(p, arg2, 0);
>           return ret;
>   #endif
> +#if defined(TARGET_NR_faccessat2) && defined(__NR_faccessat2)
> +    case TARGET_NR_faccessat2:
> +        if (!(p = lock_user_string(arg2))) {
> +            return -TARGET_EFAULT;
> +        }
> +        ret = get_errno(faccessat(arg1, p, arg3, arg4));

You rely here on the libc faccessat() function to either use
faccessat2() or faccessat() syscalls - which is probably the
best way around...

Helge

> +        unlock_user(p, arg2, 0);
> +        return ret;
> +#endif
>   #ifdef TARGET_NR_nice /* not on alpha */
>       case TARGET_NR_nice:
>           return get_errno(nice(arg1));

Re: [PATCH] linux-user: Implement faccessat2

[Michael Tokarev]

10.10.2022 11:53, Helge Deller wrote:
> On 10/9/22 08:08, WANG Xuerui wrote:
>> User space has been preferring this syscall for a while, due to its
>> closer match with C semantics, and newer platforms such as LoongArch
>> apparently have libc implementations that don't fallback to faccessat
>> so normal access checks are failing without the emulation in place.
>>
>> Tested by successfully emerging several packages within a Gentoo loong
>> stage3 chroot, emulated on amd64 with help of static qemu-loongarch64.
>>
>> Reported-by: Andreas K. Hüttel <dilfridge@gentoo.org>
>> Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
>> ---
>>   linux-user/strace.list | 3 +++
>>   linux-user/syscall.c   | 9 +++++++++
>>   2 files changed, 12 insertions(+)
> 
> There were two similar approaches from Richard and me:
> https://lore.kernel.org/qemu-devel/20220729201414.29869-1-richard.henderson@linaro.org/#t
> and
> https://lore.kernel.org/qemu-devel/YzLdcnL6x646T61W@p100/

So can we combine the 3 and actually implement it for good? :)

For loongarch64 users this has become essential, because this is a
new enough arch so that userspace does not bother using older syscalls,
in this case it uses faccessat2() for everything, and simplest programs
fail under qemu due to no fallback whatsoever.

Thanks,

/mjt

Re: [PATCH] linux-user: Implement faccessat2

[Luca Bonissi]

On 18/10/22 11:58, Michael Tokarev wrote:
> 10.10.2022 11:53, Helge Deller wrote:
>> On 10/9/22 08:08, WANG Xuerui wrote:
>>> User space has been preferring this syscall for a while, due to its
>>> closer match with C semantics, and newer platforms such as LoongArch
>>> apparently have libc implementations that don't fallback to faccessat
>>> so normal access checks are failing without the emulation in place.
>>
>> https://lore.kernel.org/qemu-devel/YzLdcnL6x646T61W@p100/

I think this one is the more complete and simplest solution.
Only change:

+#if defined(TARGET_NR_faccessat2) && defined(__NR_faccessat2)

with

+#if defined(TARGET_NR_faccessat2)

(not necessary to have host __NR_faccessat2)

and replace "faccessat2(...)" with "faccessat(...)", so it uses glibc 
implementation, that uses __NR_faccessat2 if host has this syscall, 
otherwise it falls back to faccessat with the addition of fstatat if 
flags!=0 (obviously, the definition of syscall4(... faccessat2 ...) 
should be removed).

> For loongarch64 users this has become essential, because this is a
> new enough arch so that userspace does not bother using older syscalls,
> in this case it uses faccessat2() for everything, and simplest programs
> fail under qemu due to no fallback whatsoever.

I agree that it has become essential. Development with qemu-user is much 
faster than using qemu-system, with all the benefits to use chroot on a 
shared file system.

I tested (and currently testing) the above patch with Slackware-current 
build scripts on x86_64 host: all works fine!

Thanks!
   Luca

Re: [PATCH] linux-user: Implement faccessat2

[Laurent Vivier]

Le 09/10/2022 à 08:08, WANG Xuerui a écrit :
> User space has been preferring this syscall for a while, due to its
> closer match with C semantics, and newer platforms such as LoongArch
> apparently have libc implementations that don't fallback to faccessat
> so normal access checks are failing without the emulation in place.
> 
> Tested by successfully emerging several packages within a Gentoo loong
> stage3 chroot, emulated on amd64 with help of static qemu-loongarch64.
> 
> Reported-by: Andreas K. Hüttel <dilfridge@gentoo.org>
> Signed-off-by: WANG Xuerui <xen0n@gentoo.org>
> ---
>   linux-user/strace.list | 3 +++
>   linux-user/syscall.c   | 9 +++++++++
>   2 files changed, 12 insertions(+)
> 
> diff --git a/linux-user/strace.list b/linux-user/strace.list
> index a87415bf3d..3df2184580 100644
> --- a/linux-user/strace.list
> +++ b/linux-user/strace.list
> @@ -178,6 +178,9 @@
>   #ifdef TARGET_NR_faccessat
>   { TARGET_NR_faccessat, "faccessat" , NULL, print_faccessat, NULL },
>   #endif
> +#ifdef TARGET_NR_faccessat2
> +{ TARGET_NR_faccessat2, "faccessat2" , NULL, print_faccessat, NULL },
> +#endif
>   #ifdef TARGET_NR_fadvise64
>   { TARGET_NR_fadvise64, "fadvise64" , NULL, NULL, NULL },
>   #endif
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 2e954d8dbd..a81f0b65b9 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -9110,6 +9110,15 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
>           unlock_user(p, arg2, 0);
>           return ret;
>   #endif
> +#if defined(TARGET_NR_faccessat2) && defined(__NR_faccessat2)
> +    case TARGET_NR_faccessat2:
> +        if (!(p = lock_user_string(arg2))) {
> +            return -TARGET_EFAULT;
> +        }
> +        ret = get_errno(faccessat(arg1, p, arg3, arg4));
> +        unlock_user(p, arg2, 0);
> +        return ret;
> +#endif
>   #ifdef TARGET_NR_nice /* not on alpha */
>       case TARGET_NR_nice:
>           return get_errno(nice(arg1));

I have applied this patch to my linux-user-for-7.2 branch,
adding defined(TARGET_NR_faccessat2) for print_faccessat() and removing the defined(__NR_faccessat2) 
in syscall.c (as we call the glibc wrapper).

Thanks,
Laurent

Re: [PATCH] linux-user: Implement faccessat2

[Richard Henderson]

On 7/29/22 13:14, Richard Henderson wrote:
> -        ret = get_errno(access(path(p), arg2));
> -        unlock_user(p, arg1, 0);
> -        return ret;
> +        return do_faccessat2(AT_FDCWD, arg1, arg2, 0);

Oops, dropped path().

Should perhaps be incorporated into the helper, because newer targets won't have or use 
plain access()...


r~

[PATCH] linux-user: Implement faccessat2

[Richard Henderson]

Split out do_faccessat2 helper, and use it for
accessat and faccessat as well.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
Will we ever have a system libc for which __NR_faccessat2 is present,
but faccessat() does not try faccessat2 first?

r~
---
 linux-user/syscall.c | 44 +++++++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b27a6552aa..acd8452048 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8530,6 +8530,30 @@ static int do_getdents64(abi_long dirfd, abi_long arg2, abi_long count)
 _syscall2(int, pivot_root, const char *, new_root, const char *, put_old)
 #endif
 
+static int do_faccessat2(int dirfd, abi_ptr pathname, int mode, int flags)
+{
+    char *p = lock_user_string(pathname);
+    bool nosys = true;
+    int ret;
+
+    if (!p) {
+        return -TARGET_EFAULT;
+    }
+
+    /* Use the raw host syscall if possible, in case we have an old libc. */
+#ifdef __NR_faccessat2
+    ret = syscall(__NR_faccessat2, dirfd, p, mode, flags);
+    nosys = ret < 0 && errno == ENOSYS;
+#endif
+    /* If we don't have the syscall, defer to libc emulation. */
+    if (nosys) {
+        ret = faccessat(dirfd, p, mode, flags);
+    }
+
+    unlock_user(p, pathname, 0);
+    return get_errno(ret);
+}
+
 /* 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.
@@ -9058,21 +9082,15 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
 #endif
 #ifdef TARGET_NR_access
     case TARGET_NR_access:
-        if (!(p = lock_user_string(arg1))) {
-            return -TARGET_EFAULT;
-        }
-        ret = get_errno(access(path(p), arg2));
-        unlock_user(p, arg1, 0);
-        return ret;
+        return do_faccessat2(AT_FDCWD, arg1, arg2, 0);
 #endif
-#if defined(TARGET_NR_faccessat) && defined(__NR_faccessat)
+#ifdef TARGET_NR_faccessat
     case TARGET_NR_faccessat:
-        if (!(p = lock_user_string(arg2))) {
-            return -TARGET_EFAULT;
-        }
-        ret = get_errno(faccessat(arg1, p, arg3, 0));
-        unlock_user(p, arg2, 0);
-        return ret;
+        return do_faccessat2(arg1, arg2, arg3, 0);
+#endif
+#ifdef TARGET_NR_faccessat2
+    case TARGET_NR_faccessat2:
+        return do_faccessat2(arg1, arg2, arg3, arg4);
 #endif
 #ifdef TARGET_NR_nice /* not on alpha */
     case TARGET_NR_nice:
-- 
2.34.1