Re: [patch] timerfd add flags check

[Michael Kerrisk <mtk.manpages@googlemail.com>]

On Tue, Feb 10, 2009 at 1:36 PM, Michael Kerrisk
<mtk.manpages@googlemail.com> wrote:
> On Tue, Feb 10, 2009 at 12:31 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.
>>
>> Michael said:
>> 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>
>>
>>
>> Signed-off-by: Davide Libenzi <davidel@xmailserver.org>
>
> Davide, one question: is the TFD_FLAGS_SET constant below needed?

Davide, Ping!  I see Andrew already took the patch, which seems to
define this unnecessary macro.

Cheers,

Michael

>
>> ---
>>  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 18:36:45.000000000 -0800
>> +++ linux-2.6.mod/fs/timerfd.c  2009-02-08 18:53: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 18:36:45.000000000 -0800
>> +++ linux-2.6.mod/include/linux/timerfd.h       2009-02-08 18:41:22.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
>



-- 
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