Date:	Thu, 09 Aug 2007 23:11:45 +0200
From:	Michael Kerrisk <mtk-manpages@....net>
To:	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>
CC:	Michael Kerrisk <mtk-manpages@....net>,
	lkml <linux-kernel@...r.kernel.org>,
	Linux Torvalds <torvalds@...ux-foundation.org>,
	Davide Libenzi <davidel@...ilserver.org>, drepper@...hat.com,
	stable@...nel.org, Christoph Hellwig <hch@....de>,
	Randy Dunlap <rdunlap@...otime.net>,
	Jan Engelhardt <jengelh@...putergmbh.de>, mtk@...gle.com
Subject: [PATCH] Revised timerfd() interface

Andrew,

Here's my first shot at changing the timerfd() interface; patch
is against 2.6.23-rc1-mm2.

I think I got to the bottom of understanding the timer code in
the end, but I may still have missed some things...

This is my first kernel patch of any substance.  Perhaps Randy
or Christoph can give my toes a light toasting for the various
boobs I've likely made.

Thomas, would you please look over this patch to verify my
timer code?

Davide: would you please bless this interface change ;-).
It makes it possible to:

1) Fetch the previous timer settings (i.e., interval, and time
   remaining until next expiration) while installing new settings

2) Retrieve the settings of an existing timer without changing
   the timer.

Both of these features are supported by the older Unix timer APIs
(setitimer/getitimer and timer_create/timer_settime/timer_gettime).

The changes are as follows:

a) A further argument, outmr, can be used to retrieve the interval
   and time remaining before the expiration of a previously created
   timer.  Thus the call signature is now:

   timerfd(int utmr, int clockid, int flags,
           struct itimerspec *utmr,
           struct itimerspec *outmr)

   The outmr argument:

   1) must be NULL for a new timer (ufd == -1).
   2) can be NULL if we are updating an existing
      timer (i.e., utmr != NULL), but we are not
      interested in the previous timer value.
   3) can be used to retrieve the time until next timer
      expiration, without changing the timer.
      (In this case, utmr == NULL and ufd >= 0.)

