Re: [Qemu-devel] [PATCH v2] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels

[Arnd Bergmann]

On Mon, Jun 17, 2019 at 3:11 PM Daniel P. Berrangé <address@hidden> wrote:
>
> The SIOCGSTAMP symbol was previously defined in the
> asm-generic/sockios.h header file. QEMU sees that header
> indirectly via sys/socket.h
>
> In linux kernel commit 0768e17073dc527ccd18ed5f96ce85f9985e9115
> the asm-generic/sockios.h header no longer defines SIOCGSTAMP.
> Instead it provides only SIOCGSTAMP_OLD, which only uses a
> 32-bit time_t on 32-bit architectures.

This is a bit misleading, as we still define SIOCGSTAMP in the
right place. asm-generic/sockios.h should not be used by normal
user space.

> The linux/sockios.h header then defines SIOCGSTAMP using
> either SIOCGSTAMP_OLD or SIOCGSTAMP_NEW as appropriate. If
> SIOCGSTAMP_NEW is used, then the tv_sec field is 64-bit even
> on 32-bit architectures
>
> To cope with this we must now define two separate syscalls,
> with corresponding old and new sizes, as well as including
> the new linux/sockios.h header.

The overall concept seems right. A few more comments on
details that may have gone wrong here. I'm not familiar with
the qemu-user implementation, so it's mostly guesswork
on my end.

>    IOCTL(SIOCGIWNAME, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_char_ifreq)))
>    IOCTL(SIOCSPGRP, IOC_W, MK_PTR(TYPE_INT)) /* pid_t */
>    IOCTL(SIOCGPGRP, IOC_R, MK_PTR(TYPE_INT)) /* pid_t */
> +
> +#ifdef SIOCGSTAMP_OLD
> +  IOCTL(SIOCGSTAMP_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval)))
> +#else
>    IOCTL(SIOCGSTAMP, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval)))
> +#endif
> +#ifdef SIOCGSTAMPNS_OLD
> +  IOCTL(SIOCGSTAMPNS_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec)))
> +#else
>    IOCTL(SIOCGSTAMPNS, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec)))
> +#endif
> +#ifdef SIOCGSTAMP_NEW
> +  IOCTL(SIOCGSTAMP_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval64)))
> +#endif
> +#ifdef SIOCGSTAMPNS_NEW
> +  IOCTL(SIOCGSTAMPNS_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec64)))
> +#endif

Is timespec64 a qemu type? How is it defined?

> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 7f141f699c..7830b600e7 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -750,6 +750,11 @@ struct target_pollfd {
>
>  #define TARGET_SIOCGSTAMP      0x8906          /* Get stamp (timeval) */
>  #define TARGET_SIOCGSTAMPNS    0x8907          /* Get stamp (timespec) */
> +#define TARGET_SIOCGSTAMP_OLD   0x8906          /* Get stamp (timeval) */
> +#define TARGET_SIOCGSTAMPNS_OLD 0x8907          /* Get stamp (timespec) */

Note that these types are architecture specific. It seems that only
one architecture is left that has its own definitions though, so this
is only broken on arch/sh for current linux (and remains broken).

Future architectures, including 32-bit risc-v should only have the _NEW
version and not support SIOCGSTAMP_OLD at all.

When emulating risc-v user space on old kernels (pre-5.1), you may need to
translate the ioctl command and all system calls that take a 64-bit time_t into
the variants with a 32-bit time_t on the way into the kernel, and then back.

Similarly, running an old user binary on a riscv32 machine, you may
need to do the reverse translation.

> +#define TARGET_SIOCGSTAMP_NEW   TARGET_IOC(TARGET_IOC_READ, 's', 6, 
> sizeof(long long) + sizeof(long)) /* Get stamp (timeval64) */
> +#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOC(TARGET_IOC_READ, 's', 7, 
> sizeof(long long) + sizeof(long)) /* Get stamp (timespec64) */

"sizeof(long long) + sizeof(long)" is not always the size of the argument to
TARGET_SIOCGSTAMP{NS}_NEW. On 32-bit architectures, the size is
two 64-bit values. sparc64 is potentially another special case, as 'struct
timeval is 'long + int' there (12 bytes).

On big-endian architectures, the nanoseconds are returned in the last
four bytes of the 16-byte structure.

>  /* Networking ioctls */
>  #define TARGET_SIOCADDRT       0x890B          /* add routing table entry */
> diff --git a/linux-user/syscall_types.h b/linux-user/syscall_types.h
> index b98a23b0f1..de4c5a5b6f 100644
> --- a/linux-user/syscall_types.h
> +++ b/linux-user/syscall_types.h
> @@ -20,6 +20,10 @@ STRUCT(timeval,
>  STRUCT(timespec,
>         MK_ARRAY(TYPE_LONG, 2))
>
> +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG)
> +
> +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG)

Same here.

        Arnd