[v10,3/3] linux-user: Add support for statx() syscall

From: Aleksandar Rikalo <arikalo@wavecomp.com>

Implement support for translation of system call statx().

The implementation is based on "best effort" approach: if host is
capable of executing statx(), host statx() is used. If not, the
implementation includes invoking other (more mature) system calls
(from the same 'stat' family) on the host side to achieve as close
as possible functionality.

Support for statx() in kernel and glibc was, however, introduced
at different points of time (the difference is more than a year):

  - kernel: Linux 4.11 (30 April 2017)
  - glibc: glibc 2.28 (1 Aug 2018)

In this patch, the availability of statx() support is established
via __NR_statx (if it is defined, statx() is considered available).
This coincedes with statx() introduction in kernel.

However, the structure statx definition may not be available for hosts
with glibc older than 2.28 (it is, by design, to be defined in one of
glibc headers), even though the full statx() functionality may be
supported in kernel, if the kernel is not older than 4.11. Hence,
a structure "target_statx" is defined in this patch, to remove that
dependency on glibc headers, and to use statx() functionality as soon
as the host kernel is capable of supporting it. Such structure statx
definition is used for both target and host structures statx (of
course, this doesn't mean the endian arrangement is the same on
target and host, and endian conversion is done in all necessary
cases).

Signed-off-by: Aleksandar Rikalo <arikalo@wavecomp.com>
Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
---
 linux-user/syscall.c      | 135 +++++++++++++++++++++++++++++++++++++++++++++-
 linux-user/syscall_defs.h |  37 +++++++++++++
 2 files changed, 171 insertions(+), 1 deletion(-)

Le 07/06/2019 à 12:35, Aleksandar Markovic a écrit :
> From: Aleksandar Rikalo <arikalo@wavecomp.com>
> 
> Implement support for translation of system call statx().
> 
> The implementation is based on "best effort" approach: if host is
> capable of executing statx(), host statx() is used. If not, the
> implementation includes invoking other (more mature) system calls
> (from the same 'stat' family) on the host side to achieve as close
> as possible functionality.
> 
> Support for statx() in kernel and glibc was, however, introduced
> at different points of time (the difference is more than a year):
> 
>   - kernel: Linux 4.11 (30 April 2017)
>   - glibc: glibc 2.28 (1 Aug 2018)
> 
> In this patch, the availability of statx() support is established
> via __NR_statx (if it is defined, statx() is considered available).
> This coincedes with statx() introduction in kernel.
> 
> However, the structure statx definition may not be available for hosts
> with glibc older than 2.28 (it is, by design, to be defined in one of
> glibc headers), even though the full statx() functionality may be
> supported in kernel, if the kernel is not older than 4.11. Hence,
> a structure "target_statx" is defined in this patch, to remove that
> dependency on glibc headers, and to use statx() functionality as soon
> as the host kernel is capable of supporting it. Such structure statx
> definition is used for both target and host structures statx (of
> course, this doesn't mean the endian arrangement is the same on
> target and host, and endian conversion is done in all necessary
> cases).
> 
> Signed-off-by: Aleksandar Rikalo <arikalo@wavecomp.com>
> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> ---
>  linux-user/syscall.c      | 135 +++++++++++++++++++++++++++++++++++++++++++++-
>  linux-user/syscall_defs.h |  37 +++++++++++++
>  2 files changed, 171 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 82c08b6..b78ed45 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
...
> @@ -10182,6 +10226,95 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>              ret = host_to_target_stat64(cpu_env, arg3, &st);
>          return ret;
>  #endif
> +#if defined(TARGET_NR_statx)
> +    case TARGET_NR_statx:
> +        {
> +            struct target_statx *target_stx;
> +            int dirfd = arg1;
> +            int flags = arg3;
> +
> +            p = lock_user_string(arg2);
> +            if (p == NULL) {
> +                return -TARGET_EFAULT;
> +            }
> +#if defined(__NR_statx)
> +            {
> +                /*
> +                 * It is assumed that struct statx is arhitecture independent

s/arhitecture/architecture/

> +                 */
> +                struct target_statx host_stx;
> +                int mask = arg4;
> +
> +                ret = get_errno(syscall(__NR_statx, dirfd, p, flags, mask,
> +                                        &host_stx));

You should define sys_statx() using _syscall5() macro and use it.

> +                if (!is_error(ret)) {
> +                    if (host_to_target_statx(&host_stx, arg5) != 0) {
> +                        unlock_user(p, arg2, 0);
> +                        return -TARGET_EFAULT;
> +                    }
> +                }
> +
> +                if (ret != TARGET_ENOSYS) {

ret != -TARGET_ENOSYS

> +                    unlock_user(p, arg2, 0);
> +                    return ret;
> +                }
> +            }
> +#endif
> +            if ((p == NULL) || (*((char *)p) == 0)) {

You already checked above p is not NULL and exited with -TARGET_EFAULT.

BTW, do we really need to emulate the syscall if it is not available?

I think the user-space application calling statx() should be ready to
receive ENOSYS and define some kinds of fallback (like you do below). So
it should not be done by QEMU.

> +                /*
> +                 * By file descriptor
> +                 */
> +                if (flags & AT_EMPTY_PATH) {
> +                    unlock_user(p, arg2, 0);
> +                    return -TARGET_ENOENT;
> +                }
> +                ret = get_errno(fstat(dirfd, &st));
> +            } else if (*((char *)p) == '/') {
> +                /*
> +                 * By absolute pathname
> +                 */
> +                ret = get_errno(stat(path(p), &st));
> +            } else {
> +                if (dirfd == AT_FDCWD) {
> +                    /*
> +                     * By pathname relative to the current working directory
> +                     */
> +                    ret = get_errno(stat(path(p), &st));

Why do we divide the case in two parts, fstatat() should work here too.

> +                } else {
> +                    /*
> +                     * By pathname relative to the directory referred to by
> +                     * the file descriptor 'dirfd'
> +                     */
> +                    ret = get_errno(fstatat(dirfd, path(p), &st, flags));
> +                }
> +            }
> +            unlock_user(p, arg2, 0);
> +
> +            if (!is_error(ret)) {
> +                if (!lock_user_struct(VERIFY_WRITE, target_stx, arg5, 0)) {
> +                    return -TARGET_EFAULT;
> +                }
> +                memset(target_stx, 0, sizeof(*target_stx));
> +                __put_user(major(st.st_dev), &target_stx->stx_dev_major);
> +                __put_user(minor(st.st_dev), &target_stx->stx_dev_minor);
> +                __put_user(st.st_ino, &target_stx->stx_ino);
> +                __put_user(st.st_mode, &target_stx->stx_mode);
> +                __put_user(st.st_uid, &target_stx->stx_uid);
> +                __put_user(st.st_gid, &target_stx->stx_gid);
> +                __put_user(st.st_nlink, &target_stx->stx_nlink);
> +                __put_user(major(st.st_rdev), &target_stx->stx_rdev_major);
> +                __put_user(minor(st.st_rdev), &target_stx->stx_rdev_minor);
> +                __put_user(st.st_size, &target_stx->stx_size);
> +                __put_user(st.st_blksize, &target_stx->stx_blksize);
> +                __put_user(st.st_blocks, &target_stx->stx_blocks);
> +                __put_user(st.st_atime, &target_stx->stx_atime.tv_sec);
> +                __put_user(st.st_mtime, &target_stx->stx_mtime.tv_sec);
> +                __put_user(st.st_ctime, &target_stx->stx_ctime.tv_sec);
> +                unlock_user_struct(target_stx, arg5, 1);
> +            }
> +        }
> +        return ret;
> +#endif

