[PATCH] [v2] pstore: use ktime_get_real_fast_ns() instead of __getnstimeofday()

[PATCH] [v2] pstore: use ktime_get_real_fast_ns() instead of __getnstimeofday()

[Arnd Bergmann]

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,
  one way to get there from NMI is

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

- The calling conventions are different from any other timekeeping functions,
  to deal with returning an error code during suspended timekeeping.
- 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
has a reasonable behavior when timekeeping is suspended: it returns
the time at which it got suspended.
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 slightly, we now return the
time if we can find it out rather than setting it to zero. As Thomas
Gleixner explained, this is still safe, as we won't attempt to
call into the clocksource driver that might be suspended

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@chromium.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: improved changelog
---
 fs/pstore/platform.c      | 5 +----
 kernel/time/timekeeping.c | 1 +
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index e3c1332dfb4c..423159abd501 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -482,10 +482,7 @@ void pstore_record_init(struct pstore_record *record,
 	record->psi = psinfo;
 
 	/* Report zeroed timestamp if called before timekeeping has resumed. */
-	if (__getnstimeofday(&record->time)) {
-		record->time.tv_sec = 0;
-		record->time.tv_nsec = 0;
-	}
+	record->time = ns_to_timespec(ktime_get_real_fast_ns());
 }
 
 /*
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index af139aa615ca..8f0d1857b78d 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -536,6 +536,7 @@ u64 ktime_get_coarse_real_fast_ns(void)
 {
 	return __ktime_get_real_fast_ns(&tk_fast_mono, true);
 }
+EXPORT_SYMBOL_GPL(ktime_get_real_fast_ns);
 
 /**
  * halt_fast_timekeeper - Prevent fast timekeeper from accessing clocksource.
-- 
2.9.0

Re: [PATCH] [v2] pstore: use ktime_get_real_fast_ns() instead of __getnstimeofday()

[Kees Cook]

On Fri, Nov 10, 2017 at 7:25 AM, Arnd Bergmann <arnd@arndb.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,
>   one way to get there from NMI is
>
>       NMI handler:
>         something bad
>           panic()
>             kmsg_dump()
>               pstore_dump()
>                  pstore_record_init()
>                    __getnstimeofday()
>
> - The calling conventions are different from any other timekeeping functions,
>   to deal with returning an error code during suspended timekeeping.
> - 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
> has a reasonable behavior when timekeeping is suspended: it returns
> the time at which it got suspended.
> 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 slightly, we now return the
> time if we can find it out rather than setting it to zero. As Thomas
> Gleixner explained, this is still safe, as we won't attempt to
> call into the clocksource driver that might be suspended
>
> 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@chromium.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> v2: improved changelog
> ---
>  fs/pstore/platform.c      | 5 +----
>  kernel/time/timekeeping.c | 1 +
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index e3c1332dfb4c..423159abd501 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -482,10 +482,7 @@ void pstore_record_init(struct pstore_record *record,
>         record->psi = psinfo;
>
>         /* Report zeroed timestamp if called before timekeeping has resumed. */
> -       if (__getnstimeofday(&record->time)) {
> -               record->time.tv_sec = 0;
> -               record->time.tv_nsec = 0;
> -       }
> +       record->time = ns_to_timespec(ktime_get_real_fast_ns());
>  }
>
>  /*
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index af139aa615ca..8f0d1857b78d 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -536,6 +536,7 @@ u64 ktime_get_coarse_real_fast_ns(void)
>  {
>         return __ktime_get_real_fast_ns(&tk_fast_mono, true);
>  }
> +EXPORT_SYMBOL_GPL(ktime_get_real_fast_ns);

I was going to add this to the pstore tree, but
ktime_get_real_fast_ns() doesn't exist. In which tree is
ktime_get_real_fast_ns() added? This should be carried there:

Acked-by: Kees Cook <keescook@chromium.org>

Thanks!

-Kees

>
>  /**
>   * halt_fast_timekeeper - Prevent fast timekeeper from accessing clocksource.
> --
> 2.9.0
>



-- 
Kees Cook
Pixel Security

Re: [PATCH] [v2] pstore: use ktime_get_real_fast_ns() instead of __getnstimeofday()

[Thomas Gleixner]

On Fri, 10 Nov 2017, Kees Cook wrote:
> > +EXPORT_SYMBOL_GPL(ktime_get_real_fast_ns);
> 
> I was going to add this to the pstore tree, but
> ktime_get_real_fast_ns() doesn't exist. In which tree is
> ktime_get_real_fast_ns() added? This should be carried there:
> 
> Acked-by: Kees Cook <keescook@chromium.org>

it's in tip timers/core

I pick it up.

Thanks,

	tglx

[tip:timers/core] pstore: Use ktime_get_real_fast_ns() instead of __getnstimeofday()

[tip-bot for Arnd Bergmann]

Commit-ID:  df27067e6040b51188184876253d93da002433aa
Gitweb:     https://git.kernel.org/tip/df27067e6040b51188184876253d93da002433aa
Author:     Arnd Bergmann <arnd@arndb.de>
AuthorDate: Fri, 10 Nov 2017 16:25:04 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 12 Nov 2017 15:05:52 +0100

pstore: Use ktime_get_real_fast_ns() instead of __getnstimeofday()

__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,
  one way to get there from NMI is

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

- The calling conventions are different from any other timekeeping functions,
  to deal with returning an error code during suspended timekeeping.

Address the above issues by using a completely different method to get the
time: ktime_get_real_fast_ns() is NMI safe and has a reasonable behavior
when timekeeping is suspended: it returns the time at which it got
suspended. As Thomas Gleixner explained, this is safe, as
ktime_get_real_fast_ns() does not call into the clocksource driver that
might be suspended.

The result can easily be transformed into a timespec structure. Since
ktime_get_real_fast_ns() was not exported to modules, add the export.

The pstore behavior for the suspended case changes slightly, as it now
stores the timestamp at which timekeeping was suspended instead of storing
a zero timestamp.

This change is not addressing y2038-safety, that's subject to a more
complex follow up patch.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Kees Cook <keescook@chromium.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Colin Cross <ccross@android.com>
Link: https://lkml.kernel.org/r/20171110152530.1926955-1-arnd@arndb.de

---
 fs/pstore/platform.c      | 5 +----
 kernel/time/timekeeping.c | 1 +
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index ec7199e..086e491 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -482,10 +482,7 @@ void pstore_record_init(struct pstore_record *record,
 	record->psi = psinfo;
 
 	/* Report zeroed timestamp if called before timekeeping has resumed. */
-	if (__getnstimeofday(&record->time)) {
-		record->time.tv_sec = 0;
-		record->time.tv_nsec = 0;
-	}
+	record->time = ns_to_timespec(ktime_get_real_fast_ns());
 }
 
 /*
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 353f7bd..198afa7 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -528,6 +528,7 @@ u64 ktime_get_real_fast_ns(void)
 {
 	return __ktime_get_real_fast_ns(&tk_fast_mono);
 }
+EXPORT_SYMBOL_GPL(ktime_get_real_fast_ns);
 
 /**
  * halt_fast_timekeeper - Prevent fast timekeeper from accessing clocksource.