lkml.org 
[lkml]   [2012]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] time: x86: Fix race switching from vsyscall to non-vsyscall clock
On 03/14/2012 06:43 PM, Andy Lutomirski wrote:
> On Wed, Mar 14, 2012 at 5:42 PM, John Stultz<john.stultz@linaro.org> wrote:
>> On 03/14/2012 05:34 PM, Thomas Gleixner wrote:
>>> On Wed, 14 Mar 2012, John Stultz wrote:
>>>> notrace static noinline int do_realtime(struct timespec *ts)
>>>> {
>>>> unsigned long seq, ns;
>>>> + int mode;
>>> Please keep a newline between declarations and code.
>>
>> Fixed below. Thanks!
>> (Let me know if you see whitespace damage, I switched mail clients today and
>> am learning the quirks here.)
>> -john
>>
>>
>>
>> When switching from a vsyscall capable to a non-vsyscall capable
>> clocksource, there was a small race, where the last vsyscall
>> gettimeofday before the switch might return a invalid time value
>> using the new non-vsyscall enabled clocksource values after the
>> switch is complete.
>>
>> This is due to the vsyscall code checking the vclock_mode once
>> outside of the seqcount protected section. After it reads the
>> vclock mode, it doesn't re-check that the sampled clock data
>> that is obtained in the seqcount critical section still matches.
>>
>> The fix is to sample vclock_mode inside the protected section,
>> and as long as it isn't VCLOCK_NONE, return the calculated
>> value. If it has changed and is now VCLOCK_NONE, fall back
>> to the syscall gettime calculation.
>>
>> v2:
>> * Cleanup checks as suggested by tglx
>> * Also fix same issue present in gettimeofday path
>>
>> CC: Andy Lutomirski<luto@amacapital.net>
>> CC: Thomas Gleixner<tglx@linutronix.de>
>> Signed-off-by: John Stultz<john.stultz@linaro.org>
>> ---
>> arch/x86/vdso/vclock_gettime.c | 68
>> +++++++++++++++++++++++++--------------
>> 1 files changed, 43 insertions(+), 25 deletions(-)
>>
> Looks reasonable to me. I like this approach better than the earlier
> way -- it's likely to cause less slowdown in the VCLOCK_TSC case.
>
> That being said, I think you might have a bug:
>
> notrace static inline long vgetns(void)
> {
> long v;
> cycles_t cycles;
> if (gtod->clock.vclock_mode == VCLOCK_TSC)
> cycles = vread_tsc();
> else
> cycles = vread_hpet();
> v = (cycles - gtod->clock.cycle_last)& gtod->clock.mask;
> return (v * gtod->clock.mult)>> gtod->clock.shift;
> }
>
> In the VCLOCK_NONE, you'll access the hpet mapping. But in
> hpet_enable, hpet_set_mapping isn't called and this will crash, I
> think.
Thanks for catching this!

My solution is to add:

else if (gtod->clock.vclock_mode == VCLOCK_HPET)
cycles = vread_hpet();
else
return 0;

Let me know if this works for you.

thanks
-john




\
 
 \ /
  Last update: 2012-03-15 21:23    [W:0.059 / U:0.744 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site