lkml.org 
[lkml]   [2015]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] timekeeping: always make sure wall_to_monotonic is negative
    From
    On Mon, Mar 16, 2015 at 12:28 PM, Wang YanQing <udknight@gmail.com> wrote:
    >
    > When wall_to_monotonic become positive, then I get below two
    > errors on an IMX6 development board without enable RTC device:
    >
    > 1:execute exportfs -a generate:
    > "exportfs: /opt/nfs/arm does not support NFS export"
    > 2:cat /proc/stat:
    > "btime 4294967236"
    >
    > I can product the same issues on x86 with newest kernel in next
    > tree with below c code:
    > "
    > int main(void)
    > {
    > struct timeval val;
    > int ret;
    >
    > val.tv_sec = 0;
    > val.tv_usec = 0;
    > ret = settimeofday(&val, NULL);
    > printf("ret:%d\n", ret);
    > return 0;
    > }
    > "
    > The reason is getboottime[64] return negative value, cause below
    > codes don't work:
    >
    > nfs error:cache.h:get_expiry return negative value cause
    > cache_flush always clear entries just added in
    > ip_map_parse.
    > proc/stat error:
    > seq_printf use "unsigned long" to show
    > a negative number return by getboottime.
    >
    > This patch fix it by validate new value of wall_to_monotonic
    > before assign it.


    Sorry for the slow response, and thanks for the problem report. Some
    questions below...

    So to restate the problem: setting the CLOCK_REALTIME back to a
    timevalue that is less then the value of CLOCK_MONOTONIC causes
    problems with various interfaces such as nfs and proc/stat.

    Is that right?



    > Signed-off-by: Wang YanQing <udknight@gmail.com>
    > ---
    > kernel/time/timekeeping.c | 71 ++++++++++++++++++++++++++++++++++++-----------
    > 1 file changed, 55 insertions(+), 16 deletions(-)
    >
    > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
    > index 91db941..799e323 100644
    > --- a/kernel/time/timekeeping.c
    > +++ b/kernel/time/timekeeping.c
    > @@ -96,11 +96,29 @@ static void tk_xtime_add(struct timekeeper *tk, const struct timespec64 *ts)
    > tk_normalize_xtime(tk);
    > }
    >
    > -static void tk_set_wall_to_mono(struct timekeeper *tk, struct timespec64 wtm)
    > +static inline bool timespec64_negative(struct timespec64 *ts)
    > +{
    > + if (ts->tv_sec > 0)
    > + return false;
    > + if (ts->tv_sec == 0 && ts->tv_nsec > 0)
    > + return false;
    > + return true;
    > +}
    > +
    > +static bool tk_set_wall_to_mono(struct timekeeper *tk, struct timespec64 wtm)
    > {
    > struct timespec64 tmp;
    >
    > /*
    > + * The current time
    > + * wall_to_monotonic is what we need to add to xtime (or xtime corrected
    > + * for sub jiffie times) to get to monotonic time. Monotonic is pegged
    > + * at zero at system boot time, so wall_to_monotonic will be negative.
    > + */
    > + if (!timespec64_negative(&wtm))
    > + return false;
    > +
    > + /*
    > * Verify consistency of: offset_real = -wall_to_monotonic
    > * before modifying anything
    > */
    > @@ -111,6 +129,7 @@ static void tk_set_wall_to_mono(struct timekeeper *tk, struct timespec64 wtm)
    > set_normalized_timespec64(&tmp, -wtm.tv_sec, -wtm.tv_nsec);
    > tk->offs_real = timespec64_to_ktime(tmp);
    > tk->offs_tai = ktime_add(tk->offs_real, ktime_set(tk->tai_offset, 0));
    > + return true;
    > }
    >
    > static inline void tk_update_sleep_time(struct timekeeper *tk, ktime_t delta)
    > @@ -796,8 +815,9 @@ EXPORT_SYMBOL(do_gettimeofday);
    > int do_settimeofday64(const struct timespec64 *ts)
    > {
    > struct timekeeper *tk = &tk_core.timekeeper;
    > - struct timespec64 ts_delta, xt;
    > + struct timespec64 ts_delta, tmp;
    > unsigned long flags;
    > + int ret = 0;
    >
    > if (!timespec64_valid_strict(ts))
    > return -EINVAL;
    > @@ -807,23 +827,27 @@ int do_settimeofday64(const struct timespec64 *ts)
    >
    > timekeeping_forward_now(tk);
    >
    > - xt = tk_xtime(tk);
    > - ts_delta.tv_sec = ts->tv_sec - xt.tv_sec;
    > - ts_delta.tv_nsec = ts->tv_nsec - xt.tv_nsec;
    > + tmp = tk_xtime(tk);
    > + ts_delta.tv_sec = ts->tv_sec - tmp.tv_sec;
    > + ts_delta.tv_nsec = ts->tv_nsec - tmp.tv_nsec;
    >
    > - tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, ts_delta));
    > + tmp = timespec64_sub(tk->wall_to_monotonic, ts_delta);
    > + if (!tk_set_wall_to_mono(tk, tmp)) {
    > + ret = -EINVAL;
    > + goto out;
    > + }

    It seems like a simpler check at the top of do_settimeofday64() would
    be easier then embedding the check in set_wall_to_mono and trying to
    deal with it later in the process of adjusting time.



    >
    > tk_set_xtime(tk, ts);
    > -
    > timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);
    > -
    > +out:
    > write_seqcount_end(&tk_core.seq);
    > raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
    >
    > /* signal hrtimers about time change */
    > - clock_was_set();
    > + if (!ret)
    > + clock_was_set();
    >
    > - return 0;
    > + return ret;
    > }
    > EXPORT_SYMBOL(do_settimeofday64);
    >
    > @@ -857,8 +881,13 @@ int timekeeping_inject_offset(struct timespec *ts)
    > goto error;
    > }
    >
    > + tmp = timespec64_sub(tk->wall_to_monotonic, ts64);
    > + if (!tk_set_wall_to_mono(tk, tmp)) {
    > + ret = -EINVAL;
    > + goto error;
    > + }
    > +

    Similarly here. A simple check at the top before we make any
    adjustments would make more sense to me.


    > tk_xtime_add(tk, &ts64);
    > - tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, ts64));
    >
    > error: /* even if we error out, we forwarded the time, so call update */
    > timekeeping_update(tk, TK_CLEAR_NTP | TK_MIRROR | TK_CLOCK_WAS_SET);
    > @@ -1140,14 +1169,21 @@ static struct timespec64 timekeeping_suspend_time;
    > static void __timekeeping_inject_sleeptime(struct timekeeper *tk,
    > struct timespec64 *delta)
    > {
    > + struct timespec64 tmp;
    > +
    > if (!timespec64_valid_strict(delta)) {
    > printk_deferred(KERN_WARNING
    > "__timekeeping_inject_sleeptime: Invalid "
    > "sleep delta value!\n");
    > return;
    > }
    > +
    > + tmp = timespec64_sub(tk->wall_to_monotonic, *delta);
    > + if (!tk_set_wall_to_mono(tk, tmp)) {
    > + WARN_ON(1);
    > + return;
    > + }


    Is this really possible? _inject_sleeptime should never be negative.
    Or am I missing something?


    > tk_xtime_add(tk, delta);
    > - tk_set_wall_to_mono(tk, timespec64_sub(tk->wall_to_monotonic, *delta));
    > tk_update_sleep_time(tk, timespec64_to_ktime(*delta));
    > tk_debug_account_sleep_time(delta);
    > }
    > @@ -1539,14 +1575,17 @@ static inline unsigned int accumulate_nsecs_to_secs(struct timekeeper *tk)
    > leap = second_overflow(tk->xtime_sec);
    > if (unlikely(leap)) {
    > struct timespec64 ts;
    > -
    > - tk->xtime_sec += leap;
    > + bool ret;
    >
    > ts.tv_sec = leap;
    > ts.tv_nsec = 0;
    > - tk_set_wall_to_mono(tk,
    > + ret = tk_set_wall_to_mono(tk,
    > timespec64_sub(tk->wall_to_monotonic, ts));

    How would adding time in accumulate_nsecs_to_secs ever cause
    wall_to_monotonic to not be negative?


    > -
    > + if (!ret) {
    > + WARN_ON_ONCE(1);
    > + break;
    > + }
    > + tk->xtime_sec += leap;
    > __timekeeping_set_tai_offset(tk, tk->tai_offset - leap);
    >
    > clock_set = TK_CLOCK_WAS_SET;
    > --
    > 2.2.2.dirty


    \
     
     \ /
      Last update: 2015-05-14 02:21    [W:6.009 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site