Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 8 Feb 2019 10:46:35 +0100 (CET)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Dmitry Safonov <dima@arista.com>
cc:     linux-kernel@vger.kernel.org, Andrei Vagin <avagin@openvz.org>,
        Adrian Reber <adrian@lisas.de>,
        Andrei Vagin <avagin@gmail.com>,
        Andy Lutomirski <luto@kernel.org>,
        Andy Tucker <agtucker@google.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Cyrill Gorcunov <gorcunov@openvz.org>,
        Dmitry Safonov <0x7f454c46@gmail.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>,
        Jeff Dike <jdike@addtoit.com>, Oleg Nesterov <oleg@redhat.com>,
        Pavel Emelyanov <xemul@virtuozzo.com>,
        Shuah Khan <shuah@kernel.org>,
        containers@lists.linux-foundation.org, criu@openvz.org,
        linux-api@vger.kernel.org, x86@kernel.org
Subject: Re: [PATCH 03/32] timens: Introduce CLOCK_MONOTONIC offsets
In-Reply-To: <alpine.DEB.2.21.1902072225420.1645@nanos.tec.linutronix.de>
Message-ID: <alpine.DEB.2.21.1902080859460.1645@nanos.tec.linutronix.de>
References: <20190206001107.16488-1-dima@arista.com> <20190206001107.16488-4-dima@arista.com> <alpine.DEB.2.21.1902072225420.1645@nanos.tec.linutronix.de>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 7 Feb 2019, Thomas Gleixner wrote:
> Does this really need to be an out of line call? If you stick this into the
> clock_get() implementations then it boils down to:
> 
> static inline void timens_add_monotonic(struct timespec64 *ts)
> {
> 	struct timens_offsets *ns_offsets = current->nsproxy->time_ns->offsets;
> 
> 	if (ns_offsets)
> 		*ts = timespec64_add(*ts, ns_offsets->monotonic_time_offset);

And this needs to be a special variant of
timespec64-add_safe(). timespec64_add_safe() is not sufficient, because it
assumes that both values are positive, which is not the case here..

In timer_set() implementations you move the timespec_valid() check after
the add. That's wrong because you really want to check the input value from
user space.

Assume that the caller supplied value is valid and the adjustment brings it
out of range then how should the caller understand why it it rejected?

So timespec64_add_namespace() must check for under and overflow. But doing
this with timespecs is a pain. I rather suggest to rework the whole thing
so hrtimer_nanosleep() takes a ktime_t expiry value and move the conversion
to the call sites. Then the whole offset magic becomes:

   expires = timespec64_to_ktime(rqtp);

   if (abstime)
   	expires = timens_to_host_mono(expires);

and that function can nicely do the underflow and overflow detection and
cap the values to 0 on underflow and KTIME_MAX on overflow.

Hmm?

Thanks,

	tglx