Date:   Fri, 10 Nov 2017 00:43:55 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Kees Cook <keescook@...omium.org>,
        Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>,
        John Stultz <john.stultz@...aro.org>,
        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 Fri, Nov 10, 2017 at 12:00 AM, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Thu, 9 Nov 2017, Arnd Bergmann wrote:
>> 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
>
> Hmm, no. None of the regular accessor functions can be called from NMI
> context safely.

Right, that's what I mean: it must not get called from NMI context, but it
currently is, at least for this case:

NMI handler:
  something bad
    panic()
      kmsg_dump()
        pstore_dump()
           pstore_record_init()
             __getnstimeofday()

I should probably add that to the changelog text ;-)

>> >> - The calling conventions are different from any other timekeeping functions
>
> Yes, that was done back then to actually return the value (which is
> possible with TSC) and indicate at the same time that timekeeping is
> suspended.
>
>> >> - The naming doesn't fit into the 'ktime_get_*()' scheme
>
> Right, because it's not in the ktime_get_() family of functions.

For the background on how I got here: I'm currently trying to go through
all functions in include/linux/timekeeping32.h with the intention of
replacing them with appropriate ktime_get_*() interfaces that don't have
the y2038 overflow problem. I considered just using
the existing __getnstimeofday64() here, which would be a trivial
change, but then I noticed the NMI problem.

Also, I have a related patch series that renames getrawmonotonic64(),
current_kernel_time64() and get_monotonic_coarse64() to
ktime_get_raw_ts64(), ktime_get_coarse_real_ts64() and
ktime_get_coarse_ts64(), for consistency, but then I couldn't
come up with a good name for __getnstimeofday64(), as the
__ktime_get_*() naming is already used for a number of other
things and I did not want to overload that more. Completely
removing __getnstimeofday64() would be handier here.

>> > 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.
>
> None of those problems exist for the fast NMI safe accessors. In
> timekeeping_suspend() we store the current time and any access to the fast
> timekeeper returns exactly the time at suspend up to the point where
> timekeeping is resumed. See halt_fast_timekeeper().
>
> So all you get between timekeeping_suspend() and timekeeping_resume() is
> a stale timestamp. No backwards, no crash and burn.

Ah, perfect, then my patch should be fine.
I think I had read that part of the code before, but then forgotten about it.

> The normal timekeeping accessor functions cannot be called between
> timekeeping_suspend() and timekeeping_resume() at all. They will emit a
> warning and can indeed crash and burn in one of the ways you described
> above. This does not happen on x86 because the TSC will just work on
> systems with pstore.

Sure, except for __getnstimeofday64(), which will intentionally not warn but
could crash in the clocksource driver (on non-x86). We do ignore the result
from __getnstimeofday64() when timekeeping is suspended, but only after
we call into the clocksource driver.

       Arnd

Powered by blists - more mailing lists