[v2,2/2] rusage: allow 64-bit times ru_utime/ru_stime

As part of the y2038 system call work, the getrusage/wait4/waitid system
calls came up, since they pass time data to user space in 'timeval'
format as required by POSIX. This means the existing kernel data structure
is no longer compatible with the one from user space once we have a C
library exposing 64-bit time_t, requiring the library to convert the
kernel structure into its own structure.

This patch moves that conversion into the kernel itself, providing a
set of system calls that can directly be used to implement the libc
getrusage/wait4/waitid functions as we have traditionally done.

There are two advantages to this:

- The new path becomes the native case, avoiding the conversion
  overhead for future 32-bit C libraries.  At least glibc will still
  have to implement a conversion logic as a fallback in order to run
  new applications on older kernels, but that does not have to be used
  on new kernels.
- The range for the ru_utime/ru_stime is no longer limited to
  a 31-bit second counter (about 68 years). That limit may theoretically
  be hit on large SMP systems with a single process running for an
  extended time, e.g. 256 concurrent threads running for more than
  97 days.  Note that there is no overflow in 2038, as all the times
  are relative to the start of a process.

The downside of this is obviously the added complexity of having three
additional system call entry points plus their respective compat handlers,
and updated syscall tables on each architecture (not included in this
patch). Overall, I think this is *not* worth it, but I feel it's important
to show how it can be done and what the cost is.

There are probably some minor improvements that can be implemented on top,
as well as bugs that I introduce.  When reviewing this patch, let's for
now focus instead on the question whether we want it at all or not.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 arch/alpha/kernel/osf_sys.c   |   2 +-
 include/linux/compat.h        |  26 ++++++++-
 include/linux/resource.h      |   4 +-
 include/linux/sched/task.h    |   4 +-
 include/linux/syscalls.h      |   8 +++
 include/uapi/linux/resource.h |  29 ++++++++++
 kernel/compat.c               |  30 ++++++++++-
 kernel/exit.c                 | 120 ++++++++++++++++++++++++++++++++++++++----
 kernel/sys.c                  |  74 +++++++++++++++++++++++---
 9 files changed, 275 insertions(+), 22 deletions(-)

-- 
2.9.0

(belated reply)

* Arnd Bergmann <arnd@arndb.de> wrote:

> +int put_compat_rusage_time64(const struct __kernel_rusage *r,

> +			     struct compat_rusage_time64 __user *ru)

