kdb: use __ktime_get_real_seconds instead of __current_kernel_time

kdb is the only user of the __current_kernel_time() interface, which is
not y2038 safe and should be removed at some point.

The kdb code also goes to great lengths to print the time in a
human-readable format from 'struct timespec', again using a non-y2038-safe
re-implementation of the generic time_to_tm() code.

Using __current_kernel_time() here is necessary since the regular
accessors that require a sequence lock might hang when called during the
xtime update. However, this is safe in the particular case since kdb is
only interested in the tv_sec field that is updated atomically.

In order to make this y2038-safe, I'm converting the code to the generic
time64_to_tm helper, but that introduces the problem that we have no
interface like __current_kernel_time() that provides a 64-bit timestamp
in a lockless, safe and architecture-independent way. I have multiple
ideas for how to solve that:

- __ktime_get_real_seconds() is lockless, but can return
  incorrect results on 32-bit architectures in the special case that
  we are in the process of changing the time across the epoch, either
  during the timer tick that overflows the seconds in 2038, or while
  calling settimeofday.

- ktime_get_real_fast_ns() would work in this context, but does
  require a call into the clocksource driver to return a high-resolution
  timestamp. This may have undesired side-effects in the debugger,
  since we want to limit the interactions with the rest of the kernel.

- Adding a ktime_get_real_fast_seconds() based on tk_fast_mono
  plus tkr->base_real without the tk_clock_read() delta. Not sure about
  the value of adding yet another interface here.

- Changing the existing ktime_get_real_seconds() to use
  tk_fast_mono on 32-bit architectures rather than xtime_sec.  I think
  this could work, but am not entirely sure if this is an improvement.

I picked the first of those for simplicity here. It's technically
not correct but probably good enough as the time is only used for the
debugging output and the race will likely never be hit in practice.
Another downside is having to move the declaration into a public header
file.

Let me know if anyone has a different preference.

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Link: https://patchwork.kernel.org/patch/9775309/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 include/linux/timekeeping.h        |  1 +
 kernel/debug/kdb/kdb_main.c        | 45 +++++---------------------------------
 kernel/time/timekeeping_internal.h |  2 --
 3 files changed, 6 insertions(+), 42 deletions(-)

-- 
2.9.0

On Thu, 12 Oct 2017 16:06:11 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> kdb is the only user of the __current_kernel_time() interface, which is

> not y2038 safe and should be removed at some point.

> 

> The kdb code also goes to great lengths to print the time in a

> human-readable format from 'struct timespec', again using a non-y2038-safe

> re-implementation of the generic time_to_tm() code.


Is it really necessary for the kdb `summary' command to print the
time/date?  Which puppies would die if we just removed it all?

On 12/10/17 23:40, Andrew Morton wrote:
> On Thu, 12 Oct 2017 16:06:11 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> 

>> kdb is the only user of the __current_kernel_time() interface, which is

>> not y2038 safe and should be removed at some point.

>>

>> The kdb code also goes to great lengths to print the time in a

>> human-readable format from 'struct timespec', again using a non-y2038-safe

>> re-implementation of the generic time_to_tm() code.

> 

> Is it really necessary for the kdb `summary' command to print the

> time/date?  Which puppies would die if we just removed it all?


kdb may enter spontaneously (BUG(), etc) so it can be useful if one 
returns from an overnight test run to know how long things survived.

It would almost certainly be possible for a skilled user to reconstruct 
the time of death. Having said that, one of the things you can do with 
kdb (although I admit *I* have never done it) is leave a macro command 
in the hands of an unskilled user.

Short summary: no puppies would die, but perhaps some might go hungry 
for a little while when their owner is late home?


Daniel.

On 10/13/2017 03:26 AM, Daniel Thompson wrote:
> On 12/10/17 23:40, Andrew Morton wrote:

>> On Thu, 12 Oct 2017 16:06:11 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

>>

