lkml.org 
[lkml]   [2010]   [Jul]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] time/fs - file's time race with vgettimeofday
    In no way I can review this patch, but I am curious and have the questions.
    Also, I think it makes sense to cc the fs/ developers, I've added Al.

    On 07/02, Jiri Olsa wrote:
    >
    > there's a race among calling gettimeofday(2) and a file's time
    > updates. Following test program expose the race.
    >
    > run it in the while loop
    > while [ 1 ]; do ./test1 || break; done
    >
    > --- SNIP ---
    > #include <stdio.h>
    > #include <stdlib.h>
    > #include <fcntl.h>
    >
    > int main (void)
    > {
    > struct stat st;
    > struct timeval tv;
    >
    > unlink("./file");
    >
    > gettimeofday(&tv, NULL);
    >
    > if (-1 == creat("./file", O_RDWR)) {
    > perror("creat");
    > return -1;
    > }
    >
    > if (stat("./file", &st) != 0) {
    > perror("stat");
    > return -1;
    > }
    >
    > printf("USER gtod: %ld\n", (long)tv.tv_sec);
    > printf("USER file: %ld.%09u\n",
    > (long) st.st_mtime,
    > (unsigned) st.st_mtim.tv_nsec);
    >
    > return tv.tv_sec <= st.st_mtime ? 0 : -1;
    > }

    Interesting. To the point, I actually compiled this code and yes,
    it triggers the problem on ext3 ;)

    > The following patch will prevent the race by adding the
    > CURRENT_TIME_SEC_REAL macro, which will return seconds from
    > the getnstimeofday call, ensuring it's computed on current tick.
    > It fixes the 'creat' case for ext4.

    What about other filesystems? Perhaps it makes sense to change
    CURRENT_TIME_SEC instead of adding CURRENT_TIME_SEC_REAL?

    Once again, I am asking. It is not that I suggest to change your patch.



    But there is something I can't understand.

    We have

    #define CURRENT_TIME_SEC ((struct timespec) { get_seconds(), 0 })

    and your patch adds

    #define CURRENT_TIME_SEC_REAL ((struct timespec) { get_seconds_real(), 0 })

    which fixes the problem for ext4.

    But, get_seconds() is also used by sys_time(), and we should have the
    same problem with another trivial test-case:

    #include <stdio.h>
    #include <sys/time.h>
    #include <time.h>

    int main(void)
    {
    struct timeval tv;
    int sec;

    do {
    gettimeofday(&tv, NULL);
    sec = time(NULL);
    } while (tv.tv_sec <= sec);

    printf("gtod: %ld.%06ld\n", tv.tv_sec, tv.tv_usec);
    printf("time: %d.000000\n", sec);
    return 0;
    }

    However, this test-case can't trigger the problem. Confused.

    Oleg.

    >
    > I'm not sure how much trouble is having this race unfixed compared
    > to the performance impact the fix might have. Maybe there're
    > better ways to fix this.
    >
    >
    > thanks for any ideas,
    > jirka
    >
    >
    >
    > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
    > ---
    > diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c
    > index 938dbc7..7a0a2fc 100644
    > --- a/fs/ext2/ialloc.c
    > +++ b/fs/ext2/ialloc.c
    > @@ -558,7 +558,7 @@ got:
    >
    > inode->i_ino = ino;
    > inode->i_blocks = 0;
    > - inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC;
    > + inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME_SEC_REAL;
    > memset(ei->i_data, 0, sizeof(ei->i_data));
    > ei->i_flags =
    > ext2_mask_flags(mode, EXT2_I(dir)->i_flags & EXT2_FL_INHERITED);
    > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
    > index 19a4de5..2c2925c 100644
    > --- a/fs/ext4/ext4.h
    > +++ b/fs/ext4/ext4.h
    > @@ -1157,7 +1157,7 @@ static inline struct ext4_inode_info *EXT4_I(struct inode *inode)
    > static inline struct timespec ext4_current_time(struct inode *inode)
    > {
    > return (inode->i_sb->s_time_gran < NSEC_PER_SEC) ?
    > - current_fs_time(inode->i_sb) : CURRENT_TIME_SEC;
    > + current_fs_time(inode->i_sb) : CURRENT_TIME_SEC_REAL;
    > }
    >
    > static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)
    > diff --git a/include/linux/time.h b/include/linux/time.h
    > index ea3559f..f390687 100644
    > --- a/include/linux/time.h
    > +++ b/include/linux/time.h
    > @@ -109,12 +109,14 @@ void timekeeping_init(void);
    > extern int timekeeping_suspended;
    >
    > unsigned long get_seconds(void);
    > +unsigned long get_seconds_real(void);
    > struct timespec current_kernel_time(void);
    > struct timespec __current_kernel_time(void); /* does not hold xtime_lock */
    > struct timespec get_monotonic_coarse(void);
    >
    > #define CURRENT_TIME (current_kernel_time())
    > #define CURRENT_TIME_SEC ((struct timespec) { get_seconds(), 0 })
    > +#define CURRENT_TIME_SEC_REAL ((struct timespec) { get_seconds_real(), 0 })
    >
    > /* Some architectures do not supply their own clocksource.
    > * This is mainly the case in architectures that get their
    > diff --git a/kernel/time.c b/kernel/time.c
    > index 848b1c2..ce10dae 100644
    > --- a/kernel/time.c
    > +++ b/kernel/time.c
    > @@ -227,7 +227,8 @@ SYSCALL_DEFINE1(adjtimex, struct timex __user *, txc_p)
    > */
    > struct timespec current_fs_time(struct super_block *sb)
    > {
    > - struct timespec now = current_kernel_time();
    > + struct timespec now;
    > + getnstimeofday(&now);
    > return timespec_trunc(now, sb->s_time_gran);
    > }
    > EXPORT_SYMBOL(current_fs_time);
    > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
    > index caf8d4d..7ebfe23 100644
    > --- a/kernel/time/timekeeping.c
    > +++ b/kernel/time/timekeeping.c
    > @@ -897,6 +897,14 @@ unsigned long get_seconds(void)
    > }
    > EXPORT_SYMBOL(get_seconds);
    >
    > +unsigned long get_seconds_real(void)
    > +{
    > + struct timespec ts;
    > + getnstimeofday(&ts);
    > + return ts.tv_sec;
    > +}
    > +EXPORT_SYMBOL(get_seconds_real);
    > +
    > struct timespec __current_kernel_time(void)
    > {
    > return xtime;



    \
     
     \ /
      Last update: 2010-07-02 18:19    [W:0.044 / U:124.212 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site