prevent timespec/timeval to ktime_t overflow

Frank v. Waveren pointed out that on 64bit machines the timespec to ktime_t conversion might overflow. This is also true for timeval to ktime_t conversions. This breaks a "sleep inf" on 64bit machines.

Check the seconds argument to the conversion and limit it to the maximum time which can be represented by ktime_t.

Quoted 3 lines · Thomas Gleixner

Frank v. Waveren pointed out that on 64bit machines the timespec to ktime_t conversion might overflow. This is also true for timeval to ktime_t conversions. This breaks a "sleep inf" on 64bit machines.

...

Quoted 2 lines

Check the seconds argument to the conversion and limit it to the maximum time which can be represented by ktime_t.

It's a solution, and it more or less fixes things without any changes to userspace, which is nice. I still prefer my patch in <[email protected]> though, possibly with modifications so it doesn't affect all timespec users but only nanosleep (we'd have to check if the other timespec users aren't converting to ktime_t).

With this patch, we sleep shorter than specified, and don't signal this in any way. Returning EINVAL for anything except negative tv_sec or invalid tv_nsec breaks the spec too, but I prefer errors to silently sleeping too short.

I'll grant this is more of an aesthetic point than something that'll cause real-world problems (300 years is a long time for any sleep), but if things break I like them to do so as loudly as possible, as a general rule.

Quoted 4 lines

With this patch, we sleep shorter than specified, and don't signal this in any way. Returning EINVAL for anything except negative tv_sec or invalid tv_nsec breaks the spec too, but I prefer errors to silently sleeping too short.

I really don't care whether we sleep 100 or 5000 years in the case of "sleep MAX_LONG"

Quoted 4 lines

I'll grant this is more of an aesthetic point than something that'll cause real-world problems (300 years is a long time for any sleep), but if things break I like them to do so as loudly as possible, as a general rule.

One thing you ignore is that your patch does not cure the introduced user space breakage, it just replaces the overflow caused very short sleep by a return -EINVAL, which is breaking existing userspace in a different way. We have to preserve user space interfaces even when they violate your aesthetic well-being.

Quoted 7 lines · Thomas Gleixner

Quoted 4 lines

With this patch, we sleep shorter than specified, and don't signal this in any way. Returning EINVAL for anything except negative tv_sec or invalid tv_nsec breaks the spec too, but I prefer errors to silently sleeping too short.

I really don't care whether we sleep 100 or 5000 years in the case of "sleep MAX_LONG"

Don't sell your patch short, it still manages nearly 300 years..

Quoted 10 lines

Quoted 4 lines

I'll grant this is more of an aesthetic point than something that'll cause real-world problems (300 years is a long time for any sleep), but if things break I like them to do so as loudly as possible, as a general rule.

One thing you ignore is that your patch does not cure the introduced user space breakage, it just replaces the overflow caused very short sleep by a return -EINVAL, which is breaking existing userspace in a different way. We have to preserve user space interfaces even when they violate your aesthetic well-being.

The userspace interface gets broken either way. The error might actually serve as a decent portability wake up call, solaris 64 bit also silently overflows in nanosleep, and since I've only had the opportunity to check on solaris and linux, I wouldn't be surprised if other OSes had the same problem.

Well, the point is that pre hrtimer kernels did just sleep as long as they internaly could. So the hrtimers / ktime_t merge changed the userspace interface behaviour, which is breakage.

Frank v. Waveren pointed out that on 64bit machines the timespec to ktime_t conversion might overflow. This is also true for timeval to ktime_t conversions. This breaks a "sleep inf" on 64bit machines.

While a timespec/timeval with tx.sec = MAX_LONG is valid by specification the internal representation of ktime_t is based on nanoseconds. The conversion of seconds to nanoseconds overflows for seconds values >= (MAX_LONG / NSEC_PER_SEC).

Check the seconds argument to the conversion and limit it to the maximum time which can be represented by ktime_t.

Signed-off-by Thomas Gleixner <[email protected]>

include/linux/ktime.h +1 -0

index ed3396d..1beafb3 100644
@@ -57,6 +57,7 @@ typedef union {
 } ktime_t;
 
 #define KTIME_MAX			(~((u64)1 << 63))
+#define KTIME_SEC_MAX			(KTIME_MAX / NSEC_PER_SEC)
 
 /*
  * ktime_t definitions when using the 64-bit scalar representation:

Quoted 7 lines

* ktime_t definitions when using the 64-bit scalar representation:

(quoted diff) +2 -0

@@ -73,6 +74,10 @@ typedef union {
  */
 static inline ktime_t ktime_set(const long secs, const unsigned long nsecs)
 {
+#if (BITS_PER_LONG == 64)
+	if (unlikely(secs >= KTIME_SEC_MAX))

Review typedef union { · line 83

Quoted 5 lines

(quoted diff) +2 -0

+		return (ktime_t){ .tv64 = KTIME_MAX };
+#endif
 	return (ktime_t) { .tv64 = (s64)secs * NSEC_PER_SEC + (s64)nsecs };
 }
 

This makes my FC3 x86_64 testbox hang during udev startup. sysrq-T trace at
http://www.zip.com.au/~akpm/linux/patches/stuff/log-x.

I had a quick look at the args to hrtimer_nanosleep and all seems to be in order.

Quoted 7 lines · Andrew Morton

Quoted 7 lines

* ktime_t definitions when using the 64-bit scalar representation:

include/linux/ktime.h +2 -0

@@ -73,6 +74,10 @@ typedef union {
  */
 static inline ktime_t ktime_set(const long secs, const unsigned long nsecs)
 {
+#if (BITS_PER_LONG == 64)
+	if (unlikely(secs >= KTIME_SEC_MAX))

Quoted 8 lines

include/linux/ktime.h typedef union { · line 86

Quoted 5 lines

include/linux/ktime.h +2 -0

+		return (ktime_t){ .tv64 = KTIME_MAX };
+#endif
 	return (ktime_t) { .tv64 = (s64)secs * NSEC_PER_SEC + (s64)nsecs };
 }
 

This makes my FC3 x86_64 testbox hang during udev startup. sysrq-T trace at
http://www.zip.com.au/~akpm/linux/patches/stuff/log-x.

Does not help much.

Quoted 2 lines

I had a quick look at the args to hrtimer_nanosleep and all seems to be in order.

Hmm. I can not reproduce that on any one of my boxen. Can you please try with this debug variant, so we have a chance to figure out what's wrong.

Quoted 8 lines

Quoted 2 lines

This makes my FC3 x86_64 testbox hang during udev startup. sysrq-T trace at
http://www.zip.com.au/~akpm/linux/patches/stuff/log-x.

Does not help much.

Quoted 2 lines

I had a quick look at the args to hrtimer_nanosleep and all seems to be in order.

Quoted 3 lines

Hmm. I can not reproduce that on any one of my boxen. Can you please try with this debug variant, so we have a chance to figure out what's wrong.

With that patch the machine emits 88 bigabytes of stuff then locks up. Something a little less aggressive is needed, methinks.

Quoted 3 lines

With that patch the machine emits 88 bigabytes of stuff then locks up. Something a little less aggressive is needed, methinks.

Fun, here is a version with a bigabyte blocker.

So I modified it to only trigger if current->mm!=NULL and:

I wonder if this is related to the occasional hrtimr_run_queues() lockup which Andi is encountering.

Quoted 1 line

Fun, here is a version with a bigabyte blocker.

Your patch triggers waaaaaaay early.

netconsole: remote IP 192.168.2.33
netconsole: remote ethernet address 00:0d:56:c6:c6:cc Initializing CPU#0
PID hash table entries: 4096 (order: 12, 32768 bytes) time.c: Using 14.318180 MHz WALL HPET GTOD HPET/TSC timer. time.c: Detected 3400.238 MHz processor.
ktime_set: -1157140842 : 0

BUG: warning at include/linux/ktime.h:84/ktime_set()

Call Trace:
 <IRQ> [<ffffffff80247021>] hrtimer_run_queues+0x10e/0x211

I wonder if this is related to the occasional hrtimr_run_queues() lockup which Andi is encountering.

This smells like gcc magic. Can you please disassemble the code in question ?

Quoted 2 lines

I wonder if this is related to the occasional hrtimr_run_queues() lockup which Andi is encountering.

Hmm, not sure.

I'm quite sure right now.

Quoted 11 lines · Andrew Morton

Quoted 1 line

Fun, here is a version with a bigabyte blocker.

Your patch triggers waaaaaaay early.

netconsole: remote IP 192.168.2.33
netconsole: remote ethernet address 00:0d:56:c6:c6:cc Initializing CPU#0
PID hash table entries: 4096 (order: 12, 32768 bytes) time.c: Using 14.318180 MHz WALL HPET GTOD HPET/TSC timer. time.c: Detected 3400.238 MHz processor.
ktime_set: -1157140842 : 0

-------------^ !!!!!!!

Quoted 4 lines

BUG: warning at include/linux/ktime.h:84/ktime_set()

Call Trace:
 <IRQ> [<ffffffff80247021>] hrtimer_run_queues+0x10e/0x211

This seems to happen inside hrtimer_get_softirq_time(). wall_to_monotonic is negative.

Why does the check trigger ? We compare a "long", which contains a negative value against some positive constant.

ktime_t ktime_set(const long secs, const unsigned long nsecs)

{
	if (unlikely(secs >= KTIME_SEC_MAX)) {

where KTIME_SEC_MAX is 0x7FFFFFFFFFFFFFFF / 1000 000 000 =

9223372036 == 0x225C17D04,

which is compared against

-1157140842 == 0xFFFFFFFFBB076E96

This smells like gcc magic. Can you please disassemble the code in question ?

Here's a different patch, which should actually sleep for the specified amount of time up to 2^64 seconds with a loop around the sleeps and a tally of how long is left to sleep. It does mean we wake up once every 300 years on long sleeps, but that shouldn't cause any huge performance problems.

Quoted 5 lines · Frank v Waveren

Here's a different patch, which should actually sleep for the specified amount of time up to 2^64 seconds with a loop around the sleeps and a tally of how long is left to sleep. It does mean we wake up once every 300 years on long sleeps, but that shouldn't cause any huge performance problems.

Which non academic problem is solved by this patch ?