Re: [PATCH v9] y2038: Introduce the __ASSUME_TIME64_SYSCALLS define

[Rich Felker <dalias at libc dot org>]

On Wed, Sep 04, 2019 at 02:34:45PM +0200, Florian Weimer wrote:
> * Rich Felker:
> 
> > On Fri, Aug 30, 2019 at 01:19:27PM -0400, Zack Weinberg wrote:
> >> On Fri, Aug 30, 2019 at 1:09 PM Rich Felker <dalias@libc.org> wrote:
> >> > On Fri, Aug 30, 2019 at 01:03:16PM -0400, Zack Weinberg wrote:
> >> > > On Fri, Aug 30, 2019 at 12:37 PM Joseph Myers <joseph@codesourcery.com> wrote:
> >> > > > On Fri, 30 Aug 2019, Rich Felker wrote:
> >> > > > > To clarify, none of the timespec ones "exactly match" -- the suffixed
> >> > > > > syscalls on 32-bit require filling the padding around tv_nsec, whereas
> >> > > >
> >> > > > What do you mean by "require filling the padding"?  I thought the
> >> > > > conclusion in the kernel was that it dealt with zeroing the padding
> >> > > ....
> >> > >
> >> > > The kernel should always explicitly clear all of the padding in any
> >> > > structure it writes to user space, anything else risks leaking kernel
> >> > > data (e.g. the compiler decides it can use a 64-bit load and store to
> >> > > copy from the kernel's struct timespec to the user space struct
> >> > > timespec because those high bits are don't-care in the destination
> >> > > .... oops, the previous allocation had a kernel pointer in that 64-bit
> >> > > slot and we just exposed the KASLR base).
> >> >
> >> > The issue is the other direction, when timespecs are passed to the
> >> > kernel (usually for read-only access), e.g. providing a timeout.
> >> 
> >> Sorry, I misunderstood which direction you were talking about.
> >> 
> >> I think ignoring the padding bits has to be the kernel's
> >> responsibility, because the C library can't assume that these pointers
> >> refer to writable storage, nor can it assume that it is safe (or
> >> acceptably efficient) to copy the data structure.
> >
> > You don't patch them up. You copy them. E.g. instead of passing ts,
> > pass ts?(struct timespec[]){*ts}:0.
> 
> I don't see how this clears the padding.  I don't think we can support
> any concise syntax for that:
> 
> If we give the padding a name, this will not work on big-endian targets
> 
>   ts?(struct timespec[]){ts->tv_sec, ts->tv_nsec}:0
> 
> because the padding is in the middle, and ts->tv_nsec is copied into the
> padding.

While it may be desirable for compatibility with wrong code for this
sort of initializer to work, there is no specification on the order or
lack of other named members in timespec. Correct initialization has to
use designated initializers. I tried to sidestep the problem by using
*ts, but indeed I botched that; it would not zero the padding.

Actually what I did on musl was just use (long long[]){s,ns} as the
argument to the (time64) syscall, which decouples the userspace
definition of the type from the kernel interface and makes it so we
can use unnamed padding. (And of course no padding at all would work
with this, but we want to match ABI and be able to directly use
timespecs written to userspace by the kernel.)

> If we do not give the padding a name, the expression leaves the padding
> uninitialized (C11: “Unnamed members of structure objects have
> indeterminate value even after initialization.”).
> 
> And who should do this?  I think for pselect, glibc has to do it, but
> what about ioctls and socket options?

These need translation logic anyway or post-time64 software won't run
on pre-time64 kernels. See

https://git.musl-libc.org/cgit/musl/commit/?id=51fd67fcbfa598e2fe1885b517451b84c0bfe3b7

So it's not a big deal to also zero the padding if needed. Ideally
though the kernel should handle this right, especially for new
ioctls/sockopts added post-time64 where there's no need for fallback
translation.

Rich