Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1531746 > unrolled thread

[PATCH 2/7] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/7] timekeeping: Ignore the bogus sleep time if pm_trace is enabled John Stultz <john.stultz@linaro.org> - 2016-11-28 23:40 +0100
    Re: [PATCH 2/7] timekeeping: Ignore the bogus sleep time if pm_trace  is enabled Ingo Molnar <mingo@kernel.org> - 2016-11-29 08:30 +0100
      Re: [PATCH 2/7] timekeeping: Ignore the bogus sleep time if pm_trace  is enabled Chen Yu <yu.c.chen@intel.com> - 2016-11-29 10:30 +0100
    [tip:timers/core] timekeeping: Ignore the bogus sleep time if  pm_trace is enabled tip-bot for Chen Yu <tipbot@zytor.com> - 2016-11-29 18:20 +0100

#1531746 — [PATCH 2/7] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

From: Chen Yu <yu.c.chen@intel.com>

Previously we encountered some memory overflow issues due to
the bogus sleep time brought by inconsistent rtc, which is
triggered when pm_trace is enabled, and we have fixed it
in recent kernel. However it's improper in the first place
to call __timekeeping_inject_sleeptime() in case that pm_trace
is enabled simply because that "hash" time value will wreckage
the timekeeping subsystem.

This patch is originally written by Thomas, which would bypass
the bogus rtc interval when pm_trace is enabled.
Meanwhile, if system succeed to resume back with pm_trace set, the
users are warned to adjust the bogus rtc either by 'ntpdate' or
'rdate', by resetting pm_trace_rtc_abused to false, otherwise above
tools might not work as expected.

Originally-from: Thomas Gleixner <tglx@linutronix.de>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Xunlei Pang <xlpang@redhat.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Len Brown <lenb@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
[jstultz: Fixed up pr_warn formating, and implemented suggestions
 from Ingo]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2: Added suggestions from Ingo
---
 arch/x86/kernel/rtc.c       |  9 +++++++++
 drivers/base/power/trace.c  | 26 ++++++++++++++++++++++++++
 drivers/rtc/rtc-cmos.c      |  7 +++++++
 include/linux/mc146818rtc.h |  1 +
 include/linux/pm-trace.h    |  9 ++++++++-
 5 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 79c6311c..3282a88 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -64,6 +64,15 @@ void mach_get_cmos_time(struct timespec *now)
 	unsigned int status, year, mon, day, hour, min, sec, century = 0;
 	unsigned long flags;
 
+	/*
+	 * If pm_trace abused the RTC as storage set the timespec to 0
+	 * which tells the caller that this RTC value is bogus.
+	 */
+	if (!pm_trace_rtc_valid()) {
+		now->tv_sec = now->tv_nsec = 0;
+		return;
+	}
+
 	spin_lock_irqsave(&rtc_lock, flags);
 
 	/*
diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
index efec10b..15563c4 100644
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c
@@ -10,6 +10,7 @@
 #include <linux/pm-trace.h>
 #include <linux/export.h>
 #include <linux/rtc.h>
+#include <linux/suspend.h>
 
 #include <linux/mc146818rtc.h>
 
@@ -74,6 +75,9 @@
 
 #define DEVSEED (7919)
 
+bool pm_trace_rtc_abused __read_mostly;
+EXPORT_SYMBOL(pm_trace_rtc_abused);
+
 static unsigned int dev_hash_value;
 
 static int set_magic_time(unsigned int user, unsigned int file, unsigned int device)
@@ -104,6 +108,7 @@ static int set_magic_time(unsigned int user, unsigned int file, unsigned int dev
 	time.tm_min = (n % 20) * 3;
 	n /= 20;
 	mc146818_set_time(&time);
+	pm_trace_rtc_abused = true;
 	return n ? -1 : 0;
 }
 
@@ -238,10 +243,31 @@ int show_trace_dev_match(char *buf, size_t size)
 	device_pm_unlock();
 	return ret;
 }
+static int pm_trace_notify(struct notifier_block *nb,
+				unsigned long mode, void *_unused)
+{
+	switch (mode) {
+	case PM_POST_HIBERNATION:
+	case PM_POST_SUSPEND:
+		if (pm_trace_rtc_abused) {
+			pm_trace_rtc_abused = false;
+			pr_warn("Possible incorrect RTC due to pm_trace, please use 'ntpdate' or 'rdate' to reset.\n");
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static struct notifier_block pm_trace_nb = {
+	.notifier_call = pm_trace_notify,
+};
 
 static int early_resume_init(void)
 {
 	hash_value_early_read = read_magic_time();
+	register_pm_notifier(&pm_trace_nb);
 	return 0;
 }
 
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index dd3d598..9cf06b7 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -191,6 +191,13 @@ static inline void cmos_write_bank2(unsigned char val, unsigned char addr)
 
 static int cmos_read_time(struct device *dev, struct rtc_time *t)
 {
+	/*
+	 * If pm_trace abused the RTC for storage tell the caller that it is
+	 * unusable.
+	 */
+	if (!pm_trace_rtc_valid())
+		return -EIO;
+
 	/* REVISIT:  if the clock has a "century" register, use
 	 * that instead of the heuristic in mc146818_get_time().
 	 * That'll make Y3K compatility (year > 2070) easy!
diff --git a/include/linux/mc146818rtc.h b/include/linux/mc146818rtc.h
index a585b4b..0661af1 100644
--- a/include/linux/mc146818rtc.h
+++ b/include/linux/mc146818rtc.h
@@ -16,6 +16,7 @@
 #include <asm/mc146818rtc.h>		/* register access macros */
 #include <linux/bcd.h>
 #include <linux/delay.h>
+#include <linux/pm-trace.h>
 
 #ifdef __KERNEL__
 #include <linux/spinlock.h>		/* spinlock_t */
diff --git a/include/linux/pm-trace.h b/include/linux/pm-trace.h
index ecbde7a..7b78793 100644
--- a/include/linux/pm-trace.h
+++ b/include/linux/pm-trace.h
@@ -1,11 +1,17 @@
 #ifndef PM_TRACE_H
 #define PM_TRACE_H
 
+#include <linux/types.h>
 #ifdef CONFIG_PM_TRACE
 #include <asm/pm-trace.h>
-#include <linux/types.h>
 
 extern int pm_trace_enabled;
