[patch 1/2] timerfd add flags check

[patch 1/2] timerfd add flags check

[Davide Libenzi]

Like Michael requested, this patch adds a missing check for valid flags in 
timerfd_settime(), and make it return EINVAL in case some extra bits are 
set. If this is or not a -stable thing, it Greg decision.
Should be queued by Andrew for .30, unless -stable and Linus decide to 
get it before, like Michael suggested.



Signed-off-by: Davide Libenzi <davidel@xmailserver.org>


- Davide


---
 fs/timerfd.c            |   12 ++++++------
 include/linux/timerfd.h |   17 ++++++++++++++---
 2 files changed, 20 insertions(+), 9 deletions(-)

Index: linux-2.6.mod/fs/timerfd.c
===================================================================
--- linux-2.6.mod.orig/fs/timerfd.c	2009-02-08 14:51:12.000000000 -0800
+++ linux-2.6.mod/fs/timerfd.c	2009-02-08 14:55:32.000000000 -0800
@@ -186,10 +186,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
 	BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
 	BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
 
-	if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
-		return -EINVAL;
-	if (clockid != CLOCK_MONOTONIC &&
-	    clockid != CLOCK_REALTIME)
+	if ((flags & ~TFD_CREATE_FLAGS) ||
+	    (clockid != CLOCK_MONOTONIC &&
+	     clockid != CLOCK_REALTIME))
 		return -EINVAL;
 
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
@@ -201,7 +200,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
 	hrtimer_init(&ctx->tmr, clockid, HRTIMER_MODE_ABS);
 
 	ufd = anon_inode_getfd("[timerfd]", &timerfd_fops, ctx,
-			       flags & (O_CLOEXEC | O_NONBLOCK));
+			       flags & TFD_SHARED_FCNTL_FLAGS);
 	if (ufd < 0)
 		kfree(ctx);
 
