| Message ID | 1430941315-18005-1-git-send-email-john.stultz@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, 6 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. > > This patch fixes the problem 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. > [...] > > -# 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. > + */ > + BUG_ON(div < 0); > + return (u64)((kt).tv64 / (div)) You forgot to remove the u64 cast? Is there a reason why it was there originally? Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, May 6, 2015 at 1:11 PM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > On Wed, 6 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. >> >> This patch fixes the problem 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. >> > [...] >> >> -# 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. >> + */ >> + BUG_ON(div < 0); >> + return (u64)((kt).tv64 / (div)) > > You forgot to remove the u64 cast? Thanks for catching that! > Is there a reason why it was there originally? I assume because nanoseconds are usually though of as unsigned. It was introduced way back in the original hrtimer core code: c0a3132963db68f1fbbd0e316b73de100fee3f08 thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/include/linux/ktime.h b/include/linux/ktime.h index 5fc3d10..ac9359e 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. + */ + BUG_ON(div < 0); + return (u64)((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; }