[RFC][PATCH v2] ktime: Fix ktime_divns to do signed division

[RFC][PATCH v2] ktime: Fix ktime_divns to do signed division

[John Stultz]

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.

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 BUG_ON checks for negative dividers in both
the 32 and 64bit versions to make sure no such use cases
creep in.

Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Josh Boyer <jwboyer@redhat.com>
Cc: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Cc: Trevor Cordes <trevor@tecnopolis.ca>
Tested-by: Trevor Cordes <trevor@tecnopolis.ca>
Reported-by: Trevor Cordes <trevor@tecnopolis.ca>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 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..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;
 }
-- 
1.9.1

Re: [RFC][PATCH v2] ktime: Fix ktime_divns to do signed division

[Nicolas Pitre]

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

Re: [RFC][PATCH v2] ktime: Fix ktime_divns to do signed division

[John Stultz]

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