[PATCH] use a stable clock reference in vdso vgetns

[PATCH] use a stable clock reference in vdso vgetns

[Glauber Costa]

When using vdso services for clock_gettime, we test for the ability
of a fine-grained measurement through the existance of a vread() function.

However, from the time we test it, to the time we use it, vread() reference
may not be valid anymore. It happens, for example, when we change the current
clocksource from one that provides vread (say tsc) to one that lacks it
(say acpi_pm), in the middle of clock_gettime routine.

seqlock does not really protect us, since readers here won't stop the writers
to change references. The proposed solution is to grab a copy of the clock
structure early, and use it as a stable reference onwards.

Since we don't free parts of memory associated with old clocksources, we
merely stop using it for a while, this is a safe thing to do.

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: Avi Kivity <avi@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@elte.hu>
CC: H. Peter Anvin <hpa@zytor.com>
CC: Peter Zijlstra <peterz@infradead.org>
---
 arch/x86/include/asm/vgtod.h   |    2 +-
 arch/x86/vdso/vclock_gettime.c |   51 +++++++++++++++++++++++++++------------
 2 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/vgtod.h b/arch/x86/include/asm/vgtod.h
index 3d61e20..7f813bc 100644
--- a/arch/x86/include/asm/vgtod.h
+++ b/arch/x86/include/asm/vgtod.h
@@ -13,7 +13,7 @@ struct vsyscall_gtod_data {
 
 	int		sysctl_enabled;
 	struct timezone sys_tz;
-	struct { /* extract of a clocksource struct */
+	struct vclock { /* extract of a clocksource struct */
 		cycle_t (*vread)(void);
 		cycle_t	cycle_last;
 		cycle_t	mask;
diff --git a/arch/x86/vdso/vclock_gettime.c b/arch/x86/vdso/vclock_gettime.c
index ee55754..c82ade0 100644
--- a/arch/x86/vdso/vclock_gettime.c
+++ b/arch/x86/vdso/vclock_gettime.c
@@ -34,23 +34,26 @@ notrace static long vdso_fallback_gettime(long clock, struct timespec *ts)
 	return ret;
 }
 
-notrace static inline long vgetns(void)
+/*
+ * The clock structure must be provided from the outside, instead of accessed
+ * from gtod. The clock can have changed, leading to an invalid vread by the
+ * time we get here. All other values must also be consistent with vread result
+ */
+notrace static inline long vgetns(struct vclock *clock)
 {
 	long v;
-	cycles_t (*vread)(void);
-	vread = gtod->clock.vread;
-	v = (vread() - gtod->clock.cycle_last) & gtod->clock.mask;
-	return (v * gtod->clock.mult) >> gtod->clock.shift;
+	v = (clock->vread() - clock->cycle_last) & clock->mask;
+	return (v * clock->mult) >> clock->shift;
 }
 
-notrace static noinline int do_realtime(struct timespec *ts)
+notrace static noinline int do_realtime(struct timespec *ts, struct vclock *clock)
 {
 	unsigned long seq, ns;
 	do {
 		seq = read_seqbegin(&gtod->lock);
 		ts->tv_sec = gtod->wall_time_sec;
 		ts->tv_nsec = gtod->wall_time_nsec;
-		ns = vgetns();
+		ns = vgetns(clock);
 	} while (unlikely(read_seqretry(&gtod->lock, seq)));
 	timespec_add_ns(ts, ns);
 	return 0;
@@ -72,13 +75,13 @@ vset_normalized_timespec(struct timespec *ts, long sec, long nsec)
 	ts->tv_nsec = nsec;
 }
 
-notrace static noinline int do_monotonic(struct timespec *ts)
+notrace static noinline int do_monotonic(struct timespec *ts, struct vclock *clock)
 {
 	unsigned long seq, ns, secs;
 	do {
 		seq = read_seqbegin(&gtod->lock);
 		secs = gtod->wall_time_sec;
-		ns = gtod->wall_time_nsec + vgetns();
+		ns = gtod->wall_time_nsec + vgetns(clock);
 		secs += gtod->wall_to_monotonic.tv_sec;
 		ns += gtod->wall_to_monotonic.tv_nsec;
 	} while (unlikely(read_seqretry(&gtod->lock, seq)));
@@ -113,21 +116,29 @@ notrace static noinline int do_monotonic_coarse(struct timespec *ts)
 
 notrace int __vdso_clock_gettime(clockid_t clock, struct timespec *ts)
 {
-	if (likely(gtod->sysctl_enabled))
+	if (likely(gtod->sysctl_enabled)) {
+		struct vclock clk;
+		unsigned long seq;
+		do {
+			seq = read_seqbegin(&gtod->lock);
+			memcpy(&clk, &gtod->clock, sizeof(clk));
+		} while (unlikely(read_seqretry(&gtod->lock, seq)));
+
 		switch (clock) {
 		case CLOCK_REALTIME:
-			if (likely(gtod->clock.vread))
-				return do_realtime(ts);
+			if (likely(clk.vread))
+				return do_realtime(ts, &clk);
 			break;
 		case CLOCK_MONOTONIC:
-			if (likely(gtod->clock.vread))
-				return do_monotonic(ts);
+			if (likely(clk.vread))
+				return do_monotonic(ts, &clk);
 			break;
 		case CLOCK_REALTIME_COARSE:
 			return do_realtime_coarse(ts);
 		case CLOCK_MONOTONIC_COARSE:
 			return do_monotonic_coarse(ts);
 		}
+	}
 	return vdso_fallback_gettime(clock, ts);
 }
 int clock_gettime(clockid_t, struct timespec *)
@@ -136,12 +147,20 @@ int clock_gettime(clockid_t, struct timespec *)
 notrace int __vdso_gettimeofday(struct timeval *tv, struct timezone *tz)
 {
 	long ret;
-	if (likely(gtod->sysctl_enabled && gtod->clock.vread)) {
+	struct vclock clock;
+	unsigned long seq;
+
+	do {
+		seq = read_seqbegin(&gtod->lock);
+		memcpy(&clock, &gtod->clock, sizeof(clock));
+	} while (unlikely(read_seqretry(&gtod->lock, seq)));
+
+	if (likely(gtod->sysctl_enabled && clock.vread)) {
 		if (likely(tv != NULL)) {
 			BUILD_BUG_ON(offsetof(struct timeval, tv_usec) !=
 				     offsetof(struct timespec, tv_nsec) ||
 				     sizeof(*tv) != sizeof(struct timespec));
-			do_realtime((struct timespec *)tv);
+			do_realtime((struct timespec *)tv, &clock);
 			tv->tv_usec /= 1000;
 		}
 		if (unlikely(tz != NULL)) {
-- 
1.7.0.1

Re: [PATCH] use a stable clock reference in vdso vgetns

[john stultz]

On Fri, Oct 1, 2010 at 6:09 AM, Glauber Costa <glommer@redhat.com> wrote:
> When using vdso services for clock_gettime, we test for the ability
> of a fine-grained measurement through the existance of a vread() function.
>
> However, from the time we test it, to the time we use it, vread() reference
> may not be valid anymore. It happens, for example, when we change the current
> clocksource from one that provides vread (say tsc) to one that lacks it
> (say acpi_pm), in the middle of clock_gettime routine.
>
> seqlock does not really protect us, since readers here won't stop the writers
> to change references. The proposed solution is to grab a copy of the clock
> structure early, and use it as a stable reference onwards.

Ah. Good find! The fix looks reasonable to me. However, its likely the
similar code in arch/x86/kernel/vsyscall_64.c will need a similar fix.

Awhile back there was some motivation to merge the two vdso/vsyscall
implementations to avoid the duplication, but my memory is failing on
why that didn't happen. I feel like it had to do with complication
with the way the two implementations are mapped out to userland. Even
so, it seems a shared forced inline method would resolve the issue, so
maybe it just fell off the todo list?

thanks
-john

Re: [PATCH] use a stable clock reference in vdso vgetns

[Glauber Costa]

On Fri, Oct 01, 2010 at 10:49:53AM -0700, john stultz wrote:
> On Fri, Oct 1, 2010 at 6:09 AM, Glauber Costa <glommer@redhat.com> wrote:
> > When using vdso services for clock_gettime, we test for the ability
> > of a fine-grained measurement through the existance of a vread() function.
> >
> > However, from the time we test it, to the time we use it, vread() reference
> > may not be valid anymore. It happens, for example, when we change the current
> > clocksource from one that provides vread (say tsc) to one that lacks it
> > (say acpi_pm), in the middle of clock_gettime routine.
> >
> > seqlock does not really protect us, since readers here won't stop the writers
> > to change references. The proposed solution is to grab a copy of the clock
> > structure early, and use it as a stable reference onwards.
> 
> Ah. Good find! The fix looks reasonable to me. However, its likely the
> similar code in arch/x86/kernel/vsyscall_64.c will need a similar fix.
> 
> Awhile back there was some motivation to merge the two vdso/vsyscall
> implementations to avoid the duplication, but my memory is failing on
> why that didn't happen. I feel like it had to do with complication
> with the way the two implementations are mapped out to userland. Even
> so, it seems a shared forced inline method would resolve the issue, so
> maybe it just fell off the todo list?

Actually vsyscall updates seem to be covered by update_vsyscall() already.
I tried hard and can't reproduce this behaviour.

Now that I am thinking, maybe we could come up something similar to vsyscall?

If we start by making vsyscall data point to the same object, we may get it
for free. Or am I missing something ?

Re: [PATCH] use a stable clock reference in vdso vgetns

[Glauber Costa]

On Fri, Oct 01, 2010 at 10:49:53AM -0700, john stultz wrote:
> On Fri, Oct 1, 2010 at 6:09 AM, Glauber Costa <glommer@redhat.com> wrote:
> > When using vdso services for clock_gettime, we test for the ability
> > of a fine-grained measurement through the existance of a vread() function.
> >
> > However, from the time we test it, to the time we use it, vread() reference
> > may not be valid anymore. It happens, for example, when we change the current
> > clocksource from one that provides vread (say tsc) to one that lacks it
> > (say acpi_pm), in the middle of clock_gettime routine.
> >
> > seqlock does not really protect us, since readers here won't stop the writers
> > to change references. The proposed solution is to grab a copy of the clock
> > structure early, and use it as a stable reference onwards.
> 
> Ah. Good find! The fix looks reasonable to me. However, its likely the
> similar code in arch/x86/kernel/vsyscall_64.c will need a similar fix.
> 
> Awhile back there was some motivation to merge the two vdso/vsyscall
> implementations to avoid the duplication, but my memory is failing on
> why that didn't happen. I feel like it had to do with complication
> with the way the two implementations are mapped out to userland. Even
> so, it seems a shared forced inline method would resolve the issue, so
> maybe it just fell off the todo list?
Ok, I just stated that vsyscall is safe because of update_vsyscall. This is
just not the case. Update vsyscall will update the timer structures but leave
us with the problem that it will do it regardless of any readers. What makes
vsyscall "safe", is this:

                vread = __vsyscall_gtod_data.clock.vread; 
                ^
                |--- saves reference to vread                    uses --.
                if (unlikely(!__vsyscall_gtod_data.sysctl_enabled || !vread)) {
                        gettimeofday(tv,NULL);
                        return;
                }

                now = vread(); <=== uses again

This was, actually, my first idea on how to solve the problem for vdso. If we are
always reading this inside the seqlock, it should be okay.

In the way vdso's clock_gettime is structured, however, it would involve passing
down the expected seq + vread pointer down the function call chain, or wrapping
all callees inside the lock. Honestly, I believe the solution I posted is cleaner
than both, even having a slight difference wrt vsyscall's today implementation.

What do you think ?

Re: [PATCH] use a stable clock reference in vdso vgetns

[Glauber Costa]

On Fri, Oct 01, 2010 at 10:49:53AM -0700, john stultz wrote:
> On Fri, Oct 1, 2010 at 6:09 AM, Glauber Costa <glommer@redhat.com> wrote:
> > When using vdso services for clock_gettime, we test for the ability
> > of a fine-grained measurement through the existance of a vread() function.
> >
> > However, from the time we test it, to the time we use it, vread() reference
> > may not be valid anymore. It happens, for example, when we change the current
> > clocksource from one that provides vread (say tsc) to one that lacks it
> > (say acpi_pm), in the middle of clock_gettime routine.
> >
> > seqlock does not really protect us, since readers here won't stop the writers
> > to change references. The proposed solution is to grab a copy of the clock
> > structure early, and use it as a stable reference onwards.
> 
> Ah. Good find! The fix looks reasonable to me. However, its likely the
> similar code in arch/x86/kernel/vsyscall_64.c will need a similar fix.
> 
> Awhile back there was some motivation to merge the two vdso/vsyscall
> implementations to avoid the duplication, but my memory is failing on
> why that didn't happen. I feel like it had to do with complication
> with the way the two implementations are mapped out to userland. Even
> so, it seems a shared forced inline method would resolve the issue, so
> maybe it just fell off the todo list?
News here?

Re: [PATCH] use a stable clock reference in vdso vgetns

[john stultz]

On Wed, 2010-10-13 at 11:07 -0300, Glauber Costa wrote:
> On Fri, Oct 01, 2010 at 10:49:53AM -0700, john stultz wrote:
> > On Fri, Oct 1, 2010 at 6:09 AM, Glauber Costa <glommer@redhat.com> wrote:
> > > When using vdso services for clock_gettime, we test for the ability
> > > of a fine-grained measurement through the existance of a vread() function.
> > >
> > > However, from the time we test it, to the time we use it, vread() reference
> > > may not be valid anymore. It happens, for example, when we change the current
> > > clocksource from one that provides vread (say tsc) to one that lacks it
> > > (say acpi_pm), in the middle of clock_gettime routine.
> > >
> > > seqlock does not really protect us, since readers here won't stop the writers
> > > to change references. The proposed solution is to grab a copy of the clock
> > > structure early, and use it as a stable reference onwards.
> > 
> > Ah. Good find! The fix looks reasonable to me. However, its likely the
> > similar code in arch/x86/kernel/vsyscall_64.c will need a similar fix.
> > 
> > Awhile back there was some motivation to merge the two vdso/vsyscall
> > implementations to avoid the duplication, but my memory is failing on
> > why that didn't happen. I feel like it had to do with complication
> > with the way the two implementations are mapped out to userland. Even
> > so, it seems a shared forced inline method would resolve the issue, so
> > maybe it just fell off the todo list?
> News here?

Errr.. Sorry, are you waiting for me to implement this? Or did you want
a comment on your earlier mail? (Apologies, I've been a bit scattered
here).

I think trying to unify the two implementations would be nice if you're
up to trying, but if not, go ahead and push your patch and that will fix
the bug until I can get around to looking at the unification.

I don't think the unification would be *that* complicated, since its
just a matter of grabbing the right reference to the mapped out data
(either the __vsyscall_gtod_data or the vdso_vsyscall_gtod_data) and
then just passing a pointer to the right reference to the common inlined
functions). 


thanks
-john

Re: [PATCH] use a stable clock reference in vdso vgetns

[Glauber Costa]

On Wed, Oct 13, 2010 at 06:36:55PM -0700, john stultz wrote:
> On Wed, 2010-10-13 at 11:07 -0300, Glauber Costa wrote:
> > On Fri, Oct 01, 2010 at 10:49:53AM -0700, john stultz wrote:
> > > On Fri, Oct 1, 2010 at 6:09 AM, Glauber Costa <glommer@redhat.com> wrote:
> > > > When using vdso services for clock_gettime, we test for the ability
> > > > of a fine-grained measurement through the existance of a vread() function.
> > > >
> > > > However, from the time we test it, to the time we use it, vread() reference
> > > > may not be valid anymore. It happens, for example, when we change the current
> > > > clocksource from one that provides vread (say tsc) to one that lacks it
> > > > (say acpi_pm), in the middle of clock_gettime routine.
> > > >
> > > > seqlock does not really protect us, since readers here won't stop the writers
> > > > to change references. The proposed solution is to grab a copy of the clock
> > > > structure early, and use it as a stable reference onwards.
> > > 
> > > Ah. Good find! The fix looks reasonable to me. However, its likely the
> > > similar code in arch/x86/kernel/vsyscall_64.c will need a similar fix.
> > > 
> > > Awhile back there was some motivation to merge the two vdso/vsyscall
> > > implementations to avoid the duplication, but my memory is failing on
> > > why that didn't happen. I feel like it had to do with complication
> > > with the way the two implementations are mapped out to userland. Even
> > > so, it seems a shared forced inline method would resolve the issue, so
> > > maybe it just fell off the todo list?
> > News here?
> 
> Errr.. Sorry, are you waiting for me to implement this? Or did you want
> a comment on your earlier mail? (Apologies, I've been a bit scattered
> here).
> 
np. Comments on the earlier e-mail.
I tried a bit more with that, and sent two other e-mails arguing why I thought
that was the right approach. (well, the second best, just after unification)

> I think trying to unify the two implementations would be nice if you're
> up to trying, but if not, go ahead and push your patch and that will fix
> the bug until I can get around to looking at the unification.
Unfortunately, I have other things under my radar now, so if people
don't see a problem, I'd like to have this particular problem solved.
The patch seems right anyway.

Re: [PATCH] use a stable clock reference in vdso vgetns

[john stultz]

On Wed, 2010-10-13 at 23:44 -0300, Glauber Costa wrote:
> On Wed, Oct 13, 2010 at 06:36:55PM -0700, john stultz wrote:
> > On Wed, 2010-10-13 at 11:07 -0300, Glauber Costa wrote:
> > > On Fri, Oct 01, 2010 at 10:49:53AM -0700, john stultz wrote:
> > > > On Fri, Oct 1, 2010 at 6:09 AM, Glauber Costa <glommer@redhat.com> wrote:
> > > > > When using vdso services for clock_gettime, we test for the ability
> > > > > of a fine-grained measurement through the existance of a vread() function.
> > > > >
> > > > > However, from the time we test it, to the time we use it, vread() reference
> > > > > may not be valid anymore. It happens, for example, when we change the current
> > > > > clocksource from one that provides vread (say tsc) to one that lacks it
> > > > > (say acpi_pm), in the middle of clock_gettime routine.
> > > > >
> > > > > seqlock does not really protect us, since readers here won't stop the writers
> > > > > to change references. The proposed solution is to grab a copy of the clock
> > > > > structure early, and use it as a stable reference onwards.
> > > > 
> > > > Ah. Good find! The fix looks reasonable to me. However, its likely the
> > > > similar code in arch/x86/kernel/vsyscall_64.c will need a similar fix.
> > > > 
> > > > Awhile back there was some motivation to merge the two vdso/vsyscall
> > > > implementations to avoid the duplication, but my memory is failing on
> > > > why that didn't happen. I feel like it had to do with complication
> > > > with the way the two implementations are mapped out to userland. Even
> > > > so, it seems a shared forced inline method would resolve the issue, so
> > > > maybe it just fell off the todo list?
> > > News here?
> > 
> > Errr.. Sorry, are you waiting for me to implement this? Or did you want
> > a comment on your earlier mail? (Apologies, I've been a bit scattered
> > here).
> > 
> np. Comments on the earlier e-mail.
> I tried a bit more with that, and sent two other e-mails arguing why I thought
> that was the right approach. (well, the second best, just after unification)
> 
> > I think trying to unify the two implementations would be nice if you're
> > up to trying, but if not, go ahead and push your patch and that will fix
> > the bug until I can get around to looking at the unification.
> Unfortunately, I have other things under my radar now, so if people
> don't see a problem, I'd like to have this particular problem solved.
> The patch seems right anyway.

Ok. Then go ahead and push it in and I'll try to get the unification
done later.

thanks
-john

Re: [PATCH] use a stable clock reference in vdso vgetns

[Glauber Costa]

On Thu, Oct 14, 2010 at 11:16:51AM -0700, john stultz wrote:
> On Wed, 2010-10-13 at 23:44 -0300, Glauber Costa wrote:
> > On Wed, Oct 13, 2010 at 06:36:55PM -0700, john stultz wrote:
> > > On Wed, 2010-10-13 at 11:07 -0300, Glauber Costa wrote:
> > > > On Fri, Oct 01, 2010 at 10:49:53AM -0700, john stultz wrote:
> > > > > On Fri, Oct 1, 2010 at 6:09 AM, Glauber Costa <glommer@redhat.com> wrote:
> > > > > > When using vdso services for clock_gettime, we test for the ability
> > > > > > of a fine-grained measurement through the existance of a vread() function.
> > > > > >
> > > > > > However, from the time we test it, to the time we use it, vread() reference
> > > > > > may not be valid anymore. It happens, for example, when we change the current
> > > > > > clocksource from one that provides vread (say tsc) to one that lacks it
> > > > > > (say acpi_pm), in the middle of clock_gettime routine.
> > > > > >
> > > > > > seqlock does not really protect us, since readers here won't stop the writers
> > > > > > to change references. The proposed solution is to grab a copy of the clock
> > > > > > structure early, and use it as a stable reference onwards.
> > > > > 
> > > > > Ah. Good find! The fix looks reasonable to me. However, its likely the
> > > > > similar code in arch/x86/kernel/vsyscall_64.c will need a similar fix.
> > > > > 
> > > > > Awhile back there was some motivation to merge the two vdso/vsyscall
> > > > > implementations to avoid the duplication, but my memory is failing on
> > > > > why that didn't happen. I feel like it had to do with complication
> > > > > with the way the two implementations are mapped out to userland. Even
> > > > > so, it seems a shared forced inline method would resolve the issue, so
> > > > > maybe it just fell off the todo list?
> > > > News here?
> > > 
> > > Errr.. Sorry, are you waiting for me to implement this? Or did you want
> > > a comment on your earlier mail? (Apologies, I've been a bit scattered
> > > here).
> > > 
> > np. Comments on the earlier e-mail.
> > I tried a bit more with that, and sent two other e-mails arguing why I thought
> > that was the right approach. (well, the second best, just after unification)
> > 
> > > I think trying to unify the two implementations would be nice if you're
> > > up to trying, but if not, go ahead and push your patch and that will fix
> > > the bug until I can get around to looking at the unification.
> > Unfortunately, I have other things under my radar now, so if people
> > don't see a problem, I'd like to have this particular problem solved.
> > The patch seems right anyway.
> 
> Ok. Then go ahead and push it in and I'll try to get the unification
> done later.
Also, if we're integrating this, I'd expect the code in vdso to be
the end result, not the other way around. This makes this patch applicable
as-is, anyway.

So, as for pushing it, I don't expect any changes to it. Maintainers?