Messages in this thread | | | Subject | Re: [PATCH RFC v2 5/6] x86: Use global pages when PTI is disabled | From | Dave Hansen <> | Date | Thu, 15 Feb 2018 08:54:58 -0800 |
| |
On 02/15/2018 08:36 AM, Nadav Amit wrote: > As long as PTI is disabled, it is possible to use global pages, as long > as we remove them once PTI is enabled again. To do so, return the global > bit to __supported_pte_mask and disable global pages using CR4. > > Signed-off-by: Nadav Amit <namit@vmware.com> > --- > arch/x86/include/asm/tlbflush.h | 6 ++++++ > arch/x86/mm/init.c | 14 ++++++-------- > arch/x86/mm/tlb.c | 3 ++- > 3 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h > index ea65cf951c49..3a44cb0a9f56 100644 > --- a/arch/x86/include/asm/tlbflush.h > +++ b/arch/x86/include/asm/tlbflush.h > @@ -319,6 +319,12 @@ static inline void set_cpu_pti_disable(unsigned short disable) > WARN_ON_ONCE(preemptible()); > > pti_update_user_cs64(cpu_pti_disable(), disable); > + if (__supported_pte_mask & _PAGE_GLOBAL) { > + if (disable) > + cr4_set_bits(X86_CR4_PGE); > + else > + cr4_clear_bits(X86_CR4_PGE); > + } > this_cpu_write(cpu_tlbstate.pti_disable, disable); > }
The TLB invalidations when doing this switch are *CRITICAL*. Otherwise, we end up globally-mapped kernel entries persisting to other processes that are then vulnerable to Meltdown.
So, where are the TLB flushes?
They're hidden in the cr4_set/clear_bits() function, of course. This is dangerous for two reasons because it makes them non-obvious and hard to find. It also has no interactions with the existing TLB invalidation infrastructure. That's _safe_ of course because extra flushing is OK, but it feels really funky because you're going to end up double-flushing on context switches which is rather unfortunate.
This also needs some heavy commenting about the fact that _PAGE_GLOBAL is ignored when CR4.PGE=0. That's key to this working and not mentioned anywhere.
While this looks OK to me, it still makes me rather nervous. The changelog and commenting definitely need a lot of work. I'm also still rather unconvinced that the added complexity here is worth it.
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c > index c67ef3fb4f35..979c7ec6baab 100644 > --- a/arch/x86/mm/tlb.c > +++ b/arch/x86/mm/tlb.c > @@ -74,7 +74,8 @@ static void choose_new_asid(struct mm_struct *next, u64 next_tlb_gen, > return; > } > > - if (this_cpu_read(cpu_tlbstate.invalidate_other)) > + if (this_cpu_read(cpu_tlbstate.invalidate_other) && > + !mm_pti_disable(next)) > clear_asid_other();
This isn't obviously correct. Don't we still need to invalidate other user asids?
| |