Thanks,
Laurent

On 6/7/19 3:35 AM, Aleksandar Markovic wrote:
> Implement support for translation of system call statx().

I also need these patches for 32-bit RISC-V linux user mode support.

glibc ld.so calls statx if fstatat is not supported.  Apparently new 
linux architecture ports aren't allowed to define __ARCH_WANT_NEW_STAT 
which enables fstatat because this is already obsolete.  64-bit RISC-V 
linux does have fstatat, but apparently this was a mistake which we 
can't fix now because the ABI is already frozen.  The 32-bit RISC-V ABI 
is not frozen yet, so it won't have fstatat.  Anyways, without statx, 
ld.so doesn't work, which makes user mode qemu pretty useless, so we do 
need this emulated in qemu to make the 32-bit RISC-V linux user mode 
support work properly.

I started with the August 2018 version of the patch a few weeks ago, and 
just noticed that it has been resubmitted.  I had to modify the patch 
slightly to apply to current sources, and had to fix one bug to make it 
work.  The line
+                if (ret != TARGET_ENOSYS) {
needs to instead be
+                if (ret != -TARGET_ENOSYS) {
I see that Laurent has already pointed that out.

Incidentally, I also have strace patches for statx that work on top of 
this patch, since I didn't see that in the nanomips patch set I started 
with.  That helped me debug the 32-bit RISC-V user mode support.

I've tested this on Ubuntu 16.04 (no host statx) and Ubuntu 19.10 (with 
host statx) and it worked well for me running the gcc testsuite for a 
riscv32-linux target.  I haven't tried testing the latest version of the 
patch yet.  I can do that if this is helpful.

Jim

On Wednesday, June 19, 2019, Jim Wilson <jimw@sifive.com> wrote:

> On 6/7/19 3:35 AM, Aleksandar Markovic wrote:
>
>> Implement support for translation of system call statx().
>>
>
> I also need these patches for 32-bit RISC-V linux user mode support.
>
> glibc ld.so calls statx if fstatat is not supported.  Apparently new linux
> architecture ports aren't allowed to define __ARCH_WANT_NEW_STAT which
> enables fstatat because this is already obsolete.  64-bit RISC-V linux does
> have fstatat, but apparently this was a mistake which we can't fix now
> because the ABI is already frozen.  The 32-bit RISC-V ABI is not frozen
> yet, so it won't have fstatat.  Anyways, without statx, ld.so doesn't work,
> which makes user mode qemu pretty useless, so we do need this emulated in
> qemu to make the 32-bit RISC-V linux user mode support work properly.
>
>
Jim, Aleksandar Rikalo, the author of the patch was about to send Laurent
explanation why this aproach is needed in its current organization, on a
very similar line of reasoning as yours.

I am waiting on him to send a new version of the series. Meanwhile you can
send strace patch to the list, and I can even incude it in my series after
and together with Aleksandar's patch, if you don't object.

Yours,

Aleksandar M.

I started with the August 2018 version of the patch a few weeks ago, and
> just noticed that it has been resubmitted.  I had to modify the patch
> slightly to apply to current sources, and had to fix one bug to make it
> work.  The line
> +                if (ret != TARGET_ENOSYS) {
> needs to instead be
> +                if (ret != -TARGET_ENOSYS) {
> I see that Laurent has already pointed that out.
>
> Incidentally, I also have strace patches for statx that work on top of
> this patch, since I didn't see that in the nanomips patch set I started
> with.  That helped me debug the 32-bit RISC-V user mode support.
>
> I've tested this on Ubuntu 16.04 (no host statx) and Ubuntu 19.10 (with
> host statx) and it worked well for me running the gcc testsuite for a
> riscv32-linux target.  I haven't tried testing the latest version of the
> patch yet.  I can do that if this is helpful.
>
> Jim
>
>

On Tue, Jun 18, 2019 at 4:13 PM Aleksandar Markovic
<aleksandar.m.mail@gmail.com> wrote:
> I am waiting on him to send a new version of the series. Meanwhile you can send strace patch to the list, and I can even incude it in my series after and together with Aleksandar's patch, if you don't object.

I submitted it the usual way, so it is on the mailing list now.  If
you want to include it with your patch series that is fine.
https://lists.nongnu.org/archive/html/qemu-devel/2019-06/msg04087.html

Jim

Hi Laurent,

> s/arhitecture/architecture/

Done.

> You should define sys_statx() using _syscall5() macro and use it.

Done.

> ret != -TARGET_ENOSYS

Done.

> You already checked above p is not NULL and exited with -TARGET_EFAULT.

Done.

> BTW, do we really need to emulate the syscall if it is not available?
>
> I think the user-space application calling statx() should be ready to
> receive ENOSYS and define some kinds of fallback (like you do below). So
> it should not be done by QEMU.

nanoMIPS linux port doesn't support any of "stats" but the statx, so there
is no fallback in nanoMIPS user-space applications.

I think, we can expect similar situation for any new linux port.

> Why do we divide the case in two parts, fstatat() should work here too.

fstat() uses file descriptor, but here we have string which represents
file name with absolute path.

All system calls from 'stat' group whose name starts with letter f require
file descriptor as an argument. Whereas remaining system calls require
file name / path as string. In that sense, statx() is a hybrid between
the two, hence the solution I propose.

Aleksandar Rikalo

Le 19/06/2019 à 14:12, Aleksandar Rikalo a écrit :
> Hi Laurent,
...
>> BTW, do we really need to emulate the syscall if it is not available?
>>
>> I think the user-space application calling statx() should be ready to
>> receive ENOSYS and define some kinds of fallback (like you do below). So
>> it should not be done by QEMU.
> 
> nanoMIPS linux port doesn't support any of "stats" but the statx, so there
> is no fallback in nanoMIPS user-space applications.
> 
> I think, we can expect similar situation for any new linux port.

OK, I understand, so I agree, we need the emulation part.

>> Why do we divide the case in two parts, fstatat() should work here too.
> 
> fstat() uses file descriptor, but here we have string which represents
> file name with absolute path.
> 
> All system calls from 'stat' group whose name starts with letter f require
> file descriptor as an argument. Whereas remaining system calls require
> file name / path as string. In that sense, statx() is a hybrid between
> the two, hence the solution I propose.

but fstatat() works like statx(), it accepts file descriptor and path.

So what I proposed is to replace:

+            } else {
+                if (dirfd == AT_FDCWD) {
+                    /*
+                     * By pathname relative to the current working directory
+                     */
+                    ret = get_errno(stat(path(p), &st));
+                    unlock_user(p, arg2, 0);
+                } else {
+                    /*
+                     * By pathname relative to the directory referred to by
+                     * the file descriptor 'dirfd'
+                     */
+                    ret = get_errno(fstatat(dirfd, path(p), &st, flags));
+                    unlock_user(p, arg2, 0);
+                }
+            }

by something like;

+            } else {
+                ret = get_errno(fstatat(dirfd, path(p), &st, flags));
+            }

as fstatat() also accepts AT_FDCWD.

Moreover in kernel vfs_fstatat() calls vfs_statx():

static inline int vfs_fstatat(int dfd, const char __user *filename,
                              struct kstat *stat, int flags)
{
        return vfs_statx(dfd, filename, flags | AT_NO_AUTOMOUNT,
                         stat, STATX_BASIC_STATS);
}

so maybe all the cases can be emulated by fstatat()?

Or did I miss something?

Thanks,
Laurent