Re: [RFC PATCH] linux-user: time64: consolidate rt_sigtimedwait_time64 and rt_sigtimedwait

[Arnd Bergmann]

On Fri, Dec 16, 2022, at 19:08, Michael Tokarev wrote:
> Both system calls are exactly the same, the only difference is the
> (optional) timespec conversion. Consolidate the two, and check which
> syscall being emulated in the timespec conversion place.
>
> This is just a PoC. But this brings at least 2 questions.
>
> Let's take pselect6 as an example. There are 2 possible pselects
> in there (actually more, but let's focus on just the two):
> TARGET_NR_pselect6 and TARGET_NR_pselect6_time64.  Both are implemented
> in a single do_pselect6() function, which, depending on its "time64"
> argument, will call either target_to_host_timespec64() or
> target_to_host_timespec().
>
> But these functions, in turn, are guarded by a lot of #if
> defined(foo). In particular, in context of pselect6,
> target_to_host_timespec() is guarded by
>  if defined(TARGET_NR_pselect6),
> while target_to_host_timespec64() is guarded by
>  if defined(TARGET_NR_pselect6_time64).
>
> So basically, if just one of the two TARGET_NR_pselect6*
> is defined, one of target_to_host_timespec*() will not
> be defined, but do_pselect6() calls *both* anyway.  In
> other words, both functions must be provided if any of
> the select6 are to be implemented.
>
> But the good news is that these functions
> (target_to_host_timespec*()) are always defined because
> they're needed for some syscalls anyway, like, eg,
> TARGET_NR_semop or TARGET_NR_utimensat or CONFIG_TIMERFD.
>
> It looks like whole gigantic ifdeffery around these two
> functions should be dropped, and a common function provided,
> with an extra argument like time64, to be used in many
> places where this construct is already used. Like in
> this PoC too, which again calls one of the two depending
> on the actual syscall being used.  Or maybe we can even
> combine the two into one with an extra arg like "time64".

The cleanup you suggest here looks correct to me, but as I
mentioned on IRC, I think this should only be done if the
time64 host side bug is also fixed. At the moment, both
the time32 and time64 target syscalls are always translated
into the time32 variant on 32-bit hosts, which is a mistake
for multiple reasons:

- the libc-defined 'timespec' argument that is passed into
  the kernel-defined syscall has the wrong layout, resulting
  in incorrect data. This could be avoided by using
  the kernel-defined __kernel_old_timespec instead, but then

- time64 target arguments needlessly get truncated to
  the time32 range. This happens e.g. when running 64-bit
  target on 32-bit host, but could be easily avoided
  by converting into the time64 types (__kernel_timespec)
  and calling the time64 syscalls whenever those are
  available.

- On modern hosts, only the time64 syscalls are available, so
  e.g. on riscv32 hosts, the current implementation does not
  even build.

The best solution for all of the above I think would be to
have a wrapper for each of the affected syscalls that calls
the time64 interface first and falls back to the time32
version only if that fails, using __kernel_timespec
on the interface instead of libc timespec.

A simpler but less flexible method would be to detect
sizeof(time_t) at compile time and then use either the
time32 or time64 host syscall numbers, whichever goes
with the libc time_t/timespec.


      Arnd