@@ -219,7 +218,8 @@ SYSCALL_DEFINE4(timerfd_settime, int, uf
 	if (copy_from_user(&ktmr, utmr, sizeof(ktmr)))
 		return -EFAULT;
 
-	if (!timespec_valid(&ktmr.it_value) ||
+	if ((flags & ~TFD_SETTIME_FLAGS) ||
+	    !timespec_valid(&ktmr.it_value) ||
 	    !timespec_valid(&ktmr.it_interval))
 		return -EINVAL;
 
Index: linux-2.6.mod/include/linux/timerfd.h
===================================================================
--- linux-2.6.mod.orig/include/linux/timerfd.h	2009-02-08 14:51:12.000000000 -0800
+++ linux-2.6.mod/include/linux/timerfd.h	2009-02-08 14:53:00.000000000 -0800
@@ -11,13 +11,24 @@
 /* For O_CLOEXEC and O_NONBLOCK */
 #include <linux/fcntl.h>
 
-/* Flags for timerfd_settime.  */
+/*
+ * CAREFUL: Check include/asm-generic/fcntl.h when defining
+ * new flags, since they might collide with O_* ones. We want
+ * to re-use O_* flags that couldn't possibly have a meaning
+ * from eventfd, in order to leave a free define-space for
+ * shared O_* flags.
+ */
 #define TFD_TIMER_ABSTIME (1 << 0)
-
-/* Flags for timerfd_create.  */
 #define TFD_CLOEXEC O_CLOEXEC
 #define TFD_NONBLOCK O_NONBLOCK
 
+#define TFD_SHARED_FCNTL_FLAGS (TFD_CLOEXEC | TFD_NONBLOCK)
+/* Flags for timerfd_create.  */
+#define TFD_CREATE_FLAGS TFD_SHARED_FCNTL_FLAGS
+/* Flags for timerfd_settime.  */
+#define TFD_SETTIME_FLAGS TFD_TIMER_ABSTIME
+#define TFD_FLAGS_SET (TFD_SHARED_FCNTL_FLAGS | TFD_TIMER_ABSTIME)
+
 
 #endif /* _LINUX_TIMERFD_H */
 

Re: [patch 1/2] timerfd add flags check

[Michael Kerrisk]

On Mon, Feb 9, 2009 at 12:23 PM, Davide Libenzi <davidel@xmailserver.org> wrote:
> Like Michael requested, this patch adds a missing check for valid flags in
> timerfd_settime(), and make it return EINVAL in case some extra bits are
> set. If this is or not a -stable thing, it Greg decision.
> Should be queued by Andrew for .30, unless -stable and Linus decide to
> get it before, like Michael suggested.

If this is to be any use to userland apps that want to check flag
support (perhaps it is too late already), then the sooner we get it
into the kernel the better: 2.6.29 would be good; earlier stables as
well would be even better.

Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>

> - Davide
>
>
> ---
>  fs/timerfd.c            |   12 ++++++------
>  include/linux/timerfd.h |   17 ++++++++++++++---
>  2 files changed, 20 insertions(+), 9 deletions(-)
>
> Index: linux-2.6.mod/fs/timerfd.c
> ===================================================================
> --- linux-2.6.mod.orig/fs/timerfd.c     2009-02-08 14:51:12.000000000 -0800
> +++ linux-2.6.mod/fs/timerfd.c  2009-02-08 14:55:32.000000000 -0800
> @@ -186,10 +186,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
>        BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
>        BUILD_BUG_ON(TFD_NONBLOCK != O_NONBLOCK);
>
> -       if (flags & ~(TFD_CLOEXEC | TFD_NONBLOCK))
> -               return -EINVAL;
> -       if (clockid != CLOCK_MONOTONIC &&
> -           clockid != CLOCK_REALTIME)
> +       if ((flags & ~TFD_CREATE_FLAGS) ||
> +           (clockid != CLOCK_MONOTONIC &&
> +            clockid != CLOCK_REALTIME))
>                return -EINVAL;
>
>        ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> @@ -201,7 +200,7 @@ SYSCALL_DEFINE2(timerfd_create, int, clo
>        hrtimer_init(&ctx->tmr, clockid, HRTIMER_MODE_ABS);
>
>        ufd = anon_inode_getfd("[timerfd]", &timerfd_fops, ctx,
> -                              flags & (O_CLOEXEC | O_NONBLOCK));
> +                              flags & TFD_SHARED_FCNTL_FLAGS);
>        if (ufd < 0)
>                kfree(ctx);
>
> @@ -219,7 +218,8 @@ SYSCALL_DEFINE4(timerfd_settime, int, uf
>        if (copy_from_user(&ktmr, utmr, sizeof(ktmr)))
>                return -EFAULT;
>
> -       if (!timespec_valid(&ktmr.it_value) ||
> +       if ((flags & ~TFD_SETTIME_FLAGS) ||
> +           !timespec_valid(&ktmr.it_value) ||
>            !timespec_valid(&ktmr.it_interval))
>                return -EINVAL;
>
> Index: linux-2.6.mod/include/linux/timerfd.h
> ===================================================================
> --- linux-2.6.mod.orig/include/linux/timerfd.h  2009-02-08 14:51:12.000000000 -0800
> +++ linux-2.6.mod/include/linux/timerfd.h       2009-02-08 14:53:00.000000000 -0800
> @@ -11,13 +11,24 @@
>  /* For O_CLOEXEC and O_NONBLOCK */
>  #include <linux/fcntl.h>
>
> -/* Flags for timerfd_settime.  */
> +/*
> + * CAREFUL: Check include/asm-generic/fcntl.h when defining
> + * new flags, since they might collide with O_* ones. We want
> + * to re-use O_* flags that couldn't possibly have a meaning
> + * from eventfd, in order to leave a free define-space for
> + * shared O_* flags.
> + */
>  #define TFD_TIMER_ABSTIME (1 << 0)
> -
> -/* Flags for timerfd_create.  */
>  #define TFD_CLOEXEC O_CLOEXEC
>  #define TFD_NONBLOCK O_NONBLOCK
>
> +#define TFD_SHARED_FCNTL_FLAGS (TFD_CLOEXEC | TFD_NONBLOCK)
> +/* Flags for timerfd_create.  */
> +#define TFD_CREATE_FLAGS TFD_SHARED_FCNTL_FLAGS
> +/* Flags for timerfd_settime.  */
> +#define TFD_SETTIME_FLAGS TFD_TIMER_ABSTIME
> +#define TFD_FLAGS_SET (TFD_SHARED_FCNTL_FLAGS | TFD_TIMER_ABSTIME)
> +
>
>  #endif /* _LINUX_TIMERFD_H */
>
>



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [patch 1/2] timerfd add flags check

[Michael Kerrisk]

On Mon, Feb 9, 2009 at 12:23 PM, Davide Libenzi <davidel@xmailserver.org> wrote:
> Like Michael requested, this patch adds a missing check for valid flags in
> timerfd_settime(), and make it return EINVAL in case some extra bits are
> set. If this is or not a -stable thing, it Greg decision.
> Should be queued by Andrew for .30, unless -stable and Linus decide to
> get it before, like Michael suggested.

When these two patches are applied against .29-rc3:

$ make
[...]
  CC      fs/timerfd.o
fs/timerfd.c: In function 'sys_timerfd_create':
fs/timerfd.c:190: error: implicit declaration of function 'invalid_clockid'
make[1]: *** [fs/timerfd.o] Error 1
make: *** [fs] Error 2


Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
man-pages online: http://www.kernel.org/doc/man-pages/online_pages.html
Found a bug? http://www.kernel.org/doc/man-pages/reporting_bugs.html

Re: [patch 1/2] timerfd add flags check

[Davide Libenzi]

On Mon, 9 Feb 2009, Michael Kerrisk wrote:

> On Mon, Feb 9, 2009 at 12:23 PM, Davide Libenzi <davidel@xmailserver.org> wrote:
> > Like Michael requested, this patch adds a missing check for valid flags in
> > timerfd_settime(), and make it return EINVAL in case some extra bits are
> > set. If this is or not a -stable thing, it Greg decision.
> > Should be queued by Andrew for .30, unless -stable and Linus decide to
> > get it before, like Michael suggested.
> 
> When these two patches are applied against .29-rc3:
> 
> $ make
> [...]
>   CC      fs/timerfd.o
> fs/timerfd.c: In function 'sys_timerfd_create':
> fs/timerfd.c:190: error: implicit declaration of function 'invalid_clockid'
> make[1]: *** [fs/timerfd.o] Error 1
> make: *** [fs] Error 2

Yeah, I missed the posix-timers.h inclusion in timerfd.c, when I split the 
first patch into two separate ones. Will re-send tomorrow.


- Davide