| Message ID | 1431118043-23452-1-git-send-email-john.stultz@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 8 May 2015, John Stultz wrote: > It was noted that the 32bit implementation of ktime_divns() > was doing unsigned division and didn't properly handle > negative values. > > And when a ktime helper was changed to utilize > ktime_divns, it caused a regression on some IR blasters. > See the following bugzilla for details: > https://bugzilla.redhat.com/show_bug.cgi?id=1200353 > > This patch fixes the problem in ktime_divns by checking > and preserving the sign bit, and then reapplying it if > appropriate after the division, it also changes the return > type to a s64 to make it more obvious this is expected. > > Nicolas also pointed out that negative dividers would > cause infinite loops on 32bit systems, negative dividers > is unlikely for users of this function, but out of caution > this patch adds checks for negative dividers for both > 32-bit (BUG_ON) and 64-bit(WARN_ON) versions to make sure > no such use cases creep in. > > Cc: Nicolas Pitre <nicolas.pitre@linaro.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Josh Boyer <jwboyer@redhat.com> > Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> > Cc: Trevor Cordes <trevor@tecnopolis.ca> > Cc: <stable@vger.kernel.org> # 3.17+ for regression > Tested-by: Trevor Cordes <trevor@tecnopolis.ca> > Reported-by: Trevor Cordes <trevor@tecnopolis.ca> > Signed-off-by: John Stultz <john.stultz@linaro.org> Acked-by: Nicolas Pitre <nico@linaro.org> > --- > > New in v3: > * Fix casting issue Nicolas pointed out > * Use WARN_ON for 64bit case > > include/linux/ktime.h | 27 +++++++++++++++++++++++---- > kernel/time/hrtimer.c | 11 ++++++++--- > 2 files changed, 31 insertions(+), 7 deletions(-) > > diff --git a/include/linux/ktime.h b/include/linux/ktime.h > index 5fc3d10..ab2de1c7 100644 > --- a/include/linux/ktime.h > +++ b/include/linux/ktime.h > @@ -166,19 +166,38 @@ static inline bool ktime_before(const ktime_t cmp1, const ktime_t cmp2) > } > > #if BITS_PER_LONG < 64 > -extern u64 __ktime_divns(const ktime_t kt, s64 div); > -static inline u64 ktime_divns(const ktime_t kt, s64 div) > +extern s64 __ktime_divns(const ktime_t kt, s64 div); > +static inline s64 ktime_divns(const ktime_t kt, s64 div) > { > + /* > + * Negative divisors could cause an inf loop, > + * so bug out here. > + */ > + BUG_ON(div < 0); > if (__builtin_constant_p(div) && !(div >> 32)) { > - u64 ns = kt.tv64; > + s64 ns = kt.tv64; > + int neg = (ns < 0); > + > + if (neg) > + ns = -ns; > do_div(ns, div); > + if (neg) > + ns = -ns; > return ns; > } else { > return __ktime_divns(kt, div); > } > } > #else /* BITS_PER_LONG < 64 */ > -# define ktime_divns(kt, div) (u64)((kt).tv64 / (div)) > +static inline s64 ktime_divns(const ktime_t kt, s64 div) > +{ > + /* > + * 32-bit implementation cannot handle negative divisors, > + * so catch them on 64bit as well. > + */ > + WARN_ON(div < 0); > + return kt.tv64 / div; > +} > #endif > > static inline s64 ktime_to_us(const ktime_t kt) > diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c > index 76d4bd9..c98ce4d 100644 > --- a/kernel/time/hrtimer.c > +++ b/kernel/time/hrtimer.c > @@ -266,12 +266,15 @@ lock_hrtimer_base(const struct hrtimer *timer, unsigned long *flags) > /* > * Divide a ktime value by a nanosecond value > */ > -u64 __ktime_divns(const ktime_t kt, s64 div) > +s64 __ktime_divns(const ktime_t kt, s64 div) > { > - u64 dclc; > - int sft = 0; > + s64 dclc; > + int neg, sft = 0; > > dclc = ktime_to_ns(kt); > + neg = (dclc < 0); > + if (neg) > + dclc = -dclc; > /* Make sure the divisor is less than 2^32: */ > while (div >> 32) { > sft++; > @@ -279,6 +282,8 @@ u64 __ktime_divns(const ktime_t kt, s64 div) > } > dclc >>= sft; > do_div(dclc, (unsigned long) div); > + if (neg) > + dclc = -dclc; > > return dclc; > } > -- > 1.9.1 > > -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 12, 2015 at 7:14 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Fri, 8 May 2015, John Stultz wrote: >> +static inline s64 ktime_divns(const ktime_t kt, s64 div) >> { >> + /* >> + * Negative divisors could cause an inf loop, >> + * so bug out here. >> + */ >> + BUG_ON(div < 0); >> if (__builtin_constant_p(div) && !(div >> 32)) { >> - u64 ns = kt.tv64; >> + s64 ns = kt.tv64; >> + int neg = (ns < 0); >> + >> + if (neg) >> + ns = -ns; >> do_div(ns, div); > > This triggers the typecheck for u64 in do_div(). The delta patch below > should do the trick. > I started looking into this, and this landed in my inbox. Thanks Thomas! :) I reproduced the issue w/ the kbuildbot instructions and the patch below resolves it. Acked-by: John Stultz <john.stultz@linaro.org> > diff --git a/include/linux/ktime.h b/include/linux/ktime.h > index ab2de1c7932c..b9620ebe356f 100644 > --- a/include/linux/ktime.h > +++ b/include/linux/ktime.h > @@ -177,13 +177,13 @@ static inline s64 ktime_divns(const ktime_t kt, s64 div) > if (__builtin_constant_p(div) && !(div >> 32)) { > s64 ns = kt.tv64; > int neg = (ns < 0); > + u64 tmp; > > if (neg) > ns = -ns; > - do_div(ns, div); > - if (neg) > - ns = -ns; > - return ns; > + tmp = ns; > + do_div(tmp, div); > + return neg ? -tmp : tmp; > } else { > return __ktime_divns(kt, div); > } > diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c > index c98ce4d9f613..bac65f810afd 100644 > --- a/kernel/time/hrtimer.c > +++ b/kernel/time/hrtimer.c > @@ -268,8 +268,9 @@ lock_hrtimer_base(const struct hrtimer *timer, unsigned long *flags) > */ > s64 __ktime_divns(const ktime_t kt, s64 div) > { > - s64 dclc; > int neg, sft = 0; > + s64 dclc; > + u64 tmp; > > dclc = ktime_to_ns(kt); > neg = (dclc < 0); > @@ -280,12 +281,9 @@ s64 __ktime_divns(const ktime_t kt, s64 div) > sft++; > div >>= 1; > } > - dclc >>= sft; > - do_div(dclc, (unsigned long) div); > - if (neg) > - dclc = -dclc; > - > - return dclc; > + tmp = dclc >> sft; > + do_div(tmp, (unsigned long) div); > + return neg ? -tmp : tmp; > } > EXPORT_SYMBOL_GPL(__ktime_divns); > #endif /* BITS_PER_LONG >= 64 */ -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/ktime.h b/include/linux/ktime.h index 5fc3d10..ab2de1c7 100644 --- a/include/linux/ktime.h +++ b/include/linux/ktime.h @@ -166,19 +166,38 @@ static inline bool ktime_before(const ktime_t cmp1, const ktime_t cmp2) } #if BITS_PER_LONG < 64 -extern u64 __ktime_divns(const ktime_t kt, s64 div); -static inline u64 ktime_divns(const ktime_t kt, s64 div) +extern s64 __ktime_divns(const ktime_t kt, s64 div); +static inline s64 ktime_divns(const ktime_t kt, s64 div) { + /* + * Negative divisors could cause an inf loop, + * so bug out here. + */ + BUG_ON(div < 0); if (__builtin_constant_p(div) && !(div >> 32)) { - u64 ns = kt.tv64; + s64 ns = kt.tv64; + int neg = (ns < 0); + + if (neg) + ns = -ns; do_div(ns, div); + if (neg) + ns = -ns; return ns; } else { return __ktime_divns(kt, div); } } #else /* BITS_PER_LONG < 64 */ -# define ktime_divns(kt, div) (u64)((kt).tv64 / (div)) +static inline s64 ktime_divns(const ktime_t kt, s64 div) +{ + /* + * 32-bit implementation cannot handle negative divisors, + * so catch them on 64bit as well. + */ + WARN_ON(div < 0); + return kt.tv64 / div; +} #endif static inline s64 ktime_to_us(const ktime_t kt) diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c index 76d4bd9..c98ce4d 100644 --- a/kernel/time/hrtimer.c +++ b/kernel/time/hrtimer.c @@ -266,12 +266,15 @@ lock_hrtimer_base(const struct hrtimer *timer, unsigned long *flags) /* * Divide a ktime value by a nanosecond value */ -u64 __ktime_divns(const ktime_t kt, s64 div) +s64 __ktime_divns(const ktime_t kt, s64 div) { - u64 dclc; - int sft = 0; + s64 dclc; + int neg, sft = 0; dclc = ktime_to_ns(kt); + neg = (dclc < 0); + if (neg) + dclc = -dclc; /* Make sure the divisor is less than 2^32: */ while (div >> 32) { sft++; @@ -279,6 +282,8 @@ u64 __ktime_divns(const ktime_t kt, s64 div) } dclc >>= sft; do_div(dclc, (unsigned long) div); + if (neg) + dclc = -dclc; return dclc; }