Re: [PATCH] prevent timespec/timeval to ktime_t overflow

[Frank v Waveren <fvw@var.cx>]

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.

It's not particularly pretty, the restart handler means there's a
little code duplication and the limit of 4 args in the restart block
is heaps of fun, but on the bright side this may avoid the hangs
Andrew was complaining of. As it's a patch to nanosleep instead of
timespec_to_ktime, it won't catch it if there are any other buggy
users. It should be compatible with Thomas' patch though, so once the
hangs on FC3 are resolved both could be used.

Thomas: This doesn't exactly preserve the pre-ktime behaviour, but I
assume that's ok if instead it does what it says on the tin?

Syscall restarts and such are new territory to me, so if people could
cast a critical eye over this for bugs I'd be most grateful.

Signed-off-by: Frank v Waveren <fvw@var.cx>

diff -urpN linux-2.6.17.11/include/linux/ktime.h linux-2.6.17.11-fvw/include/linux/ktime.h
--- linux-2.6.17.11/include/linux/ktime.h	2006-08-23 23:16:33.000000000 +0200
+++ linux-2.6.17.11-fvw/include/linux/ktime.h	2006-09-02 10:48:43.000000000 +0200
@@ -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:
diff -urpN linux-2.6.17.11/kernel/hrtimer.c linux-2.6.17.11-fvw/kernel/hrtimer.c
--- linux-2.6.17.11/kernel/hrtimer.c	2006-08-23 23:16:33.000000000 +0200
+++ linux-2.6.17.11-fvw/kernel/hrtimer.c	2006-09-02 12:07:02.000000000 +0200
@@ -706,31 +706,46 @@ static long __sched nanosleep_restart(st
 {
 	struct hrtimer_sleeper t;
 	struct timespec __user *rmtp;
-	struct timespec tu;
+	struct timespec tu, touter, tinner;
 	ktime_t time;
 
 	restart->fn = do_no_restart_syscall;
 
-	hrtimer_init(&t.timer, restart->arg3, HRTIMER_ABS);
-	t.timer.expires.tv64 = ((u64)restart->arg1 << 32) | (u64) restart->arg0;
-
-	if (do_nanosleep(&t, HRTIMER_ABS))
-		return 0;
-
-	rmtp = (struct timespec __user *) restart->arg2;
-	if (rmtp) {
-		time = ktime_sub(t.timer.expires, t.timer.base->get_time());
-		if (time.tv64 <= 0)
-			return 0;
-		tu = ktime_to_timespec(time);
-		if (copy_to_user(rmtp, &tu, sizeof(tu)))
-			return -EFAULT;
-	}
-
-	restart->fn = nanosleep_restart;
-
-	/* The other values in restart are already filled in */
-	return -ERESTART_RESTARTBLOCK;
+	time.tv64=(((u64)restart->arg1 & 0xFFFFFF) << 32) | (u64) restart->arg0;
+	touter=ktime_to_timespec(time);
+        touter.tv_sec += (u64)restart->arg1 >> 32;
+   
+        while (unlikely(touter.tv_sec > 0 || touter.tv_nsec > 0))
+   	{
+	        tinner.tv_sec = touter.tv_sec % KTIME_SEC_MAX;
+                touter.tv_sec -= tinner.tv_sec;
+                tinner.tv_nsec = touter.tv_nsec;
+                touter.tv_nsec = 0;
+
+
+	        hrtimer_init(&t.timer, restart->arg3, HRTIMER_ABS);
+		t.timer.expires = timespec_to_ktime(tinner);
+
+		if (do_nanosleep(&t, HRTIMER_ABS))
+			continue;
+
+		rmtp = (struct timespec __user *) restart->arg2;
+		if (rmtp) {
+			time = ktime_sub(t.timer.expires, t.timer.base->get_time());
+			if (time.tv64 <= 0)
+				continue;
+			tu = ktime_to_timespec(time);
+			if (copy_to_user(rmtp, &tu, sizeof(tu)))
+				return -EFAULT;
+		}
+
+		restart->fn = nanosleep_restart;
+
+		/* The other values in restart are already filled in */
+		return -ERESTART_RESTARTBLOCK;
+        }
+   	
+   return 0;
 }
 
 long hrtimer_nanosleep(struct timespec *rqtp, struct timespec __user *rmtp,
@@ -738,35 +753,55 @@ long hrtimer_nanosleep(struct timespec *
 {
 	struct restart_block *restart;
 	struct hrtimer_sleeper t;
-	struct timespec tu;
+	struct timespec tu, touter, tinner;
 	ktime_t rem;
 
-	hrtimer_init(&t.timer, clockid, mode);
-	t.timer.expires = timespec_to_ktime(*rqtp);
-	if (do_nanosleep(&t, mode))
-		return 0;
-
-	/* Absolute timers do not update the rmtp value and restart: */
-	if (mode == HRTIMER_ABS)
-		return -ERESTARTNOHAND;
-
-	if (rmtp) {
-		rem = ktime_sub(t.timer.expires, t.timer.base->get_time());
-		if (rem.tv64 <= 0)
-			return 0;
-		tu = ktime_to_timespec(rem);
-		if (copy_to_user(rmtp, &tu, sizeof(tu)))
-			return -EFAULT;
-	}
+        touter=*rqtp;
+        
+        while (touter.tv_sec > 0 || touter.tv_nsec > 0)
+        {
+	        tinner.tv_sec = touter.tv_sec % KTIME_SEC_MAX;
+                touter.tv_sec -= tinner.tv_sec;
+                tinner.tv_nsec = touter.tv_nsec;
+                touter.tv_nsec = 0;
+           
+        	hrtimer_init(&t.timer, clockid, mode);
+		t.timer.expires = timespec_to_ktime(tinner);
+		if (do_nanosleep(&t, mode))
+		       	continue;
+
+		/* Absolute timers do not update the rmtp value and restart: */
+		if (mode == HRTIMER_ABS)
+			return -ERESTARTNOHAND;
+
+		if (rmtp) {
+			rem = ktime_sub(t.timer.expires, t.timer.base->get_time());
+			if (rem.tv64 <= 0)
+				continue;
+			tu = ktime_to_timespec(rem);
+                        tu.tv_sec+=touter.tv_sec; /* No need to add tv_nsec, it always
+                                                   * gets fully handled on the first 
+                                                   * iteration */
+			if (copy_to_user(rmtp, &tu, sizeof(tu)))
+				return -EFAULT;
+		}
+
+		restart = &current_thread_info()->restart_block;
+		restart->fn = nanosleep_restart;
+		restart->arg0 = t.timer.expires.tv64 & 0xFFFFFFFF;
+                /* Iff longs are 64 bits, we may have a non-zero touter.tv_sec.
+                 * As long is 64 bits, the restart args are also 64 bits so
+                 * we can store the touter.tv_sec in the high 32 bits of 
+                 * arg2 */
+		restart->arg1 = (t.timer.expires.tv64 >> 32) + ((u64)touter.tv_sec << 32);
+		restart->arg2 = (unsigned long) rmtp;
+		restart->arg3 = (unsigned long) t.timer.base->index;
 
-	restart = &current_thread_info()->restart_block;
-	restart->fn = nanosleep_restart;
-	restart->arg0 = t.timer.expires.tv64 & 0xFFFFFFFF;
-	restart->arg1 = t.timer.expires.tv64 >> 32;
-	restart->arg2 = (unsigned long) rmtp;
-	restart->arg3 = (unsigned long) t.timer.base->index;
+		return -ERESTART_RESTARTBLOCK;
+        }
 
-	return -ERESTART_RESTARTBLOCK;
+   return 0;
+	
 }
 
 asmlinkage long


-- 
Frank v Waveren                                  Key fingerprint: BDD7 D61E
fvw@var.cx                                              5D39 CF05 4BFC F57A
Public key: hkp://wwwkeys.pgp.net/468D62C8              FA00 7D51 468D 62C8

Attachment: signature.asc
Description: Digital signature