Skip to contentSkip to conversation

time: create __getnstimeofday for WARNless calls

PATCH In review John Stultz 4 participants 4 messages Dec 17, 2012 – Dec 20, 2012

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 time: create __getnstimeofday for WARNless calls

From: Kees Cook <[email protected]>

Hey Thomas,
Wanted to see if maybe there was still time for this for 3.8?
thanks
-john

The pstore RAM backend can get called during resume, and must be defensive against a suspended time source. Expose getnstimeofday logic that returns an error instead of a WARN. This can be detected and the timestamp can be zeroed out.

---
 fs/pstore/ram.c           |   10 +++++++---
 include/linux/time.h      |    1 +
 kernel/time/timekeeping.c |   29 ++++++++++++++++++++++++-----
 3 files changed, 32 insertions(+), 8 deletions(-)
fs/pstore/ram.c +7 -3
index 1a4f6da..dacfe78 100644
@@ -168,12 +168,16 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
 static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz)
 {
 	char *hdr;
-	struct timeval timestamp;
+	struct timespec timestamp;
 	size_t len;
 
-	do_gettimeofday(&timestamp);
+	/* Report zeroed timestamp if called before timekeeping has resumed. */
+	if (__getnstimeofday(&timestamp)) {
+		timestamp.tv_sec = 0;
+		timestamp.tv_nsec = 0;
+	}
 	hdr = kasprintf(GFP_ATOMIC, RAMOOPS_KERNMSG_HDR "%lu.%lu\n",
-		(long)timestamp.tv_sec, (long)timestamp.tv_usec);
+		(long)timestamp.tv_sec, (long)(timestamp.tv_nsec / 1000));
 	WARN_ON_ONCE(!hdr);
 	len = hdr ? strlen(hdr) : 0;
 	persistent_ram_write(prz, hdr, len);
include/linux/time.h +1 -0
index 4d358e9..0015aea 100644
@@ -158,6 +158,7 @@ extern int do_setitimer(int which, struct itimerval *value,
 			struct itimerval *ovalue);
 extern unsigned int alarm_setitimer(unsigned int seconds);
 extern int do_getitimer(int which, struct itimerval *value);
+extern int __getnstimeofday(struct timespec *tv);
 extern void getnstimeofday(struct timespec *tv);
 extern void getrawmonotonic(struct timespec *ts);
 extern void getnstime_raw_and_real(struct timespec *ts_raw,
kernel/time/timekeeping.c +24 -5
index 4c7de02..dfc7f87 100644
@@ -214,19 +214,18 @@ static void timekeeping_forward_now(struct timekeeper *tk)
 }
 
 /**
- * getnstimeofday - Returns the time of day in a timespec
+ * __getnstimeofday - Returns the time of day in a timespec.
  * @ts:		pointer to the timespec to be set
  *
- * Returns the time of day in a timespec.
+ * Updates the time of day in the timespec.
+ * Returns 0 on success, or -ve when suspended (timespec will be undefined).
  */
-void getnstimeofday(struct timespec *ts)
+int __getnstimeofday(struct timespec *ts)
 {
 	struct timekeeper *tk = &timekeeper;
 	unsigned long seq;
 	s64 nsecs = 0;
 
-	WARN_ON(timekeeping_suspended);
-
 	do {
 		seq = read_seqbegin(&tk->lock);
 
@@ -237,6 +236,26 @@ void getnstimeofday(struct timespec *ts)
 
 	ts->tv_nsec = 0;
 	timespec_add_ns(ts, nsecs);
+
+	/*
+	 * Do not bail out early, in case there were callers still using
+	 * the value, even in the face of the WARN_ON.
+	 */
+	if (unlikely(timekeeping_suspended))
+		return -EAGAIN;
+	return 0;
+}
+EXPORT_SYMBOL(__getnstimeofday);
+
+/**
+ * getnstimeofday - Returns the time of day in a timespec.
+ * @ts:		pointer to the timespec to be set
+ *
+ * Returns the time of day in a timespec (WARN if suspended).
+ */
+void getnstimeofday(struct timespec *ts)
+{
+	WARN_ON(__getnstimeofday(ts));
 }
 EXPORT_SYMBOL(getnstimeofday);
 
Signature · 1 line
1.7.9.5
Acked-by on v1 1/1
#1 lore raw
Quoted 12 lines · John Stultz

From: Kees Cook <[email protected]>

Hey Thomas,
Wanted to see if maybe there was still time for this for 3.8?
thanks
-john

The pstore RAM backend can get called during resume, and must be defensive against a suspended time source. Expose getnstimeofday logic that returns an error instead of a WARN. This can be detected and the timestamp can be zeroed out.

Quoted 7 lines
---
 fs/pstore/ram.c           |   10 +++++++---

For pstore/ram part:

Thanks!

#2 lore raw
In reply to John Stultz
Quoted 11 lines · John Stultz

From: Kees Cook <[email protected]>

Hey Thomas,
Wanted to see if maybe there was still time for this for 3.8?
thanks
-john

The pstore RAM backend can get called during resume, and must be defensive against a suspended time source. Expose getnstimeofday logic that returns an error instead of a WARN. This can be detected and the timestamp can be zeroed out.

Shouldn't we zero out the time stamp in the core code ?

Thanks,

tglx

Quoted 106 lines
---
 fs/pstore/ram.c           |   10 +++++++---
 include/linux/time.h      |    1 +
 kernel/time/timekeeping.c |   29 ++++++++++++++++++++++++-----
 3 files changed, 32 insertions(+), 8 deletions(-)
fs/pstore/ram.c +7 -3
index 1a4f6da..dacfe78 100644
@@ -168,12 +168,16 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
 static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz)
 {
 	char *hdr;
-	struct timeval timestamp;
+	struct timespec timestamp;
 	size_t len;
 
-	do_gettimeofday(&timestamp);
+	/* Report zeroed timestamp if called before timekeeping has resumed. */
+	if (__getnstimeofday(&timestamp)) {
+		timestamp.tv_sec = 0;
+		timestamp.tv_nsec = 0;
+	}
 	hdr = kasprintf(GFP_ATOMIC, RAMOOPS_KERNMSG_HDR "%lu.%lu\n",
-		(long)timestamp.tv_sec, (long)timestamp.tv_usec);
+		(long)timestamp.tv_sec, (long)(timestamp.tv_nsec / 1000));
 	WARN_ON_ONCE(!hdr);
 	len = hdr ? strlen(hdr) : 0;
 	persistent_ram_write(prz, hdr, len);
include/linux/time.h +1 -0
index 4d358e9..0015aea 100644
@@ -158,6 +158,7 @@ extern int do_setitimer(int which, struct itimerval *value,
 			struct itimerval *ovalue);
 extern unsigned int alarm_setitimer(unsigned int seconds);
 extern int do_getitimer(int which, struct itimerval *value);
+extern int __getnstimeofday(struct timespec *tv);
 extern void getnstimeofday(struct timespec *tv);
 extern void getrawmonotonic(struct timespec *ts);
 extern void getnstime_raw_and_real(struct timespec *ts_raw,
kernel/time/timekeeping.c +24 -5
index 4c7de02..dfc7f87 100644
@@ -214,19 +214,18 @@ static void timekeeping_forward_now(struct timekeeper *tk)
 }
 
 /**
- * getnstimeofday - Returns the time of day in a timespec
+ * __getnstimeofday - Returns the time of day in a timespec.
  * @ts:		pointer to the timespec to be set
  *
- * Returns the time of day in a timespec.
+ * Updates the time of day in the timespec.
+ * Returns 0 on success, or -ve when suspended (timespec will be undefined).
  */
