Re: [PATCH] time: create __getnstimeofday for WARNless calls
On Thu, Dec 20, 2012 at 3:43 AM, Thomas Gleixner wrote:
> On Mon, 17 Dec 2012, John Stultz wrote:
>> From: Kees Cook
>>
>> 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 ?
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
>
> Thanks,
>
> tglx
>
>> Reported-by: Doug Anderson
>> Cc: Anton Vorontsov
>> Cc: Thomas Gleixner
>> Signed-off-by: Kees Cook
>> Signed-off-by: John Stultz
>> ---
>> fs/pstore/ram.c | 10 +++---
>> include/linux/time.h |1 +
>> kernel/time/timekeeping.c | 29 -
>> 3 files changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index 1a4f6da..dacfe78 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -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();
>> + /* Report zeroed timestamp if called before timekeeping has resumed. */
>> + if (__getnstimeofday()) {
>> + 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);
>> diff --git a/include/linux/time.h b/include/linux/time.h
>> index 4d358e9..0015aea 100644
>> --- a/include/linux/time.h
>> +++ b/include/linux/time.h
>> @@ -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,
>> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
>> index 4c7de02..dfc7f87 100644
>> --- a/kernel/time/timekeeping.c
>> +++ b/kernel/time/timekeeping.c
>> @@ -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 =
>> unsigned long seq;
>> s64 nsecs = 0;
>>
>> - WARN_ON(timekeeping_suspended);
>> -
>> do {
>> seq = read_seqbegin(>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);
>>
>> --
>> 1.7.9.5
>>
>>
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
On Mon, 17 Dec 2012, John Stultz wrote:
> From: Kees Cook
>
> 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
> Reported-by: Doug Anderson
> Cc: Anton Vorontsov
> Cc: Thomas Gleixner
> Signed-off-by: Kees Cook
> Signed-off-by: John Stultz
> ---
> fs/pstore/ram.c | 10 +++---
> include/linux/time.h |1 +
> kernel/time/timekeeping.c | 29 -
> 3 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 1a4f6da..dacfe78 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -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();
> + /* Report zeroed timestamp if called before timekeeping has resumed. */
> + if (__getnstimeofday()) {
> + 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);
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 4d358e9..0015aea 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -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,
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 4c7de02..dfc7f87 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -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 =
> unsigned long seq;
> s64 nsecs = 0;
>
> - WARN_ON(timekeeping_suspended);
> -
> do {
> seq = read_seqbegin(>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);
>
> --
> 1.7.9.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
On Mon, 17 Dec 2012, John Stultz wrote:
From: Kees Cook keesc...@chromium.org
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
Reported-by: Doug Anderson diand...@chromium.org
Cc: Anton Vorontsov anton.voront...@linaro.org
Cc: Thomas Gleixner t...@linutronix.de
Signed-off-by: Kees Cook keesc...@chromium.org
Signed-off-by: John Stultz john.stu...@linaro.org
---
fs/pstore/ram.c | 10 +++---
include/linux/time.h |1 +
kernel/time/timekeeping.c | 29 -
3 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 1a4f6da..dacfe78 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -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);
diff --git a/include/linux/time.h b/include/linux/time.h
index 4d358e9..0015aea 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -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,
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4c7de02..dfc7f87 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -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);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
On Thu, Dec 20, 2012 at 3:43 AM, Thomas Gleixner t...@linutronix.de wrote:
On Mon, 17 Dec 2012, John Stultz wrote:
From: Kees Cook keesc...@chromium.org
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 ?
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
Thanks,
tglx
Reported-by: Doug Anderson diand...@chromium.org
Cc: Anton Vorontsov anton.voront...@linaro.org
Cc: Thomas Gleixner t...@linutronix.de
Signed-off-by: Kees Cook keesc...@chromium.org
Signed-off-by: John Stultz john.stu...@linaro.org
---
fs/pstore/ram.c | 10 +++---
include/linux/time.h |1 +
kernel/time/timekeeping.c | 29 -
3 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 1a4f6da..dacfe78 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -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);
diff --git a/include/linux/time.h b/include/linux/time.h
index 4d358e9..0015aea 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -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,
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4c7de02..dfc7f87 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -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);
--
1.7.9.5
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
On Mon, Dec 17, 2012 at 01:51:54PM -0500, John Stultz wrote: > From: Kees Cook > > 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. > > Reported-by: Doug Anderson > Cc: Anton Vorontsov > Cc: Thomas Gleixner > Signed-off-by: Kees Cook > Signed-off-by: John Stultz > --- > fs/pstore/ram.c | 10 +++--- For pstore/ram part: Acked-by: Anton Vorontsov Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
On Mon, Dec 17, 2012 at 01:51:54PM -0500, John Stultz wrote: From: Kees Cook keesc...@chromium.org 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. Reported-by: Doug Anderson diand...@chromium.org Cc: Anton Vorontsov anton.voront...@linaro.org Cc: Thomas Gleixner t...@linutronix.de Signed-off-by: Kees Cook keesc...@chromium.org Signed-off-by: John Stultz john.stu...@linaro.org --- fs/pstore/ram.c | 10 +++--- For pstore/ram part: Acked-by: Anton Vorontsov anton.voront...@linaro.org Thanks! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
On Fri, Dec 14, 2012 at 5:16 PM, John Stultz wrote: > On 12/13/2012 10:17 AM, Kees Cook wrote: >> >> John, any feedback on this? >> > Sorry, yea, I've been meaning to get back to this. > > I'm still on the fence about just making getnstimeofday() safe for when > timekeeping is suspended, but at the same time, your issue needs fixing. > Also bailing out at the end still seems off to me. Even if someone is using > the values despite the WARN_ON, they really are getting junk values, and for > all the time that WARN_ON has been there, you're the first to report running > into it. > > Even so, I think I'm ok with this patch for now, but I suspect we may want > to rework it later. > > Looking at my inbox, I actually can't find a copy of this specific patch. Do > you mind bouncing it to me, so I have something I can apply? > > Should this also get marked for -stable? Nah, I don't think it's worth it. Thanks! -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
On 12/14/2012 05:16 PM, John Stultz wrote: On 12/13/2012 10:17 AM, Kees Cook wrote: John, any feedback on this? Looking at my inbox, I actually can't find a copy of this specific patch. Do you mind bouncing it to me, so I have something I can apply? Nm, I found it. thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
On 12/13/2012 10:17 AM, Kees Cook wrote: John, any feedback on this? Sorry, yea, I've been meaning to get back to this. I'm still on the fence about just making getnstimeofday() safe for when timekeeping is suspended, but at the same time, your issue needs fixing. Also bailing out at the end still seems off to me. Even if someone is using the values despite the WARN_ON, they really are getting junk values, and for all the time that WARN_ON has been there, you're the first to report running into it. Even so, I think I'm ok with this patch for now, but I suspect we may want to rework it later. Looking at my inbox, I actually can't find a copy of this specific patch. Do you mind bouncing it to me, so I have something I can apply? Should this also get marked for -stable? thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
On 12/13/2012 10:17 AM, Kees Cook wrote: John, any feedback on this? Sorry, yea, I've been meaning to get back to this. I'm still on the fence about just making getnstimeofday() safe for when timekeeping is suspended, but at the same time, your issue needs fixing. Also bailing out at the end still seems off to me. Even if someone is using the values despite the WARN_ON, they really are getting junk values, and for all the time that WARN_ON has been there, you're the first to report running into it. Even so, I think I'm ok with this patch for now, but I suspect we may want to rework it later. Looking at my inbox, I actually can't find a copy of this specific patch. Do you mind bouncing it to me, so I have something I can apply? Should this also get marked for -stable? thanks -john -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
On 12/14/2012 05:16 PM, John Stultz wrote: On 12/13/2012 10:17 AM, Kees Cook wrote: John, any feedback on this? Looking at my inbox, I actually can't find a copy of this specific patch. Do you mind bouncing it to me, so I have something I can apply? Nm, I found it. thanks -john -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
On Fri, Dec 14, 2012 at 5:16 PM, John Stultz johns...@us.ibm.com wrote: On 12/13/2012 10:17 AM, Kees Cook wrote: John, any feedback on this? Sorry, yea, I've been meaning to get back to this. I'm still on the fence about just making getnstimeofday() safe for when timekeeping is suspended, but at the same time, your issue needs fixing. Also bailing out at the end still seems off to me. Even if someone is using the values despite the WARN_ON, they really are getting junk values, and for all the time that WARN_ON has been there, you're the first to report running into it. Even so, I think I'm ok with this patch for now, but I suspect we may want to rework it later. Looking at my inbox, I actually can't find a copy of this specific patch. Do you mind bouncing it to me, so I have something I can apply? Should this also get marked for -stable? Nah, I don't think it's worth it. Thanks! -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
John, any feedback on this?
Thanks,
-Kees
On Mon, Nov 19, 2012 at 10:26 AM, Kees Cook wrote:
> 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.
>
> Reported-by: Doug Anderson
> Cc: John Stultz
> Cc: Anton Vorontsov
> Signed-off-by: Kees Cook
> ---
> fs/pstore/ram.c | 10 +++---
> include/linux/time.h |1 +
> kernel/time/timekeeping.c | 29 -
> 3 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 1a4f6da..dacfe78 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -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();
> + /* Report zeroed timestamp if called before timekeeping has resumed.
> */
> + if (__getnstimeofday()) {
> + 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);
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 4d358e9..0015aea 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -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,
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index e424970..84820dc 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -220,19 +220,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 =
> unsigned long seq;
> s64 nsecs = 0;
>
> - WARN_ON(timekeeping_suspended);
> -
> do {
> seq = read_seqbegin(>lock);
>
> @@ -243,6 +242,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);
>
> --
> 1.7.9.5
>
>
> --
> Kees Cook
> Chrome OS Security
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] time: create __getnstimeofday for WARNless calls
John, any feedback on this?
Thanks,
-Kees
On Mon, Nov 19, 2012 at 10:26 AM, Kees Cook keesc...@chromium.org wrote:
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.
Reported-by: Doug Anderson diand...@chromium.org
Cc: John Stultz johns...@us.ibm.com
Cc: Anton Vorontsov anton.voront...@linaro.org
Signed-off-by: Kees Cook keesc...@chromium.org
---
fs/pstore/ram.c | 10 +++---
include/linux/time.h |1 +
kernel/time/timekeeping.c | 29 -
3 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 1a4f6da..dacfe78 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -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);
diff --git a/include/linux/time.h b/include/linux/time.h
index 4d358e9..0015aea 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -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,
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index e424970..84820dc 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -220,19 +220,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);
@@ -243,6 +242,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);
--
1.7.9.5
--
Kees Cook
Chrome OS Security
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
