Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable
On Mon, Nov 19, 2012 at 10:57 AM, John Stultz wrote:
> On 11/19/2012 09:45 AM, Kees Cook wrote:
>>
>> On Mon, Nov 19, 2012 at 9:23 AM, John Stultz wrote:
>>>
>>> On 11/18/2012 12:09 PM, Kees Cook wrote:
On Fri, Nov 16, 2012 at 7:16 PM, John Stultz
wrote:
>
> Yea, I wanted to revisit this, because it is an odd case.
>
> We don't want to call getnstimeofday() while the timekeeping code is
> suspended, since the clocksource cycle_last value may be invalid if the
> hardware was reset during suspend. Kees is correct, the WARN_ONs were
> there to make sure no one tries to use the timekeeping core before its
> resumed, so removing them is problematic.
>
> Your sugggestion of having the __do_gettimeofday() internal accessor
> that
> maybe returns an error if timekeeping has been suspended could work.
>
> The other possibility is depending on the needs for accuracy with the
> timestamp, current_kernel_time() might be a better interface to use,
> since
> it will return the time at the last tick, and doesn't require accessing
> the
> clocksource hardware. Might that be a simpler solution? Or is sub-tick
> granularity necessary?
I think it's only useful to have this to the same granularity as
sched_clock(), so things can be correlated to dmesg output. If it's
the same, I'd be fine to switch to using current_kernel_time().
>>>
>>> Oof. That's another can of worms, sched_clock() resolution isn't tied
>>> to
>>> getnstimeofday(), since you may have cases where the TSC is invalid for
>>> timekeeping (so we use something slow like the ACPI PM) but ok for
>>> scheduling, etc.
>>>
>>> Even so, its current_kernel_time() is just tick granularity, so that
>>> probably won't work for you.
>>>
>>> So I'm leaning towards Anton's suggestion of adding a new internal
>>> accessor
>>> that returns an error if we're suspended.
>>>
>>> Thomas, what do you think of something like what's below?
>>>
>>> thanks
>>> -john
>>>
>>>
>>> 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..bb2638c 100644
>>> --- a/kernel/time/timekeeping.c
>>> +++ b/kernel/time/timekeeping.c
>>> @@ -220,19 +220,20 @@ 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.
>>> + * Returns -1 if timekeeping is suspended.
>>> + * Returns 0 on success.
>>>*/
>>> -void getnstimeofday(struct timespec *ts)
>>> +int __getnstimeofday(struct timespec *ts)
>>> {
>>> struct timekeeper *tk =
>>> unsigned long seq;
>>> s64 nsecs = 0;
>>> - WARN_ON(timekeeping_suspended);
>>> -
>>> + if (unlikely(timekeeping_suspended));
>>> + return -1;
>>
>> Is it useful to make this -EAGAIN or something, just for clarity?
>> Also, this technically changes the semantics of callers that were
>> hitting WARN (there should be none) in that the timespec isn't updated
>> at all. In the prior code, a WARN would be emitted, but it would still
>> fill out the timespec and return.
>>
>> When I looked at implementing this error path, I actually moved the
>> return -EAGAIN to the end of the function to keep the results the
>> same. All that said, this is much cleaner and more correct if not
>> touching the timespec on error is tolerable.
>
>
> Yea, although really, the value returned if the WARN_ON is emitted is really
> junk. If the clocksource was reset under us, you could see really crazy time
> values.
>
> Thinking some more, a better solution could be to simply return the time
> when timekeeping was suspended, which would be sane.
>
> This wouldn't catch cases where folks were accessing the time while
> timekeeping is suspended, but they would get as valid a value as we can
> provide.
>
> The only downside, is that accessors could see a flat-spot in time where
> they just get the same value over and over while timekeeping is suspended.
> So could be possible for bad behavior if something was doing something dumb
> like spinning in a loop waiting for some time
Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable
On 11/19/2012 09:45 AM, Kees Cook wrote:
On Mon, Nov 19, 2012 at 9:23 AM, John Stultz wrote:
On 11/18/2012 12:09 PM, Kees Cook wrote:
On Fri, Nov 16, 2012 at 7:16 PM, John Stultz wrote:
Yea, I wanted to revisit this, because it is an odd case.
We don't want to call getnstimeofday() while the timekeeping code is
suspended, since the clocksource cycle_last value may be invalid if the
hardware was reset during suspend. Kees is correct, the WARN_ONs were
there to make sure no one tries to use the timekeeping core before its
resumed, so removing them is problematic.
Your sugggestion of having the __do_gettimeofday() internal accessor that
maybe returns an error if timekeeping has been suspended could work.
The other possibility is depending on the needs for accuracy with the
timestamp, current_kernel_time() might be a better interface to use,
since
it will return the time at the last tick, and doesn't require accessing
the
clocksource hardware. Might that be a simpler solution? Or is sub-tick
granularity necessary?
I think it's only useful to have this to the same granularity as
sched_clock(), so things can be correlated to dmesg output. If it's
the same, I'd be fine to switch to using current_kernel_time().
Oof. That's another can of worms, sched_clock() resolution isn't tied to
getnstimeofday(), since you may have cases where the TSC is invalid for
timekeeping (so we use something slow like the ACPI PM) but ok for
scheduling, etc.
Even so, its current_kernel_time() is just tick granularity, so that
probably won't work for you.
So I'm leaning towards Anton's suggestion of adding a new internal accessor
that returns an error if we're suspended.
Thomas, what do you think of something like what's below?
thanks
-john
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..bb2638c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -220,19 +220,20 @@ 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.
+ * Returns -1 if timekeeping is suspended.
+ * Returns 0 on success.
*/
-void getnstimeofday(struct timespec *ts)
+int __getnstimeofday(struct timespec *ts)
{
struct timekeeper *tk =
unsigned long seq;
s64 nsecs = 0;
- WARN_ON(timekeeping_suspended);
-
+ if (unlikely(timekeeping_suspended));
+ return -1;
Is it useful to make this -EAGAIN or something, just for clarity?
Also, this technically changes the semantics of callers that were
hitting WARN (there should be none) in that the timespec isn't updated
at all. In the prior code, a WARN would be emitted, but it would still
fill out the timespec and return.
When I looked at implementing this error path, I actually moved the
return -EAGAIN to the end of the function to keep the results the
same. All that said, this is much cleaner and more correct if not
touching the timespec on error is tolerable.
Yea, although really, the value returned if the WARN_ON is emitted is
really junk. If the clocksource was reset under us, you could see really
crazy time values.
Thinking some more, a better solution could be to simply return the time
when timekeeping was suspended, which would be sane.
This wouldn't catch cases where folks were accessing the time while
timekeeping is suspended, but they would get as valid a value as we can
provide.
The only downside, is that accessors could see a flat-spot in time where
they just get the same value over and over while timekeeping is
suspended. So could be possible for bad behavior if something was doing
something dumb like spinning in a loop waiting for some time to pass and
was preventing suspend from completing.
Untested patch below. Any thoughts?
thanks
-john
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index e424970..84593ef 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -146,6 +146,9 @@ static inline s64 timekeeping_get_ns(struct timekeeper *tk)
struct clocksource *clock;
s64 nsec;
+ if (unlikely(timekeeping_suspended))
+ return tk->xtime_nsec >> tk->shift;
+
Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable
On Mon, Nov 19, 2012 at 9:23 AM, John Stultz wrote:
> On 11/18/2012 12:09 PM, Kees Cook wrote:
>>
>> On Fri, Nov 16, 2012 at 7:16 PM, John Stultz wrote:
>>>
>>> Yea, I wanted to revisit this, because it is an odd case.
>>>
>>> We don't want to call getnstimeofday() while the timekeeping code is
>>> suspended, since the clocksource cycle_last value may be invalid if the
>>> hardware was reset during suspend. Kees is correct, the WARN_ONs were
>>> there to make sure no one tries to use the timekeeping core before its
>>> resumed, so removing them is problematic.
>>>
>>> Your sugggestion of having the __do_gettimeofday() internal accessor that
>>> maybe returns an error if timekeeping has been suspended could work.
>>>
>>> The other possibility is depending on the needs for accuracy with the
>>> timestamp, current_kernel_time() might be a better interface to use,
>>> since
>>> it will return the time at the last tick, and doesn't require accessing
>>> the
>>> clocksource hardware. Might that be a simpler solution? Or is sub-tick
>>> granularity necessary?
>>
>> I think it's only useful to have this to the same granularity as
>> sched_clock(), so things can be correlated to dmesg output. If it's
>> the same, I'd be fine to switch to using current_kernel_time().
>
> Oof. That's another can of worms, sched_clock() resolution isn't tied to
> getnstimeofday(), since you may have cases where the TSC is invalid for
> timekeeping (so we use something slow like the ACPI PM) but ok for
> scheduling, etc.
>
> Even so, its current_kernel_time() is just tick granularity, so that
> probably won't work for you.
>
> So I'm leaning towards Anton's suggestion of adding a new internal accessor
> that returns an error if we're suspended.
>
> Thomas, what do you think of something like what's below?
>
> thanks
> -john
>
>
> 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..bb2638c 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -220,19 +220,20 @@ 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.
> + * Returns -1 if timekeeping is suspended.
> + * Returns 0 on success.
> */
> -void getnstimeofday(struct timespec *ts)
> +int __getnstimeofday(struct timespec *ts)
> {
> struct timekeeper *tk =
> unsigned long seq;
> s64 nsecs = 0;
> - WARN_ON(timekeeping_suspended);
> -
> + if (unlikely(timekeeping_suspended));
> + return -1;
Is it useful to make this -EAGAIN or something, just for clarity?
Also, this technically changes the semantics of callers that were
hitting WARN (there should be none) in that the timespec isn't updated
at all. In the prior code, a WARN would be emitted, but it would still
fill out the timespec and return.
When I looked at implementing this error path, I actually moved the
return -EAGAIN to the end of the function to keep the results the
same. All that said, this is much cleaner and more correct if not
touching the timespec on error is tolerable.
> do {
> seq = read_seqbegin(>lock);
> @@ -243,6 +244,19 @@ void getnstimeofday(struct timespec *ts)
> ts->tv_nsec = 0;
> timespec_add_ns(ts, nsecs);
> + 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.
> + */
> +void getnstimeofday(struct timespec *ts)
> +{
> + WARN_ON(__getnstimeofday(ts));
> }
> EXPORT_SYMBOL(getnstimeofday);
>
-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 v2] pstore/ram: no timekeeping calls when unavailable
On 11/18/2012 12:09 PM, Kees Cook wrote:
On Fri, Nov 16, 2012 at 7:16 PM, John Stultz wrote:
Yea, I wanted to revisit this, because it is an odd case.
We don't want to call getnstimeofday() while the timekeeping code is
suspended, since the clocksource cycle_last value may be invalid if the
hardware was reset during suspend. Kees is correct, the WARN_ONs were
there to make sure no one tries to use the timekeeping core before its
resumed, so removing them is problematic.
Your sugggestion of having the __do_gettimeofday() internal accessor that
maybe returns an error if timekeeping has been suspended could work.
The other possibility is depending on the needs for accuracy with the
timestamp, current_kernel_time() might be a better interface to use, since
it will return the time at the last tick, and doesn't require accessing the
clocksource hardware. Might that be a simpler solution? Or is sub-tick
granularity necessary?
I think it's only useful to have this to the same granularity as
sched_clock(), so things can be correlated to dmesg output. If it's
the same, I'd be fine to switch to using current_kernel_time().
Oof. That's another can of worms, sched_clock() resolution isn't tied
to getnstimeofday(), since you may have cases where the TSC is invalid
for timekeeping (so we use something slow like the ACPI PM) but ok for
scheduling, etc.
Even so, its current_kernel_time() is just tick granularity, so that
probably won't work for you.
So I'm leaning towards Anton's suggestion of adding a new internal
accessor that returns an error if we're suspended.
Thomas, what do you think of something like what's below?
thanks
-john
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..bb2638c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -220,19 +220,20 @@ 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.
+ * Returns -1 if timekeeping is suspended.
+ * Returns 0 on success.
*/
-void getnstimeofday(struct timespec *ts)
+int __getnstimeofday(struct timespec *ts)
{
struct timekeeper *tk =
unsigned long seq;
s64 nsecs = 0;
- WARN_ON(timekeeping_suspended);
-
+ if (unlikely(timekeeping_suspended));
+ return -1;
do {
seq = read_seqbegin(>lock);
@@ -243,6 +244,19 @@ void getnstimeofday(struct timespec *ts)
ts->tv_nsec = 0;
timespec_add_ns(ts, nsecs);
+ 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.
+ */
+void getnstimeofday(struct timespec *ts)
+{
+ WARN_ON(__getnstimeofday(ts));
}
EXPORT_SYMBOL(getnstimeofday);
--
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 v2] pstore/ram: no timekeeping calls when unavailable
On 11/18/2012 12:09 PM, Kees Cook wrote:
On Fri, Nov 16, 2012 at 7:16 PM, John Stultz johns...@us.ibm.com wrote:
Yea, I wanted to revisit this, because it is an odd case.
We don't want to call getnstimeofday() while the timekeeping code is
suspended, since the clocksource cycle_last value may be invalid if the
hardware was reset during suspend. Kees is correct, the WARN_ONs were
there to make sure no one tries to use the timekeeping core before its
resumed, so removing them is problematic.
Your sugggestion of having the __do_gettimeofday() internal accessor that
maybe returns an error if timekeeping has been suspended could work.
The other possibility is depending on the needs for accuracy with the
timestamp, current_kernel_time() might be a better interface to use, since
it will return the time at the last tick, and doesn't require accessing the
clocksource hardware. Might that be a simpler solution? Or is sub-tick
granularity necessary?
I think it's only useful to have this to the same granularity as
sched_clock(), so things can be correlated to dmesg output. If it's
the same, I'd be fine to switch to using current_kernel_time().
Oof. That's another can of worms, sched_clock() resolution isn't tied
to getnstimeofday(), since you may have cases where the TSC is invalid
for timekeeping (so we use something slow like the ACPI PM) but ok for
scheduling, etc.
Even so, its current_kernel_time() is just tick granularity, so that
probably won't work for you.
So I'm leaning towards Anton's suggestion of adding a new internal
accessor that returns an error if we're suspended.
Thomas, what do you think of something like what's below?
thanks
-john
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..bb2638c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -220,19 +220,20 @@ 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.
+ * Returns -1 if timekeeping is suspended.
+ * Returns 0 on success.
*/
-void getnstimeofday(struct timespec *ts)
+int __getnstimeofday(struct timespec *ts)
{
struct timekeeper *tk = timekeeper;
unsigned long seq;
s64 nsecs = 0;
- WARN_ON(timekeeping_suspended);
-
+ if (unlikely(timekeeping_suspended));
+ return -1;
do {
seq = read_seqbegin(tk-lock);
@@ -243,6 +244,19 @@ void getnstimeofday(struct timespec *ts)
ts-tv_nsec = 0;
timespec_add_ns(ts, nsecs);
+ 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.
+ */
+void getnstimeofday(struct timespec *ts)
+{
+ WARN_ON(__getnstimeofday(ts));
}
EXPORT_SYMBOL(getnstimeofday);
--
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 v2] pstore/ram: no timekeeping calls when unavailable
On Mon, Nov 19, 2012 at 9:23 AM, John Stultz johns...@us.ibm.com wrote:
On 11/18/2012 12:09 PM, Kees Cook wrote:
On Fri, Nov 16, 2012 at 7:16 PM, John Stultz johns...@us.ibm.com wrote:
Yea, I wanted to revisit this, because it is an odd case.
We don't want to call getnstimeofday() while the timekeeping code is
suspended, since the clocksource cycle_last value may be invalid if the
hardware was reset during suspend. Kees is correct, the WARN_ONs were
there to make sure no one tries to use the timekeeping core before its
resumed, so removing them is problematic.
Your sugggestion of having the __do_gettimeofday() internal accessor that
maybe returns an error if timekeeping has been suspended could work.
The other possibility is depending on the needs for accuracy with the
timestamp, current_kernel_time() might be a better interface to use,
since
it will return the time at the last tick, and doesn't require accessing
the
clocksource hardware. Might that be a simpler solution? Or is sub-tick
granularity necessary?
I think it's only useful to have this to the same granularity as
sched_clock(), so things can be correlated to dmesg output. If it's
the same, I'd be fine to switch to using current_kernel_time().
Oof. That's another can of worms, sched_clock() resolution isn't tied to
getnstimeofday(), since you may have cases where the TSC is invalid for
timekeeping (so we use something slow like the ACPI PM) but ok for
scheduling, etc.
Even so, its current_kernel_time() is just tick granularity, so that
probably won't work for you.
So I'm leaning towards Anton's suggestion of adding a new internal accessor
that returns an error if we're suspended.
Thomas, what do you think of something like what's below?
thanks
-john
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..bb2638c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -220,19 +220,20 @@ 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.
+ * Returns -1 if timekeeping is suspended.
+ * Returns 0 on success.
*/
-void getnstimeofday(struct timespec *ts)
+int __getnstimeofday(struct timespec *ts)
{
struct timekeeper *tk = timekeeper;
unsigned long seq;
s64 nsecs = 0;
- WARN_ON(timekeeping_suspended);
-
+ if (unlikely(timekeeping_suspended));
+ return -1;
Is it useful to make this -EAGAIN or something, just for clarity?
Also, this technically changes the semantics of callers that were
hitting WARN (there should be none) in that the timespec isn't updated
at all. In the prior code, a WARN would be emitted, but it would still
fill out the timespec and return.
When I looked at implementing this error path, I actually moved the
return -EAGAIN to the end of the function to keep the results the
same. All that said, this is much cleaner and more correct if not
touching the timespec on error is tolerable.
do {
seq = read_seqbegin(tk-lock);
@@ -243,6 +244,19 @@ void getnstimeofday(struct timespec *ts)
ts-tv_nsec = 0;
timespec_add_ns(ts, nsecs);
+ 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.
+ */
+void getnstimeofday(struct timespec *ts)
+{
+ WARN_ON(__getnstimeofday(ts));
}
EXPORT_SYMBOL(getnstimeofday);
-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 v2] pstore/ram: no timekeeping calls when unavailable
On 11/19/2012 09:45 AM, Kees Cook wrote:
On Mon, Nov 19, 2012 at 9:23 AM, John Stultz johns...@us.ibm.com wrote:
On 11/18/2012 12:09 PM, Kees Cook wrote:
On Fri, Nov 16, 2012 at 7:16 PM, John Stultz johns...@us.ibm.com wrote:
Yea, I wanted to revisit this, because it is an odd case.
We don't want to call getnstimeofday() while the timekeeping code is
suspended, since the clocksource cycle_last value may be invalid if the
hardware was reset during suspend. Kees is correct, the WARN_ONs were
there to make sure no one tries to use the timekeeping core before its
resumed, so removing them is problematic.
Your sugggestion of having the __do_gettimeofday() internal accessor that
maybe returns an error if timekeeping has been suspended could work.
The other possibility is depending on the needs for accuracy with the
timestamp, current_kernel_time() might be a better interface to use,
since
it will return the time at the last tick, and doesn't require accessing
the
clocksource hardware. Might that be a simpler solution? Or is sub-tick
granularity necessary?
I think it's only useful to have this to the same granularity as
sched_clock(), so things can be correlated to dmesg output. If it's
the same, I'd be fine to switch to using current_kernel_time().
Oof. That's another can of worms, sched_clock() resolution isn't tied to
getnstimeofday(), since you may have cases where the TSC is invalid for
timekeeping (so we use something slow like the ACPI PM) but ok for
scheduling, etc.
Even so, its current_kernel_time() is just tick granularity, so that
probably won't work for you.
So I'm leaning towards Anton's suggestion of adding a new internal accessor
that returns an error if we're suspended.
Thomas, what do you think of something like what's below?
thanks
-john
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..bb2638c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -220,19 +220,20 @@ 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.
+ * Returns -1 if timekeeping is suspended.
+ * Returns 0 on success.
*/
-void getnstimeofday(struct timespec *ts)
+int __getnstimeofday(struct timespec *ts)
{
struct timekeeper *tk = timekeeper;
unsigned long seq;
s64 nsecs = 0;
- WARN_ON(timekeeping_suspended);
-
+ if (unlikely(timekeeping_suspended));
+ return -1;
Is it useful to make this -EAGAIN or something, just for clarity?
Also, this technically changes the semantics of callers that were
hitting WARN (there should be none) in that the timespec isn't updated
at all. In the prior code, a WARN would be emitted, but it would still
fill out the timespec and return.
When I looked at implementing this error path, I actually moved the
return -EAGAIN to the end of the function to keep the results the
same. All that said, this is much cleaner and more correct if not
touching the timespec on error is tolerable.
Yea, although really, the value returned if the WARN_ON is emitted is
really junk. If the clocksource was reset under us, you could see really
crazy time values.
Thinking some more, a better solution could be to simply return the time
when timekeeping was suspended, which would be sane.
This wouldn't catch cases where folks were accessing the time while
timekeeping is suspended, but they would get as valid a value as we can
provide.
The only downside, is that accessors could see a flat-spot in time where
they just get the same value over and over while timekeeping is
suspended. So could be possible for bad behavior if something was doing
something dumb like spinning in a loop waiting for some time to pass and
was preventing suspend from completing.
Untested patch below. Any thoughts?
thanks
-john
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index e424970..84593ef 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -146,6 +146,9 @@ static inline s64 timekeeping_get_ns(struct timekeeper *tk)
struct clocksource *clock;
s64 nsec;
+ if (unlikely(timekeeping_suspended))
+
Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable
On Mon, Nov 19, 2012 at 10:57 AM, John Stultz johns...@us.ibm.com wrote:
On 11/19/2012 09:45 AM, Kees Cook wrote:
On Mon, Nov 19, 2012 at 9:23 AM, John Stultz johns...@us.ibm.com wrote:
On 11/18/2012 12:09 PM, Kees Cook wrote:
On Fri, Nov 16, 2012 at 7:16 PM, John Stultz johns...@us.ibm.com
wrote:
Yea, I wanted to revisit this, because it is an odd case.
We don't want to call getnstimeofday() while the timekeeping code is
suspended, since the clocksource cycle_last value may be invalid if the
hardware was reset during suspend. Kees is correct, the WARN_ONs were
there to make sure no one tries to use the timekeeping core before its
resumed, so removing them is problematic.
Your sugggestion of having the __do_gettimeofday() internal accessor
that
maybe returns an error if timekeeping has been suspended could work.
The other possibility is depending on the needs for accuracy with the
timestamp, current_kernel_time() might be a better interface to use,
since
it will return the time at the last tick, and doesn't require accessing
the
clocksource hardware. Might that be a simpler solution? Or is sub-tick
granularity necessary?
I think it's only useful to have this to the same granularity as
sched_clock(), so things can be correlated to dmesg output. If it's
the same, I'd be fine to switch to using current_kernel_time().
Oof. That's another can of worms, sched_clock() resolution isn't tied
to
getnstimeofday(), since you may have cases where the TSC is invalid for
timekeeping (so we use something slow like the ACPI PM) but ok for
scheduling, etc.
Even so, its current_kernel_time() is just tick granularity, so that
probably won't work for you.
So I'm leaning towards Anton's suggestion of adding a new internal
accessor
that returns an error if we're suspended.
Thomas, what do you think of something like what's below?
thanks
-john
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..bb2638c 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -220,19 +220,20 @@ 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.
+ * Returns -1 if timekeeping is suspended.
+ * Returns 0 on success.
*/
-void getnstimeofday(struct timespec *ts)
+int __getnstimeofday(struct timespec *ts)
{
struct timekeeper *tk = timekeeper;
unsigned long seq;
s64 nsecs = 0;
- WARN_ON(timekeeping_suspended);
-
+ if (unlikely(timekeeping_suspended));
+ return -1;
Is it useful to make this -EAGAIN or something, just for clarity?
Also, this technically changes the semantics of callers that were
hitting WARN (there should be none) in that the timespec isn't updated
at all. In the prior code, a WARN would be emitted, but it would still
fill out the timespec and return.
When I looked at implementing this error path, I actually moved the
return -EAGAIN to the end of the function to keep the results the
same. All that said, this is much cleaner and more correct if not
touching the timespec on error is tolerable.
Yea, although really, the value returned if the WARN_ON is emitted is really
junk. If the clocksource was reset under us, you could see really crazy time
values.
Thinking some more, a better solution could be to simply return the time
when timekeeping was suspended, which would be sane.
This wouldn't catch cases where folks were accessing the time while
timekeeping is suspended, but they would get as valid a value as we can
provide.
The only downside, is that accessors could see a flat-spot in time where
they just get the same value over and over while timekeeping is suspended.
So could be possible for bad behavior if something was doing something dumb
like spinning in a loop waiting for some time to pass and was preventing
suspend from completing.
Untested patch below. Any thoughts?
Hrm, yeah, the dead-lock condition is a bit worrisome. It seems like
it'd be better to keep the WARN_ONs and still define __getnstimeofday?
Then callers that don't care about a flat-spot can report
Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable
On Fri, Nov 16, 2012 at 7:16 PM, John Stultz wrote: > Yea, I wanted to revisit this, because it is an odd case. > > We don't want to call getnstimeofday() while the timekeeping code is > suspended, since the clocksource cycle_last value may be invalid if the > hardware was reset during suspend. Kees is correct, the WARN_ONs were > there to make sure no one tries to use the timekeeping core before its > resumed, so removing them is problematic. > > Your sugggestion of having the __do_gettimeofday() internal accessor that > maybe returns an error if timekeeping has been suspended could work. > > The other possibility is depending on the needs for accuracy with the > timestamp, current_kernel_time() might be a better interface to use, since > it will return the time at the last tick, and doesn't require accessing the > clocksource hardware. Might that be a simpler solution? Or is sub-tick > granularity necessary? I think it's only useful to have this to the same granularity as sched_clock(), so things can be correlated to dmesg output. If it's the same, I'd be fine to switch to using current_kernel_time(). -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 v2] pstore/ram: no timekeeping calls when unavailable
On Fri, Nov 16, 2012 at 7:16 PM, John Stultz johns...@us.ibm.com wrote: Yea, I wanted to revisit this, because it is an odd case. We don't want to call getnstimeofday() while the timekeeping code is suspended, since the clocksource cycle_last value may be invalid if the hardware was reset during suspend. Kees is correct, the WARN_ONs were there to make sure no one tries to use the timekeeping core before its resumed, so removing them is problematic. Your sugggestion of having the __do_gettimeofday() internal accessor that maybe returns an error if timekeeping has been suspended could work. The other possibility is depending on the needs for accuracy with the timestamp, current_kernel_time() might be a better interface to use, since it will return the time at the last tick, and doesn't require accessing the clocksource hardware. Might that be a simpler solution? Or is sub-tick granularity necessary? I think it's only useful to have this to the same granularity as sched_clock(), so things can be correlated to dmesg output. If it's the same, I'd be fine to switch to using current_kernel_time(). -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 v2] pstore/ram: no timekeeping calls when unavailable
On 11/16/2012 06:53 PM, Anton Vorontsov wrote:
On Fri, Nov 09, 2012 at 05:26:53PM -0800, Kees Cook wrote:
[]
@@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
persistent_ram_zone *prz)
struct timeval timestamp;
size_t len;
- do_gettimeofday();
+ /* Handle dumping before timekeeping has resumed. */
+ if (unlikely(timekeeping_suspended)) {
+ timestamp.tv_sec = 0;
+ timestamp.tv_usec = 0;
+ } else
+ do_gettimeofday();
+
Would nulling out the timestamp be better done in do_gettimeofday()? That
way we don't have to export timekeeping internals and users would get
something more sane for this corner case.
Well... I'm not sure. If we don't want to expose the
timekeeping_suspended variable, maybe we need a function to check
this? I think it's probably better to find the users of timekeeping
that could call it when suspended. That's why I figured the BUG was
there. Very very few things should be attempting to call gettimeofday
in a place where it might be suspended. As such, it seems like those
things should be able to determine how to handle it. Maybe not
everything would be sensible to get back 0s.
In this particular case, I'm fine with removing the BUG and returning
0 instead, since that's fine for ramoops. :)
In the lack of agreement on kernel/time/timekeeping.c change, I can't
apply the patch. And personally I tend to agree that doing this workaround
in the pstore code is odd. How about introducing ___do_gettimeofday() that
is safe to call when suspened, and the func would have good kernel doc
comments explaining the purpose of it?
Yea, I wanted to revisit this, because it is an odd case.
We don't want to call getnstimeofday() while the timekeeping code is
suspended, since the clocksource cycle_last value may be invalid if the
hardware was reset during suspend. Kees is correct, the WARN_ONs were
there to make sure no one tries to use the timekeeping core before its
resumed, so removing them is problematic.
Your sugggestion of having the __do_gettimeofday() internal accessor
that maybe returns an error if timekeeping has been suspended could work.
The other possibility is depending on the needs for accuracy with the
timestamp, current_kernel_time() might be a better interface to use,
since it will return the time at the last tick, and doesn't require
accessing the clocksource hardware. Might that be a simpler solution?
Or is sub-tick granularity necessary?
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 v2] pstore/ram: no timekeeping calls when unavailable
On Fri, Nov 09, 2012 at 05:26:53PM -0800, Kees Cook wrote:
[]
> >> @@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
> >> persistent_ram_zone *prz)
> >> struct timeval timestamp;
> >> size_t len;
> >>
> >> - do_gettimeofday();
> >> + /* Handle dumping before timekeeping has resumed. */
> >> + if (unlikely(timekeeping_suspended)) {
> >> + timestamp.tv_sec = 0;
> >> + timestamp.tv_usec = 0;
> >> + } else
> >> + do_gettimeofday();
> >> +
> >
> > Would nulling out the timestamp be better done in do_gettimeofday()? That
> > way we don't have to export timekeeping internals and users would get
> > something more sane for this corner case.
>
> Well... I'm not sure. If we don't want to expose the
> timekeeping_suspended variable, maybe we need a function to check
> this? I think it's probably better to find the users of timekeeping
> that could call it when suspended. That's why I figured the BUG was
> there. Very very few things should be attempting to call gettimeofday
> in a place where it might be suspended. As such, it seems like those
> things should be able to determine how to handle it. Maybe not
> everything would be sensible to get back 0s.
>
> In this particular case, I'm fine with removing the BUG and returning
> 0 instead, since that's fine for ramoops. :)
In the lack of agreement on kernel/time/timekeeping.c change, I can't
apply the patch. And personally I tend to agree that doing this workaround
in the pstore code is odd. How about introducing ___do_gettimeofday() that
is safe to call when suspened, and the func would have good kernel doc
comments explaining the purpose of it?
Thanks,
Anton.
--
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 v2] pstore/ram: no timekeeping calls when unavailable
On Fri, Nov 09, 2012 at 05:26:53PM -0800, Kees Cook wrote:
[]
@@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
persistent_ram_zone *prz)
struct timeval timestamp;
size_t len;
- do_gettimeofday(timestamp);
+ /* Handle dumping before timekeeping has resumed. */
+ if (unlikely(timekeeping_suspended)) {
+ timestamp.tv_sec = 0;
+ timestamp.tv_usec = 0;
+ } else
+ do_gettimeofday(timestamp);
+
Would nulling out the timestamp be better done in do_gettimeofday()? That
way we don't have to export timekeeping internals and users would get
something more sane for this corner case.
Well... I'm not sure. If we don't want to expose the
timekeeping_suspended variable, maybe we need a function to check
this? I think it's probably better to find the users of timekeeping
that could call it when suspended. That's why I figured the BUG was
there. Very very few things should be attempting to call gettimeofday
in a place where it might be suspended. As such, it seems like those
things should be able to determine how to handle it. Maybe not
everything would be sensible to get back 0s.
In this particular case, I'm fine with removing the BUG and returning
0 instead, since that's fine for ramoops. :)
In the lack of agreement on kernel/time/timekeeping.c change, I can't
apply the patch. And personally I tend to agree that doing this workaround
in the pstore code is odd. How about introducing ___do_gettimeofday() that
is safe to call when suspened, and the func would have good kernel doc
comments explaining the purpose of it?
Thanks,
Anton.
--
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 v2] pstore/ram: no timekeeping calls when unavailable
On 11/16/2012 06:53 PM, Anton Vorontsov wrote:
On Fri, Nov 09, 2012 at 05:26:53PM -0800, Kees Cook wrote:
[]
@@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
persistent_ram_zone *prz)
struct timeval timestamp;
size_t len;
- do_gettimeofday(timestamp);
+ /* Handle dumping before timekeeping has resumed. */
+ if (unlikely(timekeeping_suspended)) {
+ timestamp.tv_sec = 0;
+ timestamp.tv_usec = 0;
+ } else
+ do_gettimeofday(timestamp);
+
Would nulling out the timestamp be better done in do_gettimeofday()? That
way we don't have to export timekeeping internals and users would get
something more sane for this corner case.
Well... I'm not sure. If we don't want to expose the
timekeeping_suspended variable, maybe we need a function to check
this? I think it's probably better to find the users of timekeeping
that could call it when suspended. That's why I figured the BUG was
there. Very very few things should be attempting to call gettimeofday
in a place where it might be suspended. As such, it seems like those
things should be able to determine how to handle it. Maybe not
everything would be sensible to get back 0s.
In this particular case, I'm fine with removing the BUG and returning
0 instead, since that's fine for ramoops. :)
In the lack of agreement on kernel/time/timekeeping.c change, I can't
apply the patch. And personally I tend to agree that doing this workaround
in the pstore code is odd. How about introducing ___do_gettimeofday() that
is safe to call when suspened, and the func would have good kernel doc
comments explaining the purpose of it?
Yea, I wanted to revisit this, because it is an odd case.
We don't want to call getnstimeofday() while the timekeeping code is
suspended, since the clocksource cycle_last value may be invalid if the
hardware was reset during suspend. Kees is correct, the WARN_ONs were
there to make sure no one tries to use the timekeeping core before its
resumed, so removing them is problematic.
Your sugggestion of having the __do_gettimeofday() internal accessor
that maybe returns an error if timekeeping has been suspended could work.
The other possibility is depending on the needs for accuracy with the
timestamp, current_kernel_time() might be a better interface to use,
since it will return the time at the last tick, and doesn't require
accessing the clocksource hardware. Might that be a simpler solution?
Or is sub-tick granularity necessary?
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 v2] pstore/ram: no timekeeping calls when unavailable
On 11/05/2012 02:00 PM, Kees Cook wrote:
We must not call timekeeping functions unless they are available. If we dump
before they have resumed, avoid a WARN_ON by setting the timestamp to 0.
Since the "ram" pstore driver can be a module, we must have
timekeeping_suspended exported.
Reported-by: Doug Anderson
Cc: Anton Vorontsov
Cc: John Stultz
Signed-off-by: Kees Cook
---
v2:
- export needed for timekeeping_suspended (thanks to Fengguang Wu).
---
fs/pstore/ram.c |8 +++-
kernel/time/timekeeping.c |1 +
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 1a4f6da..6d014e0 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
persistent_ram_zone *prz)
struct timeval timestamp;
size_t len;
- do_gettimeofday();
+ /* Handle dumping before timekeeping has resumed. */
+ if (unlikely(timekeeping_suspended)) {
+ timestamp.tv_sec = 0;
+ timestamp.tv_usec = 0;
+ } else
+ do_gettimeofday();
+
Would nulling out the timestamp be better done in do_gettimeofday()?
That way we don't have to export timekeeping internals and users would
get something more sane for this corner case.
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 v2] pstore/ram: no timekeeping calls when unavailable
On Fri, Nov 9, 2012 at 4:56 PM, John Stultz wrote:
> On 11/05/2012 02:00 PM, Kees Cook wrote:
>>
>> We must not call timekeeping functions unless they are available. If we
>> dump
>> before they have resumed, avoid a WARN_ON by setting the timestamp to 0.
>>
>> Since the "ram" pstore driver can be a module, we must have
>> timekeeping_suspended exported.
>>
>> Reported-by: Doug Anderson
>> Cc: Anton Vorontsov
>> Cc: John Stultz
>> Signed-off-by: Kees Cook
>> ---
>> v2:
>> - export needed for timekeeping_suspended (thanks to Fengguang Wu).
>> ---
>> fs/pstore/ram.c |8 +++-
>> kernel/time/timekeeping.c |1 +
>> 2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index 1a4f6da..6d014e0 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
>> persistent_ram_zone *prz)
>> struct timeval timestamp;
>> size_t len;
>>
>> - do_gettimeofday();
>> + /* Handle dumping before timekeeping has resumed. */
>> + if (unlikely(timekeeping_suspended)) {
>> + timestamp.tv_sec = 0;
>> + timestamp.tv_usec = 0;
>> + } else
>> + do_gettimeofday();
>> +
>
> Would nulling out the timestamp be better done in do_gettimeofday()? That
> way we don't have to export timekeeping internals and users would get
> something more sane for this corner case.
Well... I'm not sure. If we don't want to expose the
timekeeping_suspended variable, maybe we need a function to check
this? I think it's probably better to find the users of timekeeping
that could call it when suspended. That's why I figured the BUG was
there. Very very few things should be attempting to call gettimeofday
in a place where it might be suspended. As such, it seems like those
things should be able to determine how to handle it. Maybe not
everything would be sensible to get back 0s.
In this particular case, I'm fine with removing the BUG and returning
0 instead, since that's fine for ramoops. :)
-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 v2] pstore/ram: no timekeeping calls when unavailable
On Fri, Nov 9, 2012 at 4:56 PM, John Stultz johns...@us.ibm.com wrote:
On 11/05/2012 02:00 PM, Kees Cook wrote:
We must not call timekeeping functions unless they are available. If we
dump
before they have resumed, avoid a WARN_ON by setting the timestamp to 0.
Since the ram pstore driver can be a module, we must have
timekeeping_suspended exported.
Reported-by: Doug Anderson diand...@chromium.org
Cc: Anton Vorontsov cbouatmai...@gmail.com
Cc: John Stultz johns...@us.ibm.com
Signed-off-by: Kees Cook keesc...@chromium.org
---
v2:
- export needed for timekeeping_suspended (thanks to Fengguang Wu).
---
fs/pstore/ram.c |8 +++-
kernel/time/timekeeping.c |1 +
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 1a4f6da..6d014e0 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
persistent_ram_zone *prz)
struct timeval timestamp;
size_t len;
- do_gettimeofday(timestamp);
+ /* Handle dumping before timekeeping has resumed. */
+ if (unlikely(timekeeping_suspended)) {
+ timestamp.tv_sec = 0;
+ timestamp.tv_usec = 0;
+ } else
+ do_gettimeofday(timestamp);
+
Would nulling out the timestamp be better done in do_gettimeofday()? That
way we don't have to export timekeeping internals and users would get
something more sane for this corner case.
Well... I'm not sure. If we don't want to expose the
timekeeping_suspended variable, maybe we need a function to check
this? I think it's probably better to find the users of timekeeping
that could call it when suspended. That's why I figured the BUG was
there. Very very few things should be attempting to call gettimeofday
in a place where it might be suspended. As such, it seems like those
things should be able to determine how to handle it. Maybe not
everything would be sensible to get back 0s.
In this particular case, I'm fine with removing the BUG and returning
0 instead, since that's fine for ramoops. :)
-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 v2] pstore/ram: no timekeeping calls when unavailable
On 11/05/2012 02:00 PM, Kees Cook wrote:
We must not call timekeeping functions unless they are available. If we dump
before they have resumed, avoid a WARN_ON by setting the timestamp to 0.
Since the ram pstore driver can be a module, we must have
timekeeping_suspended exported.
Reported-by: Doug Anderson diand...@chromium.org
Cc: Anton Vorontsov cbouatmai...@gmail.com
Cc: John Stultz johns...@us.ibm.com
Signed-off-by: Kees Cook keesc...@chromium.org
---
v2:
- export needed for timekeeping_suspended (thanks to Fengguang Wu).
---
fs/pstore/ram.c |8 +++-
kernel/time/timekeeping.c |1 +
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 1a4f6da..6d014e0 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
persistent_ram_zone *prz)
struct timeval timestamp;
size_t len;
- do_gettimeofday(timestamp);
+ /* Handle dumping before timekeeping has resumed. */
+ if (unlikely(timekeeping_suspended)) {
+ timestamp.tv_sec = 0;
+ timestamp.tv_usec = 0;
+ } else
+ do_gettimeofday(timestamp);
+
Would nulling out the timestamp be better done in do_gettimeofday()?
That way we don't have to export timekeeping internals and users would
get something more sane for this corner case.
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/
[PATCH v2] pstore/ram: no timekeeping calls when unavailable
We must not call timekeeping functions unless they are available. If we dump
before they have resumed, avoid a WARN_ON by setting the timestamp to 0.
Since the "ram" pstore driver can be a module, we must have
timekeeping_suspended exported.
Reported-by: Doug Anderson
Cc: Anton Vorontsov
Cc: John Stultz
Signed-off-by: Kees Cook
---
v2:
- export needed for timekeeping_suspended (thanks to Fengguang Wu).
---
fs/pstore/ram.c |8 +++-
kernel/time/timekeeping.c |1 +
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 1a4f6da..6d014e0 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
persistent_ram_zone *prz)
struct timeval timestamp;
size_t len;
- do_gettimeofday();
+ /* Handle dumping before timekeeping has resumed. */
+ if (unlikely(timekeeping_suspended)) {
+ timestamp.tv_sec = 0;
+ timestamp.tv_usec = 0;
+ } else
+ do_gettimeofday();
+
hdr = kasprintf(GFP_ATOMIC, RAMOOPS_KERNMSG_HDR "%lu.%lu\n",
(long)timestamp.tv_sec, (long)timestamp.tv_usec);
WARN_ON_ONCE(!hdr);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index e424970..24ea968 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -33,6 +33,7 @@ __cacheline_aligned_in_smp DEFINE_SEQLOCK(xtime_lock);
/* flag for if timekeeping is suspended */
int __read_mostly timekeeping_suspended;
+EXPORT_SYMBOL_GPL(timekeeping_suspended);
static inline void tk_normalize_xtime(struct timekeeper *tk)
{
--
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/
[PATCH v2] pstore/ram: no timekeeping calls when unavailable
We must not call timekeeping functions unless they are available. If we dump
before they have resumed, avoid a WARN_ON by setting the timestamp to 0.
Since the ram pstore driver can be a module, we must have
timekeeping_suspended exported.
Reported-by: Doug Anderson diand...@chromium.org
Cc: Anton Vorontsov cbouatmai...@gmail.com
Cc: John Stultz johns...@us.ibm.com
Signed-off-by: Kees Cook keesc...@chromium.org
---
v2:
- export needed for timekeeping_suspended (thanks to Fengguang Wu).
---
fs/pstore/ram.c |8 +++-
kernel/time/timekeeping.c |1 +
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 1a4f6da..6d014e0 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
persistent_ram_zone *prz)
struct timeval timestamp;
size_t len;
- do_gettimeofday(timestamp);
+ /* Handle dumping before timekeeping has resumed. */
+ if (unlikely(timekeeping_suspended)) {
+ timestamp.tv_sec = 0;
+ timestamp.tv_usec = 0;
+ } else
+ do_gettimeofday(timestamp);
+
hdr = kasprintf(GFP_ATOMIC, RAMOOPS_KERNMSG_HDR %lu.%lu\n,
(long)timestamp.tv_sec, (long)timestamp.tv_usec);
WARN_ON_ONCE(!hdr);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index e424970..24ea968 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -33,6 +33,7 @@ __cacheline_aligned_in_smp DEFINE_SEQLOCK(xtime_lock);
/* flag for if timekeeping is suspended */
int __read_mostly timekeeping_suspended;
+EXPORT_SYMBOL_GPL(timekeeping_suspended);
static inline void tk_normalize_xtime(struct timekeeper *tk)
{
--
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/