>>> kdb is the only user of the __current_kernel_time() interface, which is

>>> not y2038 safe and should be removed at some point.

>>>

>>> The kdb code also goes to great lengths to print the time in a

>>> human-readable format from 'struct timespec', again using a non-y2038-safe

>>> re-implementation of the generic time_to_tm() code.

>>

>> Is it really necessary for the kdb `summary' command to print the

>> time/date?  Which puppies would die if we just removed it all?

> 

> kdb may enter spontaneously (BUG(), etc) so it can be useful if one

> returns from an overnight test run to know how long things survived.

> 

> It would almost certainly be possible for a skilled user to reconstruct

> the time of death. Having said that, one of the things you can do with

> kdb (although I admit *I* have never done it) is leave a macro command

> in the hands of an unskilled user.

> 

> Short summary: no puppies would die, but perhaps some might go hungry

> for a little while when their owner is late home?

> 


Daniel is correct.  This is information that was just a nice to have for postmortem analysis it can also be called via gdb macros.

If kdb is really the last remaining user, it seems like the interface should get removed and kdb can print time another way that is compatible with the 2038 fixes.

After having taken a quick look it would seem __ktime_get_real_seconds()  (because we need the non-lock protected version) and time64_to_tm() should be the proper replacement.  There is certainly no reason to duplicate code in kdb for the "summary" functions.

I am assuming no one has fixed this yet, so I should be able to provide a patch to the list along the lines of what is below.  And I will follow it with a second patch to remove the __current_kernel_time() to lkml and the timekeeper maintainer.

Cheers,
Jason.

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 09168c52ab64..2529cc470a45 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -53,6 +53,8 @@ extern void getrawmonotonic64(struct timespec64 *ts);
  extern void ktime_get_ts64(struct timespec64 *ts);
  extern time64_t ktime_get_seconds(void);
  extern time64_t ktime_get_real_seconds(void);
+/* does not take xtime_lock */
+extern time64_t __ktime_get_real_seconds(void);
  
  extern int __getnstimeofday64(struct timespec64 *tv);
  extern void getnstimeofday64(struct timespec64 *tv);
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 2a20c0dfdafc..c7a02710d884 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -2477,41 +2477,6 @@ static int kdb_kill(int argc, const char **argv)
  	return 0;
  }
  
-struct kdb_tm {
-	int tm_sec;	/* seconds */
-	int tm_min;	/* minutes */
-	int tm_hour;	/* hours */
-	int tm_mday;	/* day of the month */
-	int tm_mon;	/* month */
-	int tm_year;	/* year */
-};
-
-static void kdb_gmtime(struct timespec *tv, struct kdb_tm *tm)
-{
-	/* This will work from 1970-2099, 2100 is not a leap year */
-	static int mon_day[] = { 31, 29, 31, 30, 31, 30, 31,
-				 31, 30, 31, 30, 31 };
-	memset(tm, 0, sizeof(*tm));
-	tm->tm_sec  = tv->tv_sec % (24 * 60 * 60);
-	tm->tm_mday = tv->tv_sec / (24 * 60 * 60) +
-		(2 * 365 + 1); /* shift base from 1970 to 1968 */
-	tm->tm_min =  tm->tm_sec / 60 % 60;
-	tm->tm_hour = tm->tm_sec / 60 / 60;
-	tm->tm_sec =  tm->tm_sec % 60;
-	tm->tm_year = 68 + 4*(tm->tm_mday / (4*365+1));
-	tm->tm_mday %= (4*365+1);
-	mon_day[1] = 29;
-	while (tm->tm_mday >= mon_day[tm->tm_mon]) {
-		tm->tm_mday -= mon_day[tm->tm_mon];
-		if (++tm->tm_mon == 12) {
-			tm->tm_mon = 0;
-			++tm->tm_year;
-			mon_day[1] = 28;
-		}
-	}
-	++tm->tm_mday;
-}
-
  /*
   * Most of this code has been lifted from kernel/timer.c::sys_sysinfo().
   * I cannot call that code directly from kdb, it has an unconditional
@@ -2537,8 +2502,8 @@ static void kdb_sysinfo(struct sysinfo *val)
   */
  static int kdb_summary(int argc, const char **argv)
  {
-	struct timespec now;
-	struct kdb_tm tm;
+	time64_t now_seconds;
+	struct tm tm;
  	struct sysinfo val;
  
  	if (argc)
@@ -2552,9 +2517,9 @@ static int kdb_summary(int argc, const char **argv)
  	kdb_printf("domainname %s\n", init_uts_ns.name.domainname);
  	kdb_printf("ccversion  %s\n", __stringify(CCVERSION));
  
-	now = __current_kernel_time();
-	kdb_gmtime(&now, &tm);
-	kdb_printf("date       %04d-%02d-%02d %02d:%02d:%02d "
+	now_seconds = __ktime_get_real_seconds();
+	time64_to_tm(now_seconds, sys_tz.tz_minuteswest * 60, &tm);
+	kdb_printf("date       %04ld-%02d-%02d %02d:%02d:%02d "
  		   "tz_minuteswest %d\n",
  		1900+tm.tm_year, tm.tm_mon+1, tm.tm_mday,
  		tm.tm_hour, tm.tm_min, tm.tm_sec,

On 10/12/2017 09:06 AM, Arnd Bergmann wrote:
> kdb is the only user of the __current_kernel_time() interface, which is

> not y2038 safe and should be removed at some point.

> 

> The kdb code also goes to great lengths to print the time in a

> human-readable format from 'struct timespec', again using a non-y2038-safe

> re-implementation of the generic time_to_tm() code.

> 

> Using __current_kernel_time() here is necessary since the regular

> accessors that require a sequence lock might hang when called during the

> xtime update. However, this is safe in the particular case since kdb is

> only interested in the tv_sec field that is updated atomically.

> 

> In order to make this y2038-safe, I'm converting the code to the generic

> time64_to_tm helper, but that introduces the problem that we have no

> interface like __current_kernel_time() that provides a 64-bit timestamp

> in a lockless, safe and architecture-independent way. I have multiple

> ideas for how to solve that:

> 

> - __ktime_get_real_seconds() is lockless, but can return

>    incorrect results on 32-bit architectures in the special case that

>    we are in the process of changing the time across the epoch, either

>    during the timer tick that overflows the seconds in 2038, or while

>    calling settimeofday.

> 

> - ktime_get_real_fast_ns() would work in this context, but does

>    require a call into the clocksource driver to return a high-resolution

>    timestamp. This may have undesired side-effects in the debugger,

>    since we want to limit the interactions with the rest of the kernel.

> 

> - Adding a ktime_get_real_fast_seconds() based on tk_fast_mono

>    plus tkr->base_real without the tk_clock_read() delta. Not sure about

>    the value of adding yet another interface here.

> 

> - Changing the existing ktime_get_real_seconds() to use

>    tk_fast_mono on 32-bit architectures rather than xtime_sec.  I think

>    this could work, but am not entirely sure if this is an improvement.

> 

> I picked the first of those for simplicity here. It's technically

> not correct but probably good enough as the time is only used for the

> debugging output and the race will likely never be hit in practice.

> Another downside is having to move the declaration into a public header

> file.

> 

> Let me know if anyone has a different preference.



It all seems reasonable to me.  Separately I created the same patch because I didn't see this mail first.  The only difference was that I added a comment about the __ktime_get_real_seconds() not taking the lock because it was done that way in other places in the header file.

===
  extern time64_t ktime_get_real_seconds(void);
+/* does not take xtime_lock */
+extern time64_t __ktime_get_real_seconds(void);
===

Acked-by: Jason Wessel <jason.wessel@windriver.com>


Thanks for your work on the 2038 problems.  :-)

Cheers,
Jason.