+extern bool pm_trace_rtc_abused;
+
+static inline bool pm_trace_rtc_valid(void)
+{
+	return !pm_trace_rtc_abused;
+}
 
 static inline int pm_trace_is_enabled(void)
 {
@@ -24,6 +30,7 @@ extern int show_trace_dev_match(char *buf, size_t size);
 
 #else
 
+static inline bool pm_trace_rtc_valid(void) { return true; }
 static inline int pm_trace_is_enabled(void) { return 0; }
 
 #define TRACE_DEVICE(dev) do { } while (0)
-- 
2.7.4

[toc] | [next] | [standalone]

#1531968 — Re: [PATCH 2/7] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

* John Stultz <john.stultz@linaro.org> wrote:

> From: Chen Yu <yu.c.chen@intel.com>
> 
> Previously we encountered some memory overflow issues due to
> the bogus sleep time brought by inconsistent rtc, which is
> triggered when pm_trace is enabled, and we have fixed it
> in recent kernel. However it's improper in the first place
> to call __timekeeping_inject_sleeptime() in case that pm_trace
> is enabled simply because that "hash" time value will wreckage
> the timekeeping subsystem.

s/

 Previously we encountered memory overflow issues due to
 bogus sleep time brought by an inconsistent RTC, which is
 triggered when pm_trace is enabled, and we have fixed it
 in recent kernels. However it's improper in the first place
 to call __timekeeping_inject_sleeptime() in case pm_trace
 is enabled simply because the "hash" time value will wreckage
 the timekeeping subsystem.

Half a dozen typos ...

> This patch is originally written by Thomas, which would bypass
> the bogus rtc interval when pm_trace is enabled.
> Meanwhile, if system succeed to resume back with pm_trace set, the
> users are warned to adjust the bogus rtc either by 'ntpdate' or
> 'rdate', by resetting pm_trace_rtc_abused to false, otherwise above
> tools might not work as expected.

s/

 This patch was originally written by Thomas, which would bypass
 the bogus RTC interval when pm_trace is enabled.
 Meanwhile, if the system succeeds to resume back with pm_trace set,
 users are warned to adjust the bogus RTC either by 'ntpdate' or
 'rdate', by resetting pm_trace_rtc_abused to false, otherwise above
 tools might not work as expected.

> +	/*
> +	 * If pm_trace abused the RTC as storage set the timespec to 0
> +	 * which tells the caller that this RTC value is bogus.
> +	 */

s/
	/*
	 * If pm_trace abused the RTC as storage, set the timespec to 0,
	 * which tells the caller that this RTC value is bogus.
	 */

> @@ -74,6 +75,9 @@
>  
>  #define DEVSEED (7919)
>  
> +bool pm_trace_rtc_abused __read_mostly;
> +EXPORT_SYMBOL(pm_trace_rtc_abused);

EXPORT_SYMBOL_GPL()

> +static int pm_trace_notify(struct notifier_block *nb,
> +				unsigned long mode, void *_unused)

Please no nonsensical linebreaks in the middle of an argument list.

> +{
> +	switch (mode) {
> +	case PM_POST_HIBERNATION:
> +	case PM_POST_SUSPEND:
> +		if (pm_trace_rtc_abused) {
> +			pm_trace_rtc_abused = false;
> +			pr_warn("Possible incorrect RTC due to pm_trace, please use 'ntpdate' or 'rdate' to reset.\n");


s/to reset./to reset it.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1532032 — Re: [PATCH 2/7] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

On Tue, Nov 29, 2016 at 08:19:55AM +0100, Ingo Molnar wrote:
> 
> * John Stultz <john.stultz@linaro.org> wrote:
> 
> > From: Chen Yu <yu.c.chen@intel.com>
> > 
> > Previously we encountered some memory overflow issues due to
> > the bogus sleep time brought by inconsistent rtc, which is
> > triggered when pm_trace is enabled, and we have fixed it
> > in recent kernel. However it's improper in the first place
> > to call __timekeeping_inject_sleeptime() in case that pm_trace
> > is enabled simply because that "hash" time value will wreckage
> > the timekeeping subsystem.
> 
> s/
> 
>  Previously we encountered memory overflow issues due to
>  bogus sleep time brought by an inconsistent RTC, which is
>  triggered when pm_trace is enabled, and we have fixed it
>  in recent kernels. However it's improper in the first place
>  to call __timekeeping_inject_sleeptime() in case pm_trace
>  is enabled simply because the "hash" time value will wreckage
>  the timekeeping subsystem.
> 
> Half a dozen typos ...
> 
> > This patch is originally written by Thomas, which would bypass
> > the bogus rtc interval when pm_trace is enabled.
> > Meanwhile, if system succeed to resume back with pm_trace set, the
> > users are warned to adjust the bogus rtc either by 'ntpdate' or
> > 'rdate', by resetting pm_trace_rtc_abused to false, otherwise above
> > tools might not work as expected.
> 
> s/
> 
>  This patch was originally written by Thomas, which would bypass
>  the bogus RTC interval when pm_trace is enabled.
>  Meanwhile, if the system succeeds to resume back with pm_trace set,
>  users are warned to adjust the bogus RTC either by 'ntpdate' or
>  'rdate', by resetting pm_trace_rtc_abused to false, otherwise above
>  tools might not work as expected.
> 
> > +	/*
> > +	 * If pm_trace abused the RTC as storage set the timespec to 0
> > +	 * which tells the caller that this RTC value is bogus.
> > +	 */
> 
> s/
> 	/*
> 	 * If pm_trace abused the RTC as storage, set the timespec to 0,
> 	 * which tells the caller that this RTC value is bogus.
> 	 */
> 
> > @@ -74,6 +75,9 @@
> >  
> >  #define DEVSEED (7919)
> >  
> > +bool pm_trace_rtc_abused __read_mostly;
> > +EXPORT_SYMBOL(pm_trace_rtc_abused);
> 
> EXPORT_SYMBOL_GPL()
> 
> > +static int pm_trace_notify(struct notifier_block *nb,
> > +				unsigned long mode, void *_unused)
> 
> Please no nonsensical linebreaks in the middle of an argument list.
> 
> > +{
> > +	switch (mode) {
> > +	case PM_POST_HIBERNATION:
> > +	case PM_POST_SUSPEND:
> > +		if (pm_trace_rtc_abused) {
> > +			pm_trace_rtc_abused = false;
> > +			pr_warn("Possible incorrect RTC due to pm_trace, please use 'ntpdate' or 'rdate' to reset.\n");
> 
> 
> s/to reset./to reset it.
> 
> Thanks,
> 
> 	Ingo
Thanks Ingo, I've sent out a new versin based on your comments.
Yu

[toc] | [prev] | [next] | [standalone]

#1532569 — [tip:timers/core] timekeeping: Ignore the bogus sleep time if pm_trace is enabled

Commit-ID:  ba58d1020a54933c6b087a3107661c8513556cb8
Gitweb:     http://git.kernel.org/tip/ba58d1020a54933c6b087a3107661c8513556cb8
Author:     Chen Yu <yu.c.chen@intel.com>
AuthorDate: Mon, 28 Nov 2016 14:35:19 -0800
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 29 Nov 2016 18:02:58 +0100

timekeeping: Ignore the bogus sleep time if pm_trace is enabled

Power management suspend/resume tracing (ab)uses the RTC to store
suspend/resume information persistently. As a consequence the RTC value is
clobbered when timekeeping is resumed and tries to inject the sleep time.

Commit a4f8f6667f09 ("timekeeping: Cap array access in timekeeping_debug")
plugged a out of bounds array access in the timekeeping debug code which
was caused by the clobbered RTC value, but we still use the clobbered RTC
value for sleep time injection into kernel timekeeping, which will result
in random adjustments depending on the stored "hash" value.

To prevent this keep track of the RTC clobbering and ignore the invalid RTC
timestamp at resume. If the system resumed successfully clear the flag,
which marks the RTC as unusable, warn the user about the RTC clobber and
recommend to adjust the RTC with 'ntpdate' or 'rdate'.

[jstultz: Fixed up pr_warn formating, and implemented suggestions from Ingo]
[ tglx: Rewrote changelog ]

Originally-from: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Richard Cochran <richardcochran@gmail.com>
Cc: Xunlei Pang <xlpang@redhat.com>
Cc: Len Brown <lenb@kernel.org>
Link: http://lkml.kernel.org/r/1480372524-15181-3-git-send-email-john.stultz@linaro.org
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/x86/kernel/rtc.c       |  9 +++++++++
 drivers/base/power/trace.c  | 27 +++++++++++++++++++++++++++
 drivers/rtc/rtc-cmos.c      |  7 +++++++
 include/linux/mc146818rtc.h |  1 +
 include/linux/pm-trace.h    |  9 ++++++++-
 5 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 79c6311c..5b21cb7 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -64,6 +64,15 @@ void mach_get_cmos_time(struct timespec *now)
 	unsigned int status, year, mon, day, hour, min, sec, century = 0;
 	unsigned long flags;
 
+	/*
+	 * If pm_trace abused the RTC as storage, set the timespec to 0,
+	 * which tells the caller that this RTC value is unusable.
+	 */
+	if (!pm_trace_rtc_valid()) {
+		now->tv_sec = now->tv_nsec = 0;
+		return;
+	}
+
 	spin_lock_irqsave(&rtc_lock, flags);
 
 	/*
diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
index efec10b..1cda505 100644
--- a/drivers/base/power/trace.c
+++ b/drivers/base/power/trace.c
@@ -10,6 +10,7 @@
 #include <linux/pm-trace.h>
 #include <linux/export.h>
 #include <linux/rtc.h>
+#include <linux/suspend.h>
 
 #include <linux/mc146818rtc.h>
 
@@ -74,6 +75,9 @@
 
 #define DEVSEED (7919)
 
+bool pm_trace_rtc_abused __read_mostly;
+EXPORT_SYMBOL_GPL(pm_trace_rtc_abused);
+
 static unsigned int dev_hash_value;
 
 static int set_magic_time(unsigned int user, unsigned int file, unsigned int device)
@@ -104,6 +108,7 @@ static int set_magic_time(unsigned int user, unsigned int file, unsigned int dev
 	time.tm_min = (n % 20) * 3;
 	n /= 20;
 	mc146818_set_time(&time);
+	pm_trace_rtc_abused = true;
 	return n ? -1 : 0;
 }
 
@@ -239,9 +244,31 @@ int show_trace_dev_match(char *buf, size_t size)
 	return ret;
 }
 
+static int
+pm_trace_notify(struct notifier_block *nb, unsigned long mode, void *_unused)
+{
+	switch (mode) {
+	case PM_POST_HIBERNATION:
+	case PM_POST_SUSPEND:
+		if (pm_trace_rtc_abused) {
+			pm_trace_rtc_abused = false;
+			pr_warn("Possible incorrect RTC due to pm_trace, please use 'ntpdate' or 'rdate' to reset it.\n");
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static struct notifier_block pm_trace_nb = {
+	.notifier_call = pm_trace_notify,
+};
+
 static int early_resume_init(void)
 {
 	hash_value_early_read = read_magic_time();
+	register_pm_notifier(&pm_trace_nb);
 	return 0;
 }
 
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index dd3d598..b27a189 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -191,6 +191,13 @@ static inline void cmos_write_bank2(unsigned char val, unsigned char addr)
 
 static int cmos_read_time(struct device *dev, struct rtc_time *t)
 {
+	/*
+	 * If pm_trace abused the RTC for storage, set the timespec to 0,
+	 * which tells the caller that this RTC value is unusable.
+	 */
+	if (!pm_trace_rtc_valid())
+		return -EIO;
+
 	/* REVISIT:  if the clock has a "century" register, use
 	 * that instead of the heuristic in mc146818_get_time().
 	 * That'll make Y3K compatility (year > 2070) easy!
diff --git a/include/linux/mc146818rtc.h b/include/linux/mc146818rtc.h
index a585b4b..0661af1 100644
--- a/include/linux/mc146818rtc.h
+++ b/include/linux/mc146818rtc.h
@@ -16,6 +16,7 @@
 #include <asm/mc146818rtc.h>		/* register access macros */
 #include <linux/bcd.h>
 #include <linux/delay.h>
+#include <linux/pm-trace.h>
 
 #ifdef __KERNEL__
 #include <linux/spinlock.h>		/* spinlock_t */
diff --git a/include/linux/pm-trace.h b/include/linux/pm-trace.h
index ecbde7a..7b78793 100644
--- a/include/linux/pm-trace.h
+++ b/include/linux/pm-trace.h
@@ -1,11 +1,17 @@
 #ifndef PM_TRACE_H
 #define PM_TRACE_H
 
+#include <linux/types.h>
 #ifdef CONFIG_PM_TRACE
 #include <asm/pm-trace.h>
-#include <linux/types.h>
 
 extern int pm_trace_enabled;
+extern bool pm_trace_rtc_abused;
+
+static inline bool pm_trace_rtc_valid(void)
+{
+	return !pm_trace_rtc_abused;
+}
 
 static inline int pm_trace_is_enabled(void)
 {
@@ -24,6 +30,7 @@ extern int show_trace_dev_match(char *buf, size_t size);
 
 #else
 
+static inline bool pm_trace_rtc_valid(void) { return true; }
 static inline int pm_trace_is_enabled(void) { return 0; }
 
 #define TRACE_DEVICE(dev) do { } while (0)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web