Messages in this thread | | | Date | Mon, 19 Nov 2012 13:42:21 -0800 | Subject | Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable | From | Kees Cook <> |
| |
On Mon, Nov 19, 2012 at 10:57 AM, John Stultz <johnstul@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 <johnstul@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 <johnstul@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 "most recent" time, and callers not expecting to run when suspended can still be detected.
> > 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; > + > /* read clocksource: */ > clock = tk->clock; > cycle_now = clock->read(clock); > @@ -166,6 +169,9 @@ static inline s64 timekeeping_get_ns_raw(struct > timekeeper *tk) > struct clocksource *clock; > s64 nsec; > + if (unlikely(timekeeping_suspended)) > + return 0; > + > /* read clocksource: */ > clock = tk->clock; > cycle_now = clock->read(clock);
Then fs/pstore/ram.c wouldn't have to set things to zero; it would just report the flat spot, which is better than reporting 0.
-Kees
-- Kees Cook Chrome OS Security
| |