-void getnstimeofday(struct timespec *ts)
+int __getnstimeofday(struct timespec *ts)
 {
 	struct timekeeper *tk = &timekeeper;
 	unsigned long seq;
 	s64 nsecs = 0;
 
-	WARN_ON(timekeeping_suspended);
-
 	do {
 		seq = read_seqbegin(&tk->lock);
 
@@ -237,6 +236,26 @@ void getnstimeofday(struct timespec *ts)
 
 	ts->tv_nsec = 0;
 	timespec_add_ns(ts, nsecs);
+
+	/*
+	 * Do not bail out early, in case there were callers still using
+	 * the value, even in the face of the WARN_ON.
+	 */
+	if (unlikely(timekeeping_suspended))
+		return -EAGAIN;
+	return 0;
+}
+EXPORT_SYMBOL(__getnstimeofday);
+
+/**
+ * getnstimeofday - Returns the time of day in a timespec.
+ * @ts:		pointer to the timespec to be set
+ *
+ * Returns the time of day in a timespec (WARN if suspended).
+ */
+void getnstimeofday(struct timespec *ts)
+{
+	WARN_ON(__getnstimeofday(ts));
 }
 EXPORT_SYMBOL(getnstimeofday);
 
Signature · 1 line
1.7.9.5
#3 lore raw
Quoted 8 lines · Thomas Gleixner
Quoted 7 lines · John Stultz

From: Kees Cook <[email protected]>

Hey Thomas,
Wanted to see if maybe there was still time for this for 3.8?
thanks
-john

Quoted 6 lines
Quoted 4 lines

The pstore RAM backend can get called during resume, and must be defensive against a suspended time source. Expose getnstimeofday logic that returns an error instead of a WARN. This can be detected and the timestamp can be zeroed out.

Shouldn't we zero out the time stamp in the core code ?

It wasn't clear to me if the raw/wrong value should be available to a caller when they got the EAGAIN. Since pstore is the only known user of this, I'm fine moving the zeroing into the timekeeping core.

-Kees

Quoted 107 lines

Thanks,

tglx

Quoted 102 lines
---
 fs/pstore/ram.c           |   10 +++++++---
 include/linux/time.h      |    1 +
 kernel/time/timekeeping.c |   29 ++++++++++++++++++++++++-----
 3 files changed, 32 insertions(+), 8 deletions(-)
fs/pstore/ram.c +7 -3
index 1a4f6da..dacfe78 100644
@@ -168,12 +168,16 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
 static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz)
 {
      char *hdr;
-     struct timeval timestamp;
+     struct timespec timestamp;
      size_t len;
 
-     do_gettimeofday(&timestamp);
+     /* Report zeroed timestamp if called before timekeeping has resumed. */
+     if (__getnstimeofday(&timestamp)) {
+             timestamp.tv_sec = 0;
+             timestamp.tv_nsec = 0;
+     }
      hdr = kasprintf(GFP_ATOMIC, RAMOOPS_KERNMSG_HDR "%lu.%lu\n",
-             (long)timestamp.tv_sec, (long)timestamp.tv_usec);
+             (long)timestamp.tv_sec, (long)(timestamp.tv_nsec / 1000));
      WARN_ON_ONCE(!hdr);
      len = hdr ? strlen(hdr) : 0;
      persistent_ram_write(prz, hdr, len);
include/linux/time.h +1 -0
index 4d358e9..0015aea 100644
@@ -158,6 +158,7 @@ extern int do_setitimer(int which, struct itimerval *value,
                      struct itimerval *ovalue);
 extern unsigned int alarm_setitimer(unsigned int seconds);
 extern int do_getitimer(int which, struct itimerval *value);
+extern int __getnstimeofday(struct timespec *tv);
 extern void getnstimeofday(struct timespec *tv);
 extern void getrawmonotonic(struct timespec *ts);
 extern void getnstime_raw_and_real(struct timespec *ts_raw,
kernel/time/timekeeping.c +24 -5
index 4c7de02..dfc7f87 100644
@@ -214,19 +214,18 @@ static void timekeeping_forward_now(struct timekeeper *tk)
 }
 
 /**
- * getnstimeofday - Returns the time of day in a timespec
+ * __getnstimeofday - Returns the time of day in a timespec.
  * @ts:              pointer to the timespec to be set
  *
- * Returns the time of day in a timespec.
+ * Updates the time of day in the timespec.
+ * Returns 0 on success, or -ve when suspended (timespec will be undefined).
  */
-void getnstimeofday(struct timespec *ts)
+int __getnstimeofday(struct timespec *ts)
 {
      struct timekeeper *tk = &timekeeper;
      unsigned long seq;
      s64 nsecs = 0;
 
-     WARN_ON(timekeeping_suspended);
-
      do {
              seq = read_seqbegin(&tk->lock);
 
@@ -237,6 +236,26 @@ void getnstimeofday(struct timespec *ts)
 
      ts->tv_nsec = 0;
      timespec_add_ns(ts, nsecs);
+
+     /*
+      * Do not bail out early, in case there were callers still using
+      * the value, even in the face of the WARN_ON.
+      */
+     if (unlikely(timekeeping_suspended))
+             return -EAGAIN;
+     return 0;
+}
+EXPORT_SYMBOL(__getnstimeofday);
+
+/**
+ * getnstimeofday - Returns the time of day in a timespec.
+ * @ts:              pointer to the timespec to be set
+ *
+ * Returns the time of day in a timespec (WARN if suspended).
+ */
+void getnstimeofday(struct timespec *ts)
+{
+     WARN_ON(__getnstimeofday(ts));
 }
 EXPORT_SYMBOL(getnstimeofday);
 
Quoted 4 lines
Quoted 4 lines

--
1.7.9.5

--
Kees Cook
Chrome OS Security