Re: Review of KPTI patchset

[Mathieu Desnoyers]

----- On Dec 30, 2017, at 5:02 PM, Thomas Gleixner tglx@xxxxxxxxxxxxx wrote:

> On Sat, 30 Dec 2017, Mathieu Desnoyers wrote:
>> ----- On Dec 30, 2017, at 2:58 PM, Thomas Gleixner tglx@xxxxxxxxxxxxx wrote:
>> > /*
>> >  * Called on fork from arch_dup_mmap(). Just copy the current LDT state,
>> >  * the new task is not running, so nothing can be installed.
>> >  */
>> > int ldt_dup_context(struct mm_struct *old_mm, struct mm_struct *mm)
>> > {
>> >       struct ldt_struct *new_ldt;
>> >       int retval = 0;
>> >
>> >       if (!old_mm)
>> >               return 0;
>> 
>> If old_mm is NULL, free_ldt_pgtables(mm) is not called.
> 
> Correct. Why should it be called? Nothing is allocated. You cannot
> associate anything to a NULL pointer and you cannot duplicate the LDT
> referenced by a NULL pointer, right?

Right, from a fork() context perspective it's fine. (same for rest of function)

[...]

> 
>> >> +	this_cpu_write(cpu_tlbstate.invalidate_other, false);
>> >> +}
>> >> 
>> >> Can this be called with preemption enabled ? If so, what happens
>> >> if migrated ?
>> > 
>> > No, it can't and if it is then it's a bug and the smp_processor_id() debug
>> > code will yell at you.
>> 
>> I thought the whole point about this_cpu_*() was that it could be called
>> with preemption enabled, given that it figures out the per-cpu data offset
>> using a segment selector prefix. How would smp_processor_id() debug code be
>> involved here ?
> 
> You're right, the this_cpu_read/write wont. But the this_cpu_ptr()
> dereference in one of the invoked functions will.

I don't see any this_cpu_ptr() dereference being invoked either directly
or indirectly from clear_asid_other(). What am I missing ?

> 
> Granted, it's not obvious and ideally we convert those this_cpu_read/writes
> to __this_cpu_read/writes() to get the immediate fail reported on the first
> access.

Indeed, if this function is expected to be called from non-preempt context,
the __this_cpu_*() would be appropriate.

Moreover, clear_asid_other() could use a "static" definition, given that it's only ever
used from arch/x86/mm/tlb.c:choose_new_asid(). Otherwise nothing prevents other users
from unrelated areas of the kernel from calling it, and then we should document that
preemption needs to be disabled around its invocation.

While we are there (discussing preemption and migration), I'm puzzled about the
preempt_disable/enable in __native_flush_tlb(). First, it should cover the
invalidate_user_asid() call too. Also, if the __ prefixed functions in this header
are expected to be called with preemption enabled, then we might want to add a
preempt disable around __native_flush_tlb_single(), which also do a few per-cpu
data accesses, or otherwise document that it needs to be called with preemption
disabled (with a warn-on).

The case of __flush_tlb_one() is also interesting: it invokes __flush_tlb_single()
and then invalidate_other_asid(), which each perform this_cpu_*() operations.
If preemption is not disabled across the entire function, migration could cause
__flush_tlb_single() and invalidate_other_asid() to run on different CPUs. Again,
we should either explicitly disable preemption, or document that it needs to be
invoked with preemption disabled (with a warn-on).

Thanks,

Mathieu


> 
> Thanks,
> 
> 	tglx

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com