b) Make the 'clockid' immutable: it can only be set
   if 'ufd' is -1 -- that is, on the timerfd() call that
   first creates a timer.  (This is effectively
   the limitation that is imposed by POSIX timers: the
   clockid is specified when the timer is created with
   timer_create(), and can't be changed.)

   [In the 2.6.22 interface, the clockid of an existing
   timer can be changed with a further call to timerfd()
   that specifies the file descriptor of an existing timer.]

The use cases are as follows:

ufd = timerfd(-1, clockid, flags, utmr, NULL);
to create a new timer with given clockid, flags, and utmr (initial
expiration + interval).  outmr must be NULL.

ufd = timerfd(ufd, -1, flags, utmr, NULL);
to change the flags and timer settings of an existing timer.
(The clockid argument serves no purpose when modifying an
existing timer, and as I've implemented things, the argument
must be specified as -1 for an existing timer.)

ufd = timerfd(ufd, -1, flags, utmr, &outmr);
to change the flags and timer settings of an existing timer, and
retrieve the time until the next expiration of the timer (and
the associated interval).

ufd = timerfd(ufd, -1, 0, NULL, &outmr);
Return the time until the next expiration of the timer (and the
associated interval), without changing the existing timer settings.
The flags argument has no meaning in this case, and must be
specified as 0.

Other changes:

* I've added checks for various combinations of arguments values
  that could be deemed invalid; there is probably room for debate
  about whether we want all of these checks.  Comments in the
  patch describe these checks.

* Appropriate, and possibly even correct, changes made in fs/compat.c.

* Jan Engelhardt noted that a cast could be removed from Davide's
  original code; I've removed it.

The changes are correct as far as I've tested them.  I've attached a
test program.  Davide, could you subject this to further test please?

I'm off on holiday the day after tomorrow, back in two weeks, with
very limited computer access.  I may have time for another pass of
the code if comments come in over(euro)night, otherwise Davide may
be willing to clean up whatever I messed up...

Cheers,

Michael

diff -ruN linux-2.6.23-rc1-mm2-orig/fs/compat.c linux-2.6.23-rc1-mm2/fs/compat.c
--- linux-2.6.23-rc1-mm2-orig/fs/compat.c	2007-08-05 10:43:30.000000000 +0200
+++ linux-2.6.23-rc1-mm2/fs/compat.c	2007-08-07 22:08:15.000000000 +0200
@@ -2211,18 +2211,38 @@
 #ifdef CONFIG_TIMERFD

 asmlinkage long compat_sys_timerfd(int ufd, int clockid, int flags,
-				   const struct compat_itimerspec __user *utmr)
+			 const struct compat_itimerspec __user *utmr,
+			 struct compat_itimerspec __user *old_utmr)
 {
 	struct itimerspec t;
 	struct itimerspec __user *ut;
+	long ret;

-	if (get_compat_itimerspec(&t, utmr))
-		return -EFAULT;
-	ut = compat_alloc_user_space(sizeof(*ut));
-	if (copy_to_user(ut, &t, sizeof(t)))
-		return -EFAULT;
+	/*:mtk: Framework taken from compat_sys_mq_getsetattr() */

-	return sys_timerfd(ufd, clockid, flags, ut);
+	ut = compat_alloc_user_space(2 * sizeof(*ut));
+
+	if (utmr) {
+		if (get_compat_itimerspec(&t, utmr))
+			return -EFAULT;
+		if (copy_to_user(ut, &t, sizeof(t)))
+			return -EFAULT;
+	}
+
+	ret = sys_timerfd(ufd, clockid, flags,
+			utmr ? ut : NULL,
+			old_utmr ? ut + 1 : NULL);
+
+	if (ret)
+		return ret;
+
+	if (old_utmr) {
+	    	if (copy_from_user(&t, old_ut, sizeof(t)) ||
+		    put_compat_itimerspec(&t, old_utmr))
+			return -EFAULT;
+	}
+
+	return 0;
 }

 #endif /* CONFIG_TIMERFD */
diff -ruN linux-2.6.23-rc1-mm2-orig/fs/timerfd.c linux-2.6.23-rc1-mm2/fs/timerfd.c
--- linux-2.6.23-rc1-mm2-orig/fs/timerfd.c	2007-08-05 10:44:24.000000000 +0200
+++ linux-2.6.23-rc1-mm2/fs/timerfd.c	2007-08-09 22:11:25.000000000 +0200
@@ -2,6 +2,8 @@
  *  fs/timerfd.c
  *
  *  Copyright (C) 2007  Davide Libenzi <davidel@...ilserver.org>
+ *  and Copyright (C) 2007 Google, Inc.
+ *      (Author: Michael Kerrisk <mtk@...gle.com>)
  *
  *
  *  Thanks to Thomas Gleixner for code reviews and useful comments.
@@ -26,6 +28,12 @@
 	ktime_t tintv;
 	wait_queue_head_t wqh;
 	int expired;
+	int clockid;	/* Established when timer is created, then
+			   immutable */
+	int overrun;	/* Number of overruns for this timer so far,
+			   updated on calls to timerfd() that retrieve
+			   settings of an existing timer, reset to zero
+			   by timerfd_read() */
 };

 /*
@@ -61,6 +69,7 @@
 	hrtimer_init(&ctx->tmr, clockid, htmode);
 	ctx->tmr.expires = texp;
 	ctx->tmr.function = timerfd_tmrproc;
+	ctx->overrun = 0;
 	if (texp.tv64 != 0)
 		hrtimer_start(&ctx->tmr, texp, htmode);
 }
@@ -130,10 +139,11 @@
 			 * callback to avoid DoS attacks specifying a very
 			 * short timer period.
 			 */
-			ticks = (u64)
+			ticks = ctx->overrun +
 				hrtimer_forward(&ctx->tmr,
 						hrtimer_cb_get_time(&ctx->tmr),
 						ctx->tintv);
