Re: [RFC 3/5] x86: Add a comment clarifying LDT context switching

[Andy Lutomirski]

On Thu, Oct 16, 2014 at 8:49 AM, Borislav Petkov <bp@xxxxxxxxx> wrote:
> On Tue, Oct 14, 2014 at 03:57:37PM -0700, Andy Lutomirski wrote:
>> The code is correct, but only for a rather subtle reason.  This
>> confused me for quite a while when I read switch_mm, so clarify the
>> code to avoid confusing other people, too.
>>
>> TBH, I wouldn't be surprised if this code was only correct by
>> accident.
>>
>> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
>> ---
>>  arch/x86/include/asm/mmu_context.h | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
>> index 166af2a8e865..04478103df37 100644
>> --- a/arch/x86/include/asm/mmu_context.h
>> +++ b/arch/x86/include/asm/mmu_context.h
>> @@ -53,7 +53,16 @@ static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
>>               /* Stop flush ipis for the previous mm */
>>               cpumask_clear_cpu(cpu, mm_cpumask(prev));
>>
>> -             /* Load the LDT, if the LDT is different: */
>> +             /*
>> +              * Load the LDT, if the LDT is different.
>> +              *
>> +              * It's possible leave_mm(prev) has been called.  If so,
>> +              * then prev->context.ldt could be out of sync with the
>> +              * LDT descriptor or the LDT register.  This can only happen
>
> I'm staring at the code and trying to figure out where on the leave_mm()
> path this could happen. Got any code pointers?

I think it's the same as in the other case in switch_mm.  leave_mm
does cpumask_clear_cpu(cpu, mm_cpumask(active_mm)), and, once that has
happened, modify_ldt won't send an IPI to this CPU.  So, if leave_mm
runs, and then another CPU calls modify_ldt on the mm that is in lazy
mode here, it won't update our LDT register, so the LDT register and
prev->context.ldt might not match.

I think that, if that's the case, then prev->context.ldt can't be NULL
and can't have the same non-NULL value as next->context.ldt.  I hope.

I'll try to make the comment clearer in v2.

--Andy

>
> :-)
>
>> +              * if prev->context.ldt is non-null, since we never free
>> +              * an LDT.  But LDTs can't be shared across mms, so
>> +              * prev->context.ldt won't be equal to next->context.ldt.
>> +              */
>>               if (unlikely(prev->context.ldt != next->context.ldt))
>>                       load_LDT_nolock(&next->context);
>>       }
>> --
>> 1.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>
> --
> Regards/Gruss,
>     Boris.
> --



-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/