Message-ID: <CAK8P3a2WQGzVd3p-Ps9RaFNasSKiNm9pZEb8hxZt=42QXNH_mQ@mail.gmail.com>
Date:   Thu, 9 Nov 2017 13:54:09 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Kees Cook <keescook@...omium.org>
Cc:     Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>,
        John Stultz <john.stultz@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Ingo Molnar <mingo@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pstore: use ktime_get_real_fast_ns() instead of __getnstimeofday()

On Thu, Nov 9, 2017 at 1:55 AM, Kees Cook <keescook@...omium.org> wrote:
> On Wed, Nov 8, 2017 at 8:00 AM, Arnd Bergmann <arnd@...db.de> wrote:
>> I noticed that __getnstimeofday() is a rather odd interface, with
>> a number of quirks:
>>
>> - The caller may come from NMI context, but the implementation is not NMI safe
>> - The calling conventions are different from any other timekeeping functions
>> - The naming doesn't fit into the 'ktime_get_*()' scheme
>>
>> This addresses the above issues by using a completely different
>> method to get the time: ktime_get_real_fast_ns() is NMI safe and
>> doesn't print a warning when called with the timekeeping suspended,
>> and we can easily transform the result into a timespec structure. Since
>> ktime_get_real_fast_ns() was not exported to modules, I also add the
>> export.
>>
>> The behavior for the suspended case changes, we now return the time
>> if we can find it out rather than setting it to zero. We could still
>> change that but this would require exporting the 'timekeeping_suspended'
>> to modules.
>>
>> I'm not trying to address y2038-safety at this point, but plan to
>> do that later with a more complex patch.
>>
>> Cc: Kees Cook <keescook@...omium.org>
>> Signed-off-by: Arnd Bergmann <arnd@...db.de>
>> ---
>> It would be helpful to get this merged into v4.15-rc1, since I have
>> some other cleanups that depend on this. Please have a look to see if
>> this makes sense to you.
>
> As long as this is sane to call when timekeeping is not running, I'm
> happy to take the patch.

Maybe John or Thomas can comment on this, I'm not totally sure how
reliable it is. My best guess is that it will still produce correct time stamps
a lot of the time, but it depends a bit on the clocksource hardware and
how long the timekeeping has been suspended. As far as I can tell,

- if the clocksource register contents wrap around, the reported time
  might appear to go back to the time of the last timer interrupt. The
  shortest time I could find for an overflow is a 16-bit timer running at
  32khz on i.MX1, overflowing every two seconds.
- if reading a suspended clocksource returns zero (or another incorrect
  value) the time might be in the far future
- if reading a suspended clocksource causes a hang or crash, you
  lose.

the latter two could probably be considered bugs in the respective
clocksource drivers, and it's possible that this doesn't actually happen
on any of them, but we could in theory work around it by not
reading the a suspended clocksource at all and returning the
last known time from the interrupt.

I suppose we could even make ktime_get_*_fast_ns() always
return zero when the timekeeping is suspended, but I don't
know if that would hurt the other callers.

> Would you prefer I take this via pstore or do
> you have another tree it should go through?
>
> Acked-by: Kees Cook <keescook@...omium.org>

The pstore tree is probably best, but going through -tip would work
as well I think.

     Arnd

Powered by blists - more mailing lists