+			ctx->overrun = 0;
 			hrtimer_restart(&ctx->tmr);
 		} else
 			ticks = 1;
@@ -151,24 +161,69 @@
 };

 asmlinkage long sys_timerfd(int ufd, int clockid, int flags,
-			    const struct itimerspec __user *utmr)
+			const struct itimerspec __user *utmr,
+			struct itimerspec __user *old_utmr)
 {
 	int error;
 	struct timerfd_ctx *ctx;
 	struct file *file;
 	struct inode *inode;
-	struct itimerspec ktmr;
+	struct itimerspec ktmr, oktmr;

-	if (copy_from_user(&ktmr, utmr, sizeof(ktmr)))
-		return -EFAULT;
+	/*
+	 * Not sure if we really need the first check below -- it
+	 * relies on all clocks having a non-negative value.  That's
+	 * currently true, but I'm not sure that it is anywhere
+	 * documented as a requirement.
+	 * (The point of the check is that clockid has no meaning if
+	 * we are changing the settings of an existing timer
+	 * (i.e., ufd >= 0), so let's require it to be an otherwise
+	 * unused value.)
+	 */

-	if (clockid != CLOCK_MONOTONIC &&
-	    clockid != CLOCK_REALTIME)
+	if (ufd >= 0) {
+		if (clockid != -1)
+			return -EINVAL;
+	} else if (clockid != CLOCK_MONOTONIC && clockid != CLOCK_REALTIME)
+		return -EINVAL;
+
+	/*
+	 * Disallow any other bits in flags than those we implement.
+	 */
+	if ((flags & ~(TFD_TIMER_ABSTIME)) != 0)
+		return -EINVAL;
+
+	/*
+         * Not sure if this check is strictly necessary, except to stop
+	 * userspace trying to do something silly.  Flags only has meaning
+	 * if we are creating/modifying a timer.
+	 */
+	if (utmr == NULL && flags != 0)
 		return -EINVAL;
-	if (!timespec_valid(&ktmr.it_value) ||
-	    !timespec_valid(&ktmr.it_interval))
+
+	/*
+	 * If we are creating a new timer, then we must supply the setting
+	 * and there is no previous timer setting to retrieve.
+	 */
+	if (ufd == -1 && (utmr == NULL || old_utmr != NULL))
+		return -EINVAL;
+	
+	/*
+	 * Caller must provide a new timer setting, or ask for previous
+	 * setting, or both.
+	 */
+	if (utmr == NULL && old_utmr == NULL)
 		return -EINVAL;

+	if (utmr) {
+		if (copy_from_user(&ktmr, utmr, sizeof(ktmr)))
+			return -EFAULT;
+
+		if (!timespec_valid(&ktmr.it_value) ||
+		    !timespec_valid(&ktmr.it_interval))
+			return -EINVAL;
+	}
+
 	if (ufd == -1) {
 		ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);
 		if (!ctx)
@@ -176,6 +231,11 @@

 		init_waitqueue_head(&ctx->wqh);

+		/*
+		 * Save the clockid since we need it later if we modify
+		 * the timer.
+		 */
+		ctx->clockid = clockid;
 		timerfd_setup(ctx, clockid, flags, &ktmr);

 		/*
@@ -195,23 +255,61 @@
 			fput(file);
 			return -EINVAL;
 		}
-		/*
-		 * We need to stop the existing timer before reprogramming
-		 * it to the new values.
-		 */
-		for (;;) {
-			spin_lock_irq(&ctx->wqh.lock);
-			if (hrtimer_try_to_cancel(&ctx->tmr) >= 0)
-				break;
-			spin_unlock_irq(&ctx->wqh.lock);
-			cpu_relax();
+
+		if (old_utmr) {
+
+			/*
+			 * Retrieve interval and time remaining before
+			 * next expiration of timer.
+			 */
+
+			struct hrtimer *timer = &ctx->tmr;
+			ktime_t iv = ctx->tintv;
+			ktime_t now, remaining;
+
+			clockid = ctx->clockid;
+
+			memset(&oktmr, 0, sizeof(struct itimerspec));
+			if (iv.tv64)
+				oktmr.it_interval = ktime_to_timespec(iv);
+
+			now = timer->base->get_time();
+			ctx->overrun += hrtimer_forward(&ctx->tmr,
+					hrtimer_cb_get_time(&ctx->tmr),
+					ctx->tintv);
+			remaining = ktime_sub(timer->expires, now);
+			if (remaining.tv64 <= 0)
+				oktmr.it_value.tv_nsec = 1;
+			else
+				oktmr.it_value = ktime_to_timespec(remaining);
+
+			if (copy_to_user(old_utmr, &oktmr, sizeof (oktmr))) {
+				fput(file);
+				return -EFAULT;
+			}
 		}
-		/*
-		 * Re-program the timer to the new value ...
-		 */
-		timerfd_setup(ctx, clockid, flags, &ktmr);

-		spin_unlock_irq(&ctx->wqh.lock);
+		if (utmr) {
+			/*
+			 * Modify settings of existing timer.
+			 * We need to stop the existing timer before
+			 * reprogramming it to the new values.
+			 */
+			for (;;) {
+				spin_lock_irq(&ctx->wqh.lock);
+				if (hrtimer_try_to_cancel(&ctx->tmr) >= 0)
+					break;
+				spin_unlock_irq(&ctx->wqh.lock);
+				cpu_relax();
+			}
+
+			/*
+			 * Re-program the timer to the new value ...
+			 */
+			timerfd_setup(ctx, ctx->clockid, flags, &ktmr);
+
+			spin_unlock_irq(&ctx->wqh.lock);
+		}
 		fput(file);
 	}

@@ -222,4 +320,3 @@
 	kfree(ctx);
 	return error;
 }