> +{

> +	struct compat_rusage_time64 r32;

> +	memset(&r32, 0, sizeof(r32));

> +	r32.ru_utime.tv_sec = r->ru_utime.tv_sec;

> +	r32.ru_utime.tv_usec = r->ru_utime.tv_usec;

> +	r32.ru_stime.tv_sec = r->ru_stime.tv_sec;

> +	r32.ru_stime.tv_usec = r->ru_stime.tv_usec;

> +	r32.ru_maxrss = r->ru_maxrss;

> +	r32.ru_ixrss = r->ru_ixrss;

> +	r32.ru_idrss = r->ru_idrss;

> +	r32.ru_isrss = r->ru_isrss;

> +	r32.ru_minflt = r->ru_minflt;

> +	r32.ru_majflt = r->ru_majflt;

> +	r32.ru_nswap = r->ru_nswap;

> +	r32.ru_inblock = r->ru_inblock;

> +	r32.ru_oublock = r->ru_oublock;

> +	r32.ru_msgsnd = r->ru_msgsnd;

> +	r32.ru_msgrcv = r->ru_msgrcv;

> +	r32.ru_nsignals = r->ru_nsignals;

> +	r32.ru_nvcsw = r->ru_nvcsw;

> +	r32.ru_nivcsw = r->ru_nivcsw;


Could you please vertically align the right side of the initialization as well? 
Much easier to check at a glance.

> +	user_access_begin();

> +	unsafe_put_user(signo, &infop->si_signo, Efault);

> +	unsafe_put_user(0, &infop->si_errno, Efault);

> +	unsafe_put_user(info.cause, &infop->si_code, Efault);

> +	unsafe_put_user(info.pid, &infop->si_pid, Efault);

> +	unsafe_put_user(info.uid, &infop->si_uid, Efault);

> +	unsafe_put_user(info.status, &infop->si_status, Efault);

> +	user_access_end();


This too would look nicer the following way:

> +	user_access_begin();

> +	unsafe_put_user(signo,		&infop->si_signo,	Efault);

> +	unsafe_put_user(0,		&infop->si_errno,	Efault);

> +	unsafe_put_user(info.cause,	&infop->si_code,	Efault);

> +	unsafe_put_user(info.pid,	&infop->si_pid,		Efault);

> +	unsafe_put_user(info.uid,	&infop->si_uid,		Efault);

> +	unsafe_put_user(info.status,	&infop->si_status,	Efault);

> +	user_access_end();


Which tabulated form made me notice the info.cause / si_code asymmetry - and a 
brief check of the source shows that it's correct. No way would I have noticed it 
in the jumbled up form above, so I think aligning such mass-initializations makes 
sense.

> +	memset(&r, 0, sizeof(r));

> +	r.ru_utime.tv_sec = rk->ru_utime.tv_sec;

> +	r.ru_utime.tv_usec = rk->ru_utime.tv_usec;

> +	r.ru_stime.tv_sec = rk->ru_stime.tv_sec;

> +	r.ru_stime.tv_usec = rk->ru_stime.tv_usec;

> +	r.ru_maxrss = rk->ru_maxrss;

> +	r.ru_ixrss = rk->ru_ixrss;

> +	r.ru_idrss = rk->ru_idrss;

> +	r.ru_isrss = rk->ru_isrss;

> +	r.ru_minflt = rk->ru_minflt;

> +	r.ru_majflt = rk->ru_majflt;

> +	r.ru_nswap = rk->ru_nswap;

> +	r.ru_inblock = rk->ru_inblock;

> +	r.ru_oublock = rk->ru_oublock;

> +	r.ru_msgsnd = rk->ru_msgsnd;

> +	r.ru_msgrcv = rk->ru_msgrcv;

> +	r.ru_nsignals = rk->ru_nsignals;

> +	r.ru_nvcsw = rk->ru_nvcsw;

> +	r.ru_nivcsw = rk->ru_nivcsw;


Ditto.

Thanks,

	Ingo

On Thu, Jun 21, 2018 at 5:49 PM, Ingo Molnar <mingo@kernel.org> wrote:
> * Arnd Bergmann <arnd@arndb.de> wrote:

>

>> +int put_compat_rusage_time64(const struct __kernel_rusage *r,

>> +                          struct compat_rusage_time64 __user *ru)

>> +{

>> +     struct compat_rusage_time64 r32;

>> +     memset(&r32, 0, sizeof(r32));

>> +     r32.ru_utime.tv_sec = r->ru_utime.tv_sec;

>> +     r32.ru_utime.tv_usec = r->ru_utime.tv_usec;

>> +     r32.ru_stime.tv_sec = r->ru_stime.tv_sec;

>> +     r32.ru_stime.tv_usec = r->ru_stime.tv_usec;

>> +     r32.ru_maxrss = r->ru_maxrss;

>> +     r32.ru_ixrss = r->ru_ixrss;

>> +     r32.ru_idrss = r->ru_idrss;

>> +     r32.ru_isrss = r->ru_isrss;

>> +     r32.ru_minflt = r->ru_minflt;

>> +     r32.ru_majflt = r->ru_majflt;

>> +     r32.ru_nswap = r->ru_nswap;

>> +     r32.ru_inblock = r->ru_inblock;

>> +     r32.ru_oublock = r->ru_oublock;

>> +     r32.ru_msgsnd = r->ru_msgsnd;

>> +     r32.ru_msgrcv = r->ru_msgrcv;

>> +     r32.ru_nsignals = r->ru_nsignals;

>> +     r32.ru_nvcsw = r->ru_nvcsw;

>> +     r32.ru_nivcsw = r->ru_nivcsw;

>

> Could you please vertically align the right side of the initialization as well?

> Much easier to check at a glance.

...

> Which tabulated form made me notice the info.cause / si_code asymmetry - and a

> brief check of the source shows that it's correct. No way would I have noticed it

> in the jumbled up form above, so I think aligning such mass-initializations makes

> sense.


Sure, no problem. Do you have an opinion on the question I raised in the
first patch [1], i.e. whether we actually want this to be done this way in the
kernel, or one of the other approaches I described there?

Thanks for taking a look here already!

      Arnd

[1] https://patchwork.kernel.org/patch/10352507/

* Arnd Bergmann <arnd@arndb.de> wrote:

> Sure, no problem. Do you have an opinion on the question I raised in the

> first patch [1], i.e. whether we actually want this to be done this way in the

> kernel, or one of the other approaches I described there?


So this looks like the most forward looking variant:

> a) deprecate the wait4() and getrusage() system calls, and create

>    a set of kernel interfaces based around a newly defined structure that

>    could solve multiple problems at once, e.g. provide more fine-grained

>    timestamps. The C library could then implement the posix interfaces

>    on top of the new system calls.


... but given the pretty long propagation time of new ABIs, is this a good 
solution? What would the limitations/trade-offs be on old-ABI systems?

Thanks,

	Ingo

On Thu, Jun 21, 2018 at 6:11 PM, Ingo Molnar <mingo@kernel.org> wrote:
>

> * Arnd Bergmann <arnd@arndb.de> wrote:

>

>> Sure, no problem. Do you have an opinion on the question I raised in the

>> first patch [1], i.e. whether we actually want this to be done this way in the

>> kernel, or one of the other approaches I described there?

>

> So this looks like the most forward looking variant:

>

>> a) deprecate the wait4() and getrusage() system calls, and create

