[PATCH] Fix discrepancy between VDSO based gettimeofday() and sys_gettimeofday().

[PATCH] Fix discrepancy between VDSO based gettimeofday() and sys_gettimeofday().

[Tony Breeds]

On platforms that copy sys_tz into the vdso (currently only x86_64, soon
to include powerpc), it is possible for the vdso to get out of sync if a
user calls (admittedly unusual) settimeofday(NULL, ptr).

This patch adds a hook for architectures that set CONFIG_GENERIC_TIME_VSYSCALL
to ensure when sys_tz is updated they can also updater their copy in the vdso.

Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: John Stultz <johnstul@us.ibm.com>
---

Only compile tested for IA64
Testcase at: http://bakeyournoodle.com/~tony/settimeofday-0.0/
I guess this is aimed at 2.6.24

 arch/ia64/kernel/time.c       |    5 +++++
 arch/x86_64/kernel/vsyscall.c |   11 ++++++++++-
 include/linux/clocksource.h   |    5 +++++
 kernel/time.c                 |    2 ++
 4 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/arch/ia64/kernel/time.c b/arch/ia64/kernel/time.c
index 98cfc90..2bb8421 100644
--- a/arch/ia64/kernel/time.c
+++ b/arch/ia64/kernel/time.c
@@ -371,6 +371,11 @@ ia64_setup_printk_clock(void)
 		ia64_printk_clock = ia64_itc_printk_clock;
 }
 
+/* IA64 doesn't cache the timezone */
+void update_vsyscall_tz(void)
+{
+}
+
 void update_vsyscall(struct timespec *wall, struct clocksource *c)
 {
         unsigned long flags;
diff --git a/arch/x86_64/kernel/vsyscall.c b/arch/x86_64/kernel/vsyscall.c
index 06c3494..4ff78b0 100644
--- a/arch/x86_64/kernel/vsyscall.c
+++ b/arch/x86_64/kernel/vsyscall.c
@@ -66,6 +66,16 @@ struct vsyscall_gtod_data __vsyscall_gtod_data __section_vsyscall_gtod_data =
 	.sysctl_enabled = 1,
 };
 
