linux-kernel - Re: [PATCH 2/4] clone3: allow creation of time namespace with offset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200318121317.2vyfyqj223sx5ybq@wittgenstein>
Date:   Wed, 18 Mar 2020 13:13:17 +0100
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Adrian Reber <areber@...hat.com>
Cc:     Eric Biederman <ebiederm@...ssion.com>,
        Pavel Emelyanov <ovzxemul@...il.com>,
        Oleg Nesterov <oleg@...hat.com>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Andrei Vagin <avagin@...il.com>, linux-kernel@...r.kernel.org,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Radostin Stoyanov <rstoyanov1@...il.com>,
        Michael Kerrisk <mtk.manpages@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Cyrill Gorcunov <gorcunov@...nvz.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 2/4] clone3: allow creation of time namespace with offset

On Tue, Mar 17, 2020 at 09:30:42AM +0100, Adrian Reber wrote:
> This extends clone3() to support the time namespace via CLONE_NEWTIME.
> In addition to creating a new process in a new time namespace this
> allows setting the clock offset in the newly created time namspace.
> 
> The time namespace allows to set an offset for two clocks.
> CLOCK_MONOTONIC and CLOCK_BOOTTIME.
> 
> This clone3() extension also offers setting both offsets through the
> newly introduced clone_args members timens_offset and
> timens_offset_size.
> 
> timens_offset:      Pointer to an array of clock offsets for the
>                     newly created process in a time namespaces.
>                     This requires that a new time namespace has been
>                     requested via CLONE_NEWTIME. It is only possible
>                     to set an offset for CLOCK_MONOTONIC and
>                     CLOCK_BOOTTIME. The array can therefore never
>                     have more than two elements.
>                     clone3() expects the array to contain the
>                     following struct:
>                     struct set_timens_offset {
>                             int clockid;
>                             struct timespec val;
>                     };
> 
> timens_offset_size: This defines the size of the array referenced
>                     in timens_offset. Currently this is limited
>                     to two elements.
> 
> To create a new process using clone3() in a new time namespace with
> clock offsets, something like this can be used:
> 
>   struct set_timens_offset timens_offset[2];
> 
>   timens_offset[0].clockid = CLOCK_BOOTTIME;
>   timens_offset[0].val.tv_sec = -1000;
>   timens_offset[0].val.tv_nsec = 42;
>   timens_offset[1].clockid = CLOCK_MONOTONIC;
>   timens_offset[1].val.tv_sec = 1000000;
>   timens_offset[1].val.tv_nsec = 37;
> 
>   struct _clone_args args = {
>     .flags = CLONE_NEWTIME,
>     .timens_offset = ptr_to_u64(timens_offset),
>     .timens_offset_size = 2;
>   };

In all honesty, this would be a terrible API and I think we need to come
up with something better than this. I don't want to pass down an array
of structs and in general would like to avoid this array + size pattern.
That pattern kinda made sense for the pid array because of pid
namespaces being nested but not for this case, I think. Also, why
require the additional clockid argument here? That makes sense for
clock_settime() and clock_gettime() but here we could just do:

struct timens {
	struct timespec clock_bootime;
	struct timespec clock_monotonic;
};

no? And since you need to expose that struct in a header somewhere
anyway you can version it by size just like clone_args. So the kernel
can apply the same pattern to be backwards compatible that we have with
struct clone_args and for openat2()'s struct open_how via
copy_struct_from_user. Then you only need one additional pointer in
struct clone_args.

What do we think?

Christian