>>    a set of kernel interfaces based around a newly defined structure that

>>    could solve multiple problems at once, e.g. provide more fine-grained

>>    timestamps. The C library could then implement the posix interfaces

>>    on top of the new system calls.

>

> ... but given the pretty long propagation time of new ABIs, is this a good

> solution? What would the limitations/trade-offs be on old-ABI systems?


The main purpose of this is to enable consistent 64-bit time_t interfaces in
user space, and for most users this would not change anything as the
existing glibc (both 64-bit and 32-bit) can continue calling the same
interfaces as before.

For those users that are interested in 64-bit time_t on 32-bit binaries,
the first step would be to change the glibc implementation to emulate
the existing interfaces with the new time_t on top of the new syscall
rather than the old (unsafe) syscall. Those users already require both
a new kernel and a new glibc version.

If the new kernel interfaces offer a real benefit, others could start
using them directly as soon as they have updated the libc version.
Note that glibc has not updated their kernel headers version for
a long time, they still allow building with linux-3.2 header files.
However, the glibc maintainers have indicated that they would
probably update that requirement to the then-latest version
when adding support to the new 64-bit time_t syscalls, so this
would become available immediately to all users of the new glibc
version.

However, the other question that has to be asked then is whether
there is anything wrong with wait4()/waitid() and getrusuage() that
we want to change beyond the time value passing. We have
answered a similar question with 'yes' for stat(), which has led
to the introduction of statx(), but so far I expect all other syscalls
to start compatible. See [1] for my current list. In that list, I have
currently marked waitid() and getrusuage() as not to be addressed,
i.e. my approach c) of applying only the first of the two patches but
not the second.

       Arnd

[1] https://docs.google.com/spreadsheets/d/1HCYwHXxs48TsTb6IGUduNjQnmfRvMPzCN6T_0YiQwis/edit#gid=0

* Arnd Bergmann <arnd@arndb.de> wrote:

> However, the other question that has to be asked then is whether

> there is anything wrong with wait4()/waitid() and getrusuage() that

> we want to change beyond the time value passing. We have

> answered a similar question with 'yes' for stat(), which has led

> to the introduction of statx(),


So we are thinking about adding wait5() in essence, right?

One thing we might want to look into whether the wait4() and waitid() ABIs could 
be 'merged', by making wait4() essentially a natural special case of waitid().

This would mean that the only new system call we'd have to add is waitid2() in 
essence, which would solve both the rusage layout problem and would offer a 
unified ABI.

If that makes sense (it might not!!), then I'd also modernize waitid2() by making 
it attribute structure based, have a length field and make the ABI extensible from 
now on going forward without having to introduce a new syscall variant every time 
we come up with something new...

I.e. how the perf syscall does ABI extensions: we've had dozens of ABI extensions, 
some of them pretty complex, and not a single time did we have to modify glibc and 
tooling was able to adapt quickly yet in a both backwards and forwards compatible 
fashion.

Another, simpler example is the new sys_sched_setattr() syscall, that too is using 
the perf_copy_attr() ABI method, via sched_copy_attr(). (With a minor 
compatibility quirk of SCHED_ATTR_SIZE_VER0 that a new wait ABI wouldn't have to 
do - i.e. it could be made even simpler.)

This way we only have:

SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr, unsigned int, flags)

But even 'pid' and 'flags' could have been part of the attribute, i.e. one we pick 
up an attribute structure from user-space we can have really low argument count 
system calls. This also concentrates all the compat concerns into handling the 
attribute structure properly - no weird per-arch artifacts and quirks with 4-5-6 
system call arguments.

Thanks,

	Ingo

Ingo Molnar <mingo@kernel.org> writes:

> * Arnd Bergmann <arnd@arndb.de> wrote:

>

>> However, the other question that has to be asked then is whether

>> there is anything wrong with wait4()/waitid() and getrusuage() that

>> we want to change beyond the time value passing. We have

>> answered a similar question with 'yes' for stat(), which has led

>> to the introduction of statx(),

>