+void update_vsyscall_tz(void)
+{
+	unsigned long flags;
+
+	write_seqlock_irqsave(&vsyscall_gtod_data.lock, flags);
+	/* sys_tz has changed */
+	vsyscall_gtod_data.sys_tz = sys_tz;
+	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
+}
+
 void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
 {
 	unsigned long flags;
@@ -79,7 +89,6 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
 	vsyscall_gtod_data.clock.shift = clock->shift;
 	vsyscall_gtod_data.wall_time_sec = wall_time->tv_sec;
 	vsyscall_gtod_data.wall_time_nsec = wall_time->tv_nsec;
-	vsyscall_gtod_data.sys_tz = sys_tz;
 	vsyscall_gtod_data.wall_time_nsec = wall_time->tv_nsec;
 	vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
 	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 16ea337..107787a 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -221,10 +221,15 @@ extern void clocksource_resume(void);
 
 #ifdef CONFIG_GENERIC_TIME_VSYSCALL
 extern void update_vsyscall(struct timespec *ts, struct clocksource *c);
+extern void update_vsyscall_tz(void);
 #else
 static inline void update_vsyscall(struct timespec *ts, struct clocksource *c)
 {
 }
+
+static inline void update_vsyscall_tz(void)
+{
+}
 #endif
 
 #endif /* _LINUX_CLOCKSOURCE_H */
diff --git a/kernel/time.c b/kernel/time.c
index 2289a8d..9a4873e 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -30,6 +30,7 @@
 #include <linux/module.h>
 #include <linux/timex.h>
 #include <linux/capability.h>
+#include <linux/clocksource.h>
 #include <linux/errno.h>
 #include <linux/syscalls.h>
 #include <linux/security.h>
@@ -163,6 +164,7 @@ int do_sys_settimeofday(struct timespec *tv, struct timezone *tz)
 	if (tz) {
 		/* SMP safe, global irq locking makes it work. */
 		sys_tz = *tz;
+		update_vsyscall_tz();
 		if (firsttime) {
 			firsttime = 0;
 			if (!tv)
-- 
1.5.2.5

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: [PATCH] Fix discrepancy between VDSO based gettimeofday() and sys_gettimeofday().

[john stultz]

On Fri, 2007-08-31 at 13:41 +1000, Tony Breeds wrote:
> On platforms that copy sys_tz into the vdso (currently only x86_64, soon
> to include powerpc), it is possible for the vdso to get out of sync if a
> user calls (admittedly unusual) settimeofday(NULL, ptr).
> 
> This patch adds a hook for architectures that set CONFIG_GENERIC_TIME_VSYSCALL
> to ensure when sys_tz is updated they can also updater their copy in the vdso.

Ah. Yea, so after a sys_tz update, until the next timer tick, its
possible to get the stale value. Good catch.

One little comment below.

> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: John Stultz <johnstul@us.ibm.com>
> ---
> 
> Only compile tested for IA64
> Testcase at: http://bakeyournoodle.com/~tony/settimeofday-0.0/
> I guess this is aimed at 2.6.24
> 
>  arch/ia64/kernel/time.c       |    5 +++++
>  arch/x86_64/kernel/vsyscall.c |   11 ++++++++++-
>  include/linux/clocksource.h   |    5 +++++
>  kernel/time.c                 |    2 ++
>  4 files changed, 22 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/ia64/kernel/time.c b/arch/ia64/kernel/time.c
> index 98cfc90..2bb8421 100644
> --- a/arch/ia64/kernel/time.c
> +++ b/arch/ia64/kernel/time.c
> @@ -371,6 +371,11 @@ ia64_setup_printk_clock(void)
>  		ia64_printk_clock = ia64_itc_printk_clock;
>  }
> 
> +/* IA64 doesn't cache the timezone */
> +void update_vsyscall_tz(void)
> +{
> +}
> +
>  void update_vsyscall(struct timespec *wall, struct clocksource *c)
>  {
>          unsigned long flags;
> diff --git a/arch/x86_64/kernel/vsyscall.c b/arch/x86_64/kernel/vsyscall.c
> index 06c3494..4ff78b0 100644
> --- a/arch/x86_64/kernel/vsyscall.c
> +++ b/arch/x86_64/kernel/vsyscall.c
> @@ -66,6 +66,16 @@ struct vsyscall_gtod_data __vsyscall_gtod_data __section_vsyscall_gtod_data =
>  	.sysctl_enabled = 1,
>  };
> 
> +void update_vsyscall_tz(void)
> +{
> +	unsigned long flags;
> +
> +	write_seqlock_irqsave(&vsyscall_gtod_data.lock, flags);
> +	/* sys_tz has changed */
> +	vsyscall_gtod_data.sys_tz = sys_tz;
> +	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);
> +}
> +
>  void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
>  {
>  	unsigned long flags;
> @@ -79,7 +89,6 @@ void update_vsyscall(struct timespec *wall_time, struct clocksource *clock)
>  	vsyscall_gtod_data.clock.shift = clock->shift;
>  	vsyscall_gtod_data.wall_time_sec = wall_time->tv_sec;
>  	vsyscall_gtod_data.wall_time_nsec = wall_time->tv_nsec;
> -	vsyscall_gtod_data.sys_tz = sys_tz;
>  	vsyscall_gtod_data.wall_time_nsec = wall_time->tv_nsec;
>  	vsyscall_gtod_data.wall_to_monotonic = wall_to_monotonic;
>  	write_sequnlock_irqrestore(&vsyscall_gtod_data.lock, flags);


Hmm.. I might just extend update_vsyscall() so it takes a timezone and
then it only updates bits that have non-null pointers. Then call it from
sys_settimeofday(). This cuts down the functions each arch has to
implement, and also makes sure we get the full update each tick.

Alternatively update_vsyscall_tz() could live in timekeeping.c, grab the
appropriate locks and static data and call update_vsyscall(), also
minimizing the per-arch logic.

But your way is correct, and fixes a bug, so I won't stand in the way of
it.

-john