time/fs - file's time race with vgettimeofday
in [Kernel]
|
From: Jiri Olsa on 7 Jul 2010 06:50 On Tue, Jul 06, 2010 at 04:11:40PM -0700, john stultz wrote: > On Tue, 2010-07-06 at 16:03 -0700, john stultz wrote: > > On Fri, Jul 2, 2010 at 12:41 AM, Jiri Olsa <jolsa(a)redhat.com> wrote: > > > This issue was described in the BZ 244697 > > > > > > Time goes backward - gettimeofday() vs. rename() > > > https://bugzilla.redhat.com/show_bug.cgi?id=244697 > > > > > > > > > It's not just issue of the creat but few others like rename. > > > > > > > > > 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. > > > > > > > > > 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. > > > > I do worry that your patch will have too much of a performance hit. I > > think the right fix would be in vtime(). > > > > Test patch to follow shortly. > > So the following (untested) patch _should_ resolve this in mainline on > x86. Please let me know if it works for you. > > However, for older kernels, this patch won't be sufficient, as it > depends on update_vsyscall getting the time at the last tick in its > wall_time, and kernels that don't have the logarithmic accumulation code > & remove xtime_cache patches can't be fixed so easily. > > Once we get this upstream, let me know if you have any questions about > how to backport this to older kernels. > > thanks > -john > > > > [PATCH] x86: Fix vtime/file timestamp inconsistencies > > Due to vtime calling vgettimeofday(), its possible that an application > could call time();create("stuff",O_RDRW); only to see the file's > creation timestamp to be before the value returned by time. > > The proper fix is to make vtime use the same time value as > current_kernel_time() (which is exported via update_vsyscall) instead of > vgettime(). hm, this would be solution for the time() call. But I think the issue still stays for gettimeofday and clock_gettime (CLOCK_REALTIME/CLOCK_MONOTONIC) because they call vread from clocksource to get the real time. Thats where I'm not sure if this race is that "bad", compared either to performance hit or inaccuracy of user time calls.. which are possible solutions.. jirka > > > Signed-off-by: John Stultz <johnstul(a)us.ibm.com> > > > > diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c > index 1c0c6ab..ce9a4fa 100644 > --- a/arch/x86/kernel/vsyscall_64.c > +++ b/arch/x86/kernel/vsyscall_64.c > @@ -169,13 +169,18 @@ int __vsyscall(0) vgettimeofday(struct timeval * tv, struct timezone * tz) > * unlikely */ > time_t __vsyscall(1) vtime(time_t *t) > { > - struct timeval tv; > + unsigned seq; > time_t result; > if (unlikely(!__vsyscall_gtod_data.sysctl_enabled)) > return time_syscall(t); > > - vgettimeofday(&tv, NULL); > - result = tv.tv_sec; > + do { > + seq = read_seqbegin(&__vsyscall_gtod_data.lock); > + > + result = vsyscall_gtod_data.wall_time_sec; > + > + } while (read_seqretry(&__vsyscall_gtod_data.lock, seq)); > + > if (t) > *t = result; > return result; > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: john stultz on 7 Jul 2010 12:30 On Wed, 2010-07-07 at 12:47 +0200, Jiri Olsa wrote: > On Tue, Jul 06, 2010 at 04:11:40PM -0700, john stultz wrote: > > [PATCH] x86: Fix vtime/file timestamp inconsistencies > > > > Due to vtime calling vgettimeofday(), its possible that an application > > could call time();create("stuff",O_RDRW); only to see the file's > > creation timestamp to be before the value returned by time. > > > > The proper fix is to make vtime use the same time value as > > current_kernel_time() (which is exported via update_vsyscall) instead of > > vgettime(). > > hm, this would be solution for the time() call. > > But I think the issue still stays for gettimeofday and clock_gettime > (CLOCK_REALTIME/CLOCK_MONOTONIC) because they call vread from > clocksource to get the real time. > > Thats where I'm not sure if this race is that "bad", compared either > to performance hit or inaccuracy of user time calls.. which are possible > solutions.. Right, so as long as the file timestamps are tick-granular (like other tick-granular interfaces: current_kernel_time(), time(), CLOCK_REALTIME_COARSE) you will have the possibility of inconsistencies against the clocksource resolution interfaces (gettimeofday(), CLOCK_REALTIME, etc). But that is to be expected as a constraint of the granularity. So I don't really see this as an issue. Folks may want to increase the granularity of filesystem timestamps, but that will come at the possibly very expensive cost of reading the clocksource hardware (which can have different access latencies between architectures and even machines of the same arch). I suspect its not worth it. The concerning issue here that you pointed out are the inconsistencies could be seen between vsyscall time() and time() (or filesystem timestamps). That is a problem, and my patch should resolve that one. thanks -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Olsa on 7 Jul 2010 13:20 On Tue, Jul 06, 2010 at 04:11:40PM -0700, john stultz wrote: > On Tue, 2010-07-06 at 16:03 -0700, john stultz wrote: > > On Fri, Jul 2, 2010 at 12:41 AM, Jiri Olsa <jolsa(a)redhat.com> wrote: > > > This issue was described in the BZ 244697 > > > > > > Time goes backward - gettimeofday() vs. rename() > > > https://bugzilla.redhat.com/show_bug.cgi?id=244697 > > > > > > > > > It's not just issue of the creat but few others like rename. > > > > > > > > > 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. > > > > > > > > > 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. > > > > I do worry that your patch will have too much of a performance hit. I > > think the right fix would be in vtime(). > > > > Test patch to follow shortly. > > So the following (untested) patch _should_ resolve this in mainline on > x86. Please let me know if it works for you. > > However, for older kernels, this patch won't be sufficient, as it > depends on update_vsyscall getting the time at the last tick in its > wall_time, and kernels that don't have the logarithmic accumulation code > & remove xtime_cache patches can't be fixed so easily. > > Once we get this upstream, let me know if you have any questions about > how to backport this to older kernels. > > thanks > -john > > > > [PATCH] x86: Fix vtime/file timestamp inconsistencies > > Due to vtime calling vgettimeofday(), its possible that an application > could call time();create("stuff",O_RDRW); only to see the file's > creation timestamp to be before the value returned by time. > > The proper fix is to make vtime use the same time value as > current_kernel_time() (which is exported via update_vsyscall) instead of > vgettime(). > > > Signed-off-by: John Stultz <johnstul(a)us.ibm.com> > > > > diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c > index 1c0c6ab..ce9a4fa 100644 > --- a/arch/x86/kernel/vsyscall_64.c > +++ b/arch/x86/kernel/vsyscall_64.c > @@ -169,13 +169,18 @@ int __vsyscall(0) vgettimeofday(struct timeval * tv, struct timezone * tz) > * unlikely */ > time_t __vsyscall(1) vtime(time_t *t) > { > - struct timeval tv; > + unsigned seq; > time_t result; > if (unlikely(!__vsyscall_gtod_data.sysctl_enabled)) > return time_syscall(t); > > - vgettimeofday(&tv, NULL); > - result = tv.tv_sec; > + do { > + seq = read_seqbegin(&__vsyscall_gtod_data.lock); > + > + result = vsyscall_gtod_data.wall_time_sec; > + > + } while (read_seqretry(&__vsyscall_gtod_data.lock, seq)); > + > if (t) > *t = result; > return result; > > I think there's a typo in using "vsyscall_gtod_data" inside the vtime call it should be "__vsyscall_gtod_data" intead. attaching changed patch wbr, jirka --- [PATCH] x86: Fix vtime/file timestamp inconsistencies Due to vtime calling vgettimeofday(), its possible that an application could call time();create("stuff",O_RDRW); only to see the file's creation timestamp to be before the value returned by time. The proper fix is to make vtime use the same time value as current_kernel_time() (which is exported via update_vsyscall) instead of vgettime(). Signed-off-by: John Stultz <johnstul(a)us.ibm.com> Signed-off-by: Jiri Olsa <jolsa(a)redhat.com> --- diff --git a/arch/x86/kernel/vsyscall_64.c b/arch/x86/kernel/vsyscall_64.c index 1c0c6ab..dce0c3c 100644 --- a/arch/x86/kernel/vsyscall_64.c +++ b/arch/x86/kernel/vsyscall_64.c @@ -169,13 +169,18 @@ int __vsyscall(0) vgettimeofday(struct timeval * tv, struct timezone * tz) * unlikely */ time_t __vsyscall(1) vtime(time_t *t) { - struct timeval tv; + unsigned seq; time_t result; if (unlikely(!__vsyscall_gtod_data.sysctl_enabled)) return time_syscall(t); - vgettimeofday(&tv, NULL); - result = tv.tv_sec; + do { + seq = read_seqbegin(&__vsyscall_gtod_data.lock); + + result = __vsyscall_gtod_data.wall_time_sec; + + } while (read_seqretry(&__vsyscall_gtod_data.lock, seq)); + if (t) *t = result; return result; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: john stultz on 7 Jul 2010 13:30 On Wed, 2010-07-07 at 19:11 +0200, Jiri Olsa wrote: > On Tue, Jul 06, 2010 at 04:11:40PM -0700, john stultz wrote: > > - vgettimeofday(&tv, NULL); > > - result = tv.tv_sec; > > + do { > > + seq = read_seqbegin(&__vsyscall_gtod_data.lock); > > + > > + result = vsyscall_gtod_data.wall_time_sec; > > + > > + } while (read_seqretry(&__vsyscall_gtod_data.lock, seq)); > > + > > if (t) > > *t = result; > > return result; > > > > > > I think there's a typo in using "vsyscall_gtod_data" inside > the vtime call it should be "__vsyscall_gtod_data" intead. Quite right! Thanks for catching it! -john -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
First
|
Prev
|
Pages: 1 2 Prev: iwl3945: HARDWARE GONE?? Next: [PATCH UPDATED 4/4] async: use workqueue for worker pool |