> So we are thinking about adding wait5() in essence, right?


> One thing we might want to look into whether the wait4() and waitid() ABIs could 

> be 'merged', by making wait4() essentially a natural special case of

> waitid().


Essentially waitid(2) not waitid(3) has already seen this merger.
In that there is nothing to wait for that you can not already
expression with waitid.  status vs siginfo is a little different
but the information is encoded in both.

And waitid(2) optionally returns a struct rusage.

> This would mean that the only new system call we'd have to add is waitid2() in 

> essence, which would solve both the rusage layout problem and would offer a 

> unified ABI.

>

> If that makes sense (it might not!!), then I'd also modernize waitid2() by making 

> it attribute structure based, have a length field and make the ABI extensible from 

> now on going forward without having to introduce a new syscall variant every time 

> we come up with something new...


The only part where something is not parameterized in waitid is with the
return of rusage.

What to wait for takes an explicit type parameter.
What is being returned in siginfo returns an si_code to describe how
to decode it.

If it weren't for the zombie being gone after waitid returns I don't
think it would make any sense to combine getrusage and waitid together
at all.

> I.e. how the perf syscall does ABI extensions: we've had dozens of ABI extensions, 

> some of them pretty complex, and not a single time did we have to modify glibc and 

> tooling was able to adapt quickly yet in a both backwards and forwards compatible 

> fashion.

>

> Another, simpler example is the new sys_sched_setattr() syscall, that too is using 

> the perf_copy_attr() ABI method, via sched_copy_attr(). (With a minor 

> compatibility quirk of SCHED_ATTR_SIZE_VER0 that a new wait ABI wouldn't have to 

> do - i.e. it could be made even simpler.)

>

> This way we only have:

>

> SYSCALL_DEFINE3(sched_setattr, pid_t, pid, struct sched_attr __user *, uattr, unsigned int, flags)

>

> But even 'pid' and 'flags' could have been part of the attribute, i.e. one we pick 

> up an attribute structure from user-space we can have really low argument count 

> system calls. This also concentrates all the compat concerns into handling the 

> attribute structure properly - no weird per-arch artifacts and quirks with 4-5-6 

> system call arguments.


The trouble with attributes is that means you can't filter your system
call arguments with seccomp.  Which most of the time is a pretty big
downside.


From what I have seen the only truly interesting case for extending
waitid is something file descriptor based so the parent/child
relationship is not necessary to wait for a process to terminate.

As for getrusage.  If a sane union of the rusage limits and cgroups or
something like cgroups could be devised.  That would be ideal.  Of
course except for the memory cgroups the similarity to the resource
usage measurments and limits really isn't there.  So I don't know if
merging them would be a real possibility.

So I suspect the simplest thing to do would be to set a flag in the
idtype member of waitid that says give me rusage64 and then we would
be done.  Alternately we could use the low bits of the resource usage
pointer.  Assuming we don't want to introduce another syscall that is.
I really don't see much incremental extensibility potential in the wait
or rusage interface right now.

Eric

* Eric W. Biederman <ebiederm@xmission.com> wrote:

> The trouble with attributes is that means you can't filter your system

> call arguments with seccomp. [...]


There's nothing keeping seccomp from securely fetching those arguments and 
extending filtering to them as well ...

Allowing that would make sense for a lot of other system calls as well.

Thanks,

	Ingo

Ingo Molnar <mingo@kernel.org> writes:

> * Eric W. Biederman <ebiederm@xmission.com> wrote:

>

>> The trouble with attributes is that means you can't filter your system

>> call arguments with seccomp. [...]

>

> There's nothing keeping seccomp from securely fetching those arguments and 

> extending filtering to them as well ...

>

> Allowing that would make sense for a lot of other system calls as

> well.


Possibly.  The challenge is that if the fetch for the kernel to use
those arguments is different from the fetch of seccomp to test those
arguments you have a time of test vs time of use race.

Given the location of the seccomp hook at the kernel user space border
there is no easy way for seccomp to share the fetch with the system
call itself.

So I don't see how seccomp could perform the fetch securely.

Eric

* Eric W. Biederman <ebiederm@xmission.com> wrote:

> Ingo Molnar <mingo@kernel.org> writes:

> 

> > * Eric W. Biederman <ebiederm@xmission.com> wrote:

> >

> >> The trouble with attributes is that means you can't filter your system

> >> call arguments with seccomp. [...]

> >

> > There's nothing keeping seccomp from securely fetching those arguments and 

> > extending filtering to them as well ...

> >

> > Allowing that would make sense for a lot of other system calls as

> > well.

> 

> Possibly.  The challenge is that if the fetch for the kernel to use

