Re: [PATCH] x86/vdso/pvclock: Protect STABLE check with the seqcount

[Andy Lutomirski]

On Thu, Jan 7, 2016 at 1:02 PM, Marcelo Tosatti <mtosatti@xxxxxxxxxx> wrote:
> On Mon, Jan 04, 2016 at 03:14:28PM -0800, Andy Lutomirski wrote:
>> If the clock becomes unstable while we're reading it, we need to
>> bail.  We can do this by simply moving the check into the seqcount
>> loop.
>>
>> Reported-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
>> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
>> ---
>>
>> Marcelo, how's this?
>>
>> arch/x86/entry/vdso/vclock_gettime.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
>> index 8602f06c759f..1a50e09c945b 100644
>> --- a/arch/x86/entry/vdso/vclock_gettime.c
>> +++ b/arch/x86/entry/vdso/vclock_gettime.c
>> @@ -126,23 +126,23 @@ static notrace cycle_t vread_pvclock(int *mode)
>>        *
>>        * On Xen, we don't appear to have that guarantee, but Xen still
>>        * supplies a valid seqlock using the version field.
>> -
>> +      *
>>        * We only do pvclock vdso timing at all if
>>        * PVCLOCK_TSC_STABLE_BIT is set, and we interpret that bit to
>>        * mean that all vCPUs have matching pvti and that the TSC is
>>        * synced, so we can just look at vCPU 0's pvti.
>>        */
>>
>> -     if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
>> -             *mode = VCLOCK_NONE;
>> -             return 0;
>> -     }
>> -
>>       do {
>>               version = pvti->version;
>>
>>               smp_rmb();
>>
>> +             if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
>> +                     *mode = VCLOCK_NONE;
>> +                     return 0;
>> +             }
>> +
>>               tsc = rdtsc_ordered();
>>               pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
>>               pvti_tsc_shift = pvti->tsc_shift;
>> --
>> 2.4.3
>
> Check it before returning the value (once cleared, it can't be set back
> to 1), similarly to what was in place before.
>
>

I don't understand what you mean.

In the old code (4.3 and 4.4), the vdso checks STABLE_BIT at the end,
which is correct as long as STABLE_BIT can never change from 0 to 1.

In the -tip code, it's clearly wrong.

In the code in this patch, it should be correct regardless of how
STABLE_BIT changes as long as the seqcount works.  Given that the
performance cost of doing that is zero, I'd rather keep it that way.
If we're really paranoid, we could move it after the rest of the pvti
reads and add a barrier, but is there really any host on which that
matters?

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC