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

[Arnd Bergmann]

On Fri, Jul 12, 2019 at 3:23 PM Laurent Vivier <address@hidden> wrote:
>
> Le 12/07/2019 à 14:47, Arnd Bergmann a écrit :
> > On Fri, Jul 12, 2019 at 2:17 PM Laurent Vivier <address@hidden> wrote:
> >>
> >> Le 11/07/2019 à 23:05, Arnd Bergmann a écrit :
> >>> On Thu, Jul 11, 2019 at 7:32 PM Laurent Vivier <address@hidden> wrote:
> >>>
> >>>>
> >>>> Notes:
> >>>>     v4: [lv] timeval64 and timespec64 are { long long , long }
> >>>
> >>>>
> >>>> +STRUCT(timeval64, TYPE_LONGLONG, TYPE_LONG)
> >>>> +
> >>>> +STRUCT(timespec64, TYPE_LONGLONG, TYPE_LONG)
> >>>> +
> >>>
> >>> This still doesn't look right, see my earlier comment about padding
> >>> on big-endian architectures.
> >>>
> >>> Note that the in-kernel 'timespec64' is different from the uapi
> >>> '__kernel_timespec' exported by the kernel. I also still think you may
> >>> need to convert between SIOCGSTAMP_NEW and SIOCGSTAMP_OLD,
> >>> e.g. when emulating a 32-bit riscv process (which only use
> >>> SIOCGSTAMP_NEW) on a kernel that only understands
> >>> SIOCGSTAMP_OLD.
> >>
> >> I agree.
> >> I'm preparing a patch always using SIOCGSTAMP and SIOCGSTAMPNS on the
> >> host (converting the structure when needed).
> >
> > That in turn would have the problem of breaking in 2038 when the
> > timestamp overflows.
>
> No, because SIOCGSTAMP and SIOCGSTAMPNS are aliased to the _NEW versions
> on system supporting them (yes, we need to rebuild the binary, but we have 19
> years to do that).
>
> #define SIOCGSTAMP      ((sizeof(struct timeval))  == 8 ? \
>                          SIOCGSTAMP_OLD   : SIOCGSTAMP_NEW)
> #define SIOCGSTAMPNS    ((sizeof(struct timespec)) == 8 ? \
>                          SIOCGSTAMPNS_OLD : SIOCGSTAMPNS_NEW)

Right, makes sense.

> >
> >> I've added the SH4 variant.> What is special about SH4?
>
> The definition of _OLD is different:
>
> #define SIOCGSTAMP_OLD  _IOR('s', 100, struct timeval) /* Get stamp (timeval) 
> */
> #define SIOCGSTAMPNS_OLD _IOR('s', 101, struct timespec) /* Get stamp 
> (timespec) */

Ah, that one.


> > No, you don't need to swap. The difference is only in the padding.
> > Since the kernel uses a 64/64 structure here, and user space
> > may have use 'long tv_nsec', you need to add the padding on
> > the correct side, like
> >
> > struct timeval64 {
> >    long long tv_sec;
> > #if 32bit && big-endian
> >    long :32; /* anonymous padding */
> > #endif
> >    suseconds_t tv_usec;
> > #if (32bit && little-endian) || sparc64
> >    long :32;
> > #endif
> > };
>
> We don't do memcopy() but we set each field one by one, so the padding doesn't
> seem needed if we define correctly the user structure:
>
> struct target_timeval64 {
>     abi_llong tv_sec;
>     abi_long tv_usec;
> };
>
> and do something like:
>
>     struct target_timeval64 *target_tv;
>     struct timeval *host_tv;
> ...
>     __put_user(host_tv->tv_sec, &target_tv->tv_sec);
>     __put_user(host_tv->tv_usec, &target_tv->tv_usec);
> ...

That still seems wrong. The user application has a definition
of 'timeval' that contains the padding, so your definition has
to match that.

       Arnd