> those arguments is different from the fetch of seccomp to test those

> arguments you have a time of test vs time of use race.


Those fetched values should obviously then be used to call permitted system calls.

> Given the location of the seccomp hook at the kernel user space border

> there is no easy way for seccomp to share the fetch with the system

> call itself.

> 

> So I don't see how seccomp could perform the fetch securely.


Looks like more of a seccomp mis-design/mis-implementation than some fundamental 
problem.

Mis-designed security features should not hinder system call design.

Thanks,

	Ingo

On Fri, Jun 22, 2018 at 7:45 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Ingo Molnar <mingo@kernel.org> writes:

>

> So I suspect the simplest thing to do would be to set a flag in the

> idtype member of waitid that says give me rusage64 and then we would

> be done.


It would have to be a flag in both the 'idtype' field of waitid(), and also
'who' field of getrusage(), which unfortunately uses a separate set of
flags. Not hard to do, but still a bit more complexity.

> Alternately we could use the low bits of the resource usage

> pointer.  Assuming we don't want to introduce another syscall that is.

> I really don't see much incremental extensibility potential in the wait

> or rusage interface right now.


This is also my conclusion after looking at how various other
operating systems implement getrusage() and wait4() today.
It seems that this is one of the most stable APIs, everyone uses
exactly the same structure layout (Linux/x32 being one exception,
they have the 64-bit Linux compatible layout using __s64 instead of
long members).

For the other ~20 system calls we introduce for y2038, the general
idea has been to stay mostly compatible with the source level interface,
just using a new syscall number. statx() is a notable exception here,
with clock_adjtime() and getitimer()/setitimer() still being undecided.

If we don't do an extensible layout or any other new fields, there
are still the open questions about whether any types should change:

- changing everything to 64-bit would allow sharing the kernel
  code between compat and native
- changing only __old_kernel_timeval to new 64-bit timeval would
  be the simplest user space change (only the syscall number
  changes with sizeof(time_t)), avoiding an extra copy thorough
  the user space stack.
- changing timeval to (64-bit) timespec would seem the most
  sensible update, since it avoids the silly nanosecond-to-
  microsecond conversion in the kernel (glibc would still need
  to do it for compatibility). This is what I'm considering for
  getitimer/setitimer,  too.

         Arnd

Ingo Molnar <mingo@kernel.org> writes:

> * Eric W. Biederman <ebiederm@xmission.com> wrote:

>

>> Ingo Molnar <mingo@kernel.org> writes:

>> 

>> > * Eric W. Biederman <ebiederm@xmission.com> wrote:

>> >

>> >> The trouble with attributes is that means you can't filter your system

>> >> call arguments with seccomp. [...]

>> >

>> > There's nothing keeping seccomp from securely fetching those arguments and 

>> > extending filtering to them as well ...

>> >

>> > Allowing that would make sense for a lot of other system calls as

>> > well.

>> 

>> Possibly.  The challenge is that if the fetch for the kernel to use

>> those arguments is different from the fetch of seccomp to test those

>> arguments you have a time of test vs time of use race.

>

> Those fetched values should obviously then be used to call permitted

> system calls.


Agreed.  To my knowledge no one has figured out how to make that work
yet.  For the most part it has been unnecessary.

>> Given the location of the seccomp hook at the kernel user space border

>> there is no easy way for seccomp to share the fetch with the system

>> call itself.

>> 

>> So I don't see how seccomp could perform the fetch securely.

>

> Looks like more of a seccomp mis-design/mis-implementation than some fundamental 

> problem.


Frankly.  Given that there are some very good solutions in other
operating systems, I think the misdesign is in unix/linux not providing
a good answer to what to do when you need more than 6 arguments to a
system call.

> Mis-designed security features should not hinder system call design.


I certainly agree that seccomp should not be the sole reason for not
doing something.  However there are lots of reasons to avoid
extensibility in general.

Excess extensibility has been the cause of more than one security issue.

Lots of flexibility comes at the price of lots of conditional execution
which tends to explode the test matrix of possibilities to test, with
the result that some combinations are never thought about or tested
because they don't make sense to combine.  Then someone with
mischievious intent see that combination and thinks what happens when I
do this.

Further that conditional execution can frequently be the cause of slow
code as well.

So while there are many nice features of tagged values.  I don't think
they are a general solution.  The lack of seccomp support (today) is
just one downside among many.


I do think it would be nice to have a general pattern for those
system calls that require extensibility.  My gut feel says something
like the L4 pseudo registers (to give a maxium request size)
combined with something like netlink encoding would make a very
nice template for making fast and flexible system calls.

Eric