-
diff -ruN linux-2.6.23-rc1-mm2-orig/include/linux/compat.h linux-2.6.23-rc1-mm2/include/linux/compat.h
--- linux-2.6.23-rc1-mm2-orig/include/linux/compat.h	2007-07-09 01:32:17.000000000 +0200
+++ linux-2.6.23-rc1-mm2/include/linux/compat.h	2007-08-05 17:46:33.000000000 +0200
@@ -265,7 +265,8 @@
 				const compat_sigset_t __user *sigmask,
                                 compat_size_t sigsetsize);
 asmlinkage long compat_sys_timerfd(int ufd, int clockid, int flags,
-				const struct compat_itimerspec __user *utmr);
+			const struct compat_itimerspec __user *utmr,
+			struct compat_itimerspec __user *old_utmr);

 #endif /* CONFIG_COMPAT */
 #endif /* _LINUX_COMPAT_H */
diff -ruN linux-2.6.23-rc1-mm2-orig/include/linux/syscalls.h linux-2.6.23-rc1-mm2/include/linux/syscalls.h
--- linux-2.6.23-rc1-mm2-orig/include/linux/syscalls.h	2007-08-05 10:44:32.000000000 +0200
+++ linux-2.6.23-rc1-mm2/include/linux/syscalls.h	2007-08-05 17:47:15.000000000 +0200
@@ -608,7 +608,8 @@
 asmlinkage long sys_getcpu(unsigned __user *cpu, unsigned __user *node, struct getcpu_cache __user *cache);
 asmlinkage long sys_signalfd(int ufd, sigset_t __user *user_mask, size_t sizemask);
 asmlinkage long sys_timerfd(int ufd, int clockid, int flags,
-			    const struct itimerspec __user *utmr);
+			const struct itimerspec __user *utmr,
+			struct itimerspec __user *old_utmr);
 asmlinkage long sys_eventfd(unsigned int count);
 asmlinkage long sys_fallocate(int fd, int mode, loff_t offset, loff_t len);



View attachment "new_timerfd_errors_test.c" of type "text/x-csrc" (9716 bytes)

Powered by blists - more mailing lists