| Message ID | 1447750411-6424-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 65da0a8e34a857f2ba9ccb91dc8f8f964cf938b7 |
| Headers | show |
On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote: > As pointed out by Russell King in response to the proposed ARM version > of this code, the sequence to switch between the UEFI runtime mapping > and current's actual userland mapping (and vice versa) is potentially > unsafe, since it leaves a time window between the switch to the new > page tables and the TLB flush where speculative accesses may hit on > stale global TLB entries. Wow, annoying that we missed that. > So instead, use non-global mappings, and perform the switch via the > ordinary ASID-aware context switch routines. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> From digging into the way the ASID allocator works, I believe this is correct. FWIW: Reviewed-by: Mark Rutland <mark.rutland@arm.com> For backporting, I'm not sure that this is necessarily safe prior to Will's rework of the ASID allocator. I think we can IPI in this context, and it looks like the cpu_set_reserved_ttbr0() in flush_context() would save us from the problem described above, but I may have missed something. Will, are you aware of anything that could bite us here? Mark. > --- > arch/arm64/include/asm/mmu_context.h | 2 +- > arch/arm64/kernel/efi.c | 14 +++++--------- > 2 files changed, 6 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h > index c0e87898ba96..24165784b803 100644 > --- a/arch/arm64/include/asm/mmu_context.h > +++ b/arch/arm64/include/asm/mmu_context.h > @@ -101,7 +101,7 @@ static inline void cpu_set_default_tcr_t0sz(void) > #define destroy_context(mm) do { } while(0) > void check_and_switch_context(struct mm_struct *mm, unsigned int cpu); > > -#define init_new_context(tsk,mm) ({ atomic64_set(&mm->context.id, 0); 0; }) > +#define init_new_context(tsk,mm) ({ atomic64_set(&(mm)->context.id, 0); 0; }) > > /* > * This is called when "tsk" is about to enter lazy TLB mode. > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > index de46b50f4cdf..fc5508e0df57 100644 > --- a/arch/arm64/kernel/efi.c > +++ b/arch/arm64/kernel/efi.c > @@ -224,6 +224,8 @@ static bool __init efi_virtmap_init(void) > { > efi_memory_desc_t *md; > > + init_new_context(NULL, &efi_mm); > + > for_each_efi_memory_desc(&memmap, md) { > u64 paddr, npages, size; > pgprot_t prot; > @@ -254,7 +256,8 @@ static bool __init efi_virtmap_init(void) > else > prot = PAGE_KERNEL; > > - create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot); > + create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, > + __pgprot(pgprot_val(prot) | PTE_NG)); > } > return true; > } > @@ -329,14 +332,7 @@ core_initcall(arm64_dmi_init); > > static void efi_set_pgd(struct mm_struct *mm) > { > - if (mm == &init_mm) > - cpu_set_reserved_ttbr0(); > - else > - cpu_switch_mm(mm->pgd, mm); > - > - local_flush_tlb_all(); > - if (icache_is_aivivt()) > - __local_flush_icache_all(); > + switch_mm(NULL, mm, NULL); > } > > void efi_virtmap_load(void) > -- > 1.9.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote: > On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote: > > As pointed out by Russell King in response to the proposed ARM version > > of this code, the sequence to switch between the UEFI runtime mapping > > and current's actual userland mapping (and vice versa) is potentially > > unsafe, since it leaves a time window between the switch to the new > > page tables and the TLB flush where speculative accesses may hit on > > stale global TLB entries. > > Wow, annoying that we missed that. > > > So instead, use non-global mappings, and perform the switch via the > > ordinary ASID-aware context switch routines. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > From digging into the way the ASID allocator works, I believe this is > correct. FWIW: > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > For backporting, I'm not sure that this is necessarily safe prior to > Will's rework of the ASID allocator. I think we can IPI in this context, > and it looks like the cpu_set_reserved_ttbr0() in flush_context() would > save us from the problem described above, but I may have missed > something. > > Will, are you aware of anything that could bite us here? Can we guarantee that efi_virtmap_{load,unload} are called with interrupts enabled? Also, the old rollover code seems to rely on current->active_mm being the thing to switch to, so an incoming rollover might break things if we're running with the efi_mm. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Nov 17, 2015 at 04:34:46PM +0000, Will Deacon wrote: > On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote: > > On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote: > > > As pointed out by Russell King in response to the proposed ARM version > > > of this code, the sequence to switch between the UEFI runtime mapping > > > and current's actual userland mapping (and vice versa) is potentially > > > unsafe, since it leaves a time window between the switch to the new > > > page tables and the TLB flush where speculative accesses may hit on > > > stale global TLB entries. > > > > Wow, annoying that we missed that. > > > > > So instead, use non-global mappings, and perform the switch via the > > > ordinary ASID-aware context switch routines. > > > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > From digging into the way the ASID allocator works, I believe this is > > correct. FWIW: > > > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > > > For backporting, I'm not sure that this is necessarily safe prior to > > Will's rework of the ASID allocator. I think we can IPI in this context, > > and it looks like the cpu_set_reserved_ttbr0() in flush_context() would > > save us from the problem described above, but I may have missed > > something. > > > > Will, are you aware of anything that could bite us here? > > Can we guarantee that efi_virtmap_{load,unload} are called with interrupts > enabled? Unfortuantely, it looks like we can guarantee interrupts are _disabled_. Every function in drivers/firmware/efi/runtime-wrappers.c which uses efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a spin_lock_irq{save,restore} pair. Those appear to be the only uses of efi_call_virt. Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 17 November 2015 at 17:48, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Nov 17, 2015 at 04:34:46PM +0000, Will Deacon wrote: >> On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote: >> > On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote: >> > > As pointed out by Russell King in response to the proposed ARM version >> > > of this code, the sequence to switch between the UEFI runtime mapping >> > > and current's actual userland mapping (and vice versa) is potentially >> > > unsafe, since it leaves a time window between the switch to the new >> > > page tables and the TLB flush where speculative accesses may hit on >> > > stale global TLB entries. >> > >> > Wow, annoying that we missed that. >> > >> > > So instead, use non-global mappings, and perform the switch via the >> > > ordinary ASID-aware context switch routines. >> > > >> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> > >> > From digging into the way the ASID allocator works, I believe this is >> > correct. FWIW: >> > >> > Reviewed-by: Mark Rutland <mark.rutland@arm.com> >> > >> > For backporting, I'm not sure that this is necessarily safe prior to >> > Will's rework of the ASID allocator. I think we can IPI in this context, >> > and it looks like the cpu_set_reserved_ttbr0() in flush_context() would >> > save us from the problem described above, but I may have missed >> > something. >> > >> > Will, are you aware of anything that could bite us here? >> >> Can we guarantee that efi_virtmap_{load,unload} are called with interrupts >> enabled? > > Unfortuantely, it looks like we can guarantee interrupts are _disabled_. > > Every function in drivers/firmware/efi/runtime-wrappers.c which uses > efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a > spin_lock_irq{save,restore} pair. Those appear to be the only uses of > efi_call_virt. > There is actually no need from the UEFI pov to invoke the UEFI runtime services with interrupts disabled, this is simply an implementation detail of the kernel support, and I think it is primarily for x86 (but I have to dig up the old thread for the details) And even if we stick with spin_lock_irqsave(), we could refactor the runtime wrappers to perform the mm switch outside of them. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 17 November 2015 at 17:34, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote: >> On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote: >> > As pointed out by Russell King in response to the proposed ARM version >> > of this code, the sequence to switch between the UEFI runtime mapping >> > and current's actual userland mapping (and vice versa) is potentially >> > unsafe, since it leaves a time window between the switch to the new >> > page tables and the TLB flush where speculative accesses may hit on >> > stale global TLB entries. >> >> Wow, annoying that we missed that. >> >> > So instead, use non-global mappings, and perform the switch via the >> > ordinary ASID-aware context switch routines. >> > >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> >> From digging into the way the ASID allocator works, I believe this is >> correct. FWIW: >> >> Reviewed-by: Mark Rutland <mark.rutland@arm.com> >> >> For backporting, I'm not sure that this is necessarily safe prior to >> Will's rework of the ASID allocator. I think we can IPI in this context, >> and it looks like the cpu_set_reserved_ttbr0() in flush_context() would >> save us from the problem described above, but I may have missed >> something. >> >> Will, are you aware of anything that could bite us here? > > Can we guarantee that efi_virtmap_{load,unload} are called with interrupts > enabled? Also, the old rollover code seems to rely on current->active_mm > being the thing to switch to, so an incoming rollover might break things > if we're running with the efi_mm. > OK, but not the current rollover code, right? I.e., if we go with this approach (after fixing the interupts issue), it will carry over to ARM as well? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 17 November 2015 at 18:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 17 November 2015 at 17:48, Mark Rutland <mark.rutland@arm.com> wrote: >> On Tue, Nov 17, 2015 at 04:34:46PM +0000, Will Deacon wrote: >>> On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote: >>> > On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote: >>> > > As pointed out by Russell King in response to the proposed ARM version >>> > > of this code, the sequence to switch between the UEFI runtime mapping >>> > > and current's actual userland mapping (and vice versa) is potentially >>> > > unsafe, since it leaves a time window between the switch to the new >>> > > page tables and the TLB flush where speculative accesses may hit on >>> > > stale global TLB entries. >>> > >>> > Wow, annoying that we missed that. >>> > >>> > > So instead, use non-global mappings, and perform the switch via the >>> > > ordinary ASID-aware context switch routines. >>> > > >>> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> > >>> > From digging into the way the ASID allocator works, I believe this is >>> > correct. FWIW: >>> > >>> > Reviewed-by: Mark Rutland <mark.rutland@arm.com> >>> > >>> > For backporting, I'm not sure that this is necessarily safe prior to >>> > Will's rework of the ASID allocator. I think we can IPI in this context, >>> > and it looks like the cpu_set_reserved_ttbr0() in flush_context() would >>> > save us from the problem described above, but I may have missed >>> > something. >>> > >>> > Will, are you aware of anything that could bite us here? >>> >>> Can we guarantee that efi_virtmap_{load,unload} are called with interrupts >>> enabled? >> >> Unfortuantely, it looks like we can guarantee interrupts are _disabled_. >> >> Every function in drivers/firmware/efi/runtime-wrappers.c which uses >> efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a >> spin_lock_irq{save,restore} pair. Those appear to be the only uses of >> efi_call_virt. >> > > There is actually no need from the UEFI pov to invoke the UEFI runtime > services with interrupts disabled, this is simply an implementation > detail of the kernel support, and I think it is primarily for x86 (but > I have to dig up the old thread for the details) > Thread is here: http://marc.info/?l=linux-arm-kernel&m=140429592914544 > And even if we stick with spin_lock_irqsave(), we could refactor the > runtime wrappers to perform the mm switch outside of them. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Nov 17, 2015 at 06:00:36PM +0100, Ard Biesheuvel wrote: > On 17 November 2015 at 17:48, Mark Rutland <mark.rutland@arm.com> wrote: > > On Tue, Nov 17, 2015 at 04:34:46PM +0000, Will Deacon wrote: > >> On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote: > >> > Will, are you aware of anything that could bite us here? > >> > >> Can we guarantee that efi_virtmap_{load,unload} are called with interrupts > >> enabled? > > > > Unfortuantely, it looks like we can guarantee interrupts are _disabled_. > > > > Every function in drivers/firmware/efi/runtime-wrappers.c which uses > > efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a > > spin_lock_irq{save,restore} pair. Those appear to be the only uses of > > efi_call_virt. > > > > There is actually no need from the UEFI pov to invoke the UEFI runtime > services with interrupts disabled, this is simply an implementation > detail of the kernel support, and I think it is primarily for x86 (but > I have to dig up the old thread for the details) So you have a double-edged sword here: - switch_mm must be called with interrupts enabled prior to -rc1, otherwise we play a song-and-dance with TIF_SWITCH_MM. - If you have interrupts enabled, you can receive a rollover IPI from another core, which means you switch to current->active_mm. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Nov 17, 2015 at 06:01:38PM +0100, Ard Biesheuvel wrote: > On 17 November 2015 at 17:34, Will Deacon <will.deacon@arm.com> wrote: > > On Tue, Nov 17, 2015 at 03:25:58PM +0000, Mark Rutland wrote: > >> On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote: > >> > As pointed out by Russell King in response to the proposed ARM version > >> > of this code, the sequence to switch between the UEFI runtime mapping > >> > and current's actual userland mapping (and vice versa) is potentially > >> > unsafe, since it leaves a time window between the switch to the new > >> > page tables and the TLB flush where speculative accesses may hit on > >> > stale global TLB entries. > >> > >> Wow, annoying that we missed that. > >> > >> > So instead, use non-global mappings, and perform the switch via the > >> > ordinary ASID-aware context switch routines. > >> > > >> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> > >> From digging into the way the ASID allocator works, I believe this is > >> correct. FWIW: > >> > >> Reviewed-by: Mark Rutland <mark.rutland@arm.com> > >> > >> For backporting, I'm not sure that this is necessarily safe prior to > >> Will's rework of the ASID allocator. I think we can IPI in this context, > >> and it looks like the cpu_set_reserved_ttbr0() in flush_context() would > >> save us from the problem described above, but I may have missed > >> something. > >> > >> Will, are you aware of anything that could bite us here? > > > > Can we guarantee that efi_virtmap_{load,unload} are called with interrupts > > enabled? Also, the old rollover code seems to rely on current->active_mm > > being the thing to switch to, so an incoming rollover might break things > > if we're running with the efi_mm. > > > > OK, but not the current rollover code, right? I.e., if we go with this > approach (after fixing the interupts issue), it will carry over to ARM > as well? Sure, the current code (i.e. for anything after 4.3) looks fine. Let me go back and ack that... Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote: > As pointed out by Russell King in response to the proposed ARM version > of this code, the sequence to switch between the UEFI runtime mapping > and current's actual userland mapping (and vice versa) is potentially > unsafe, since it leaves a time window between the switch to the new > page tables and the TLB flush where speculative accesses may hit on > stale global TLB entries. > > So instead, use non-global mappings, and perform the switch via the > ordinary ASID-aware context switch routines. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/include/asm/mmu_context.h | 2 +- > arch/arm64/kernel/efi.c | 14 +++++--------- > 2 files changed, 6 insertions(+), 10 deletions(-) Acked-by: Will Deacon <will.deacon@arm.com> Please do *not* tag this for stable! ;) Will > diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h > index c0e87898ba96..24165784b803 100644 > --- a/arch/arm64/include/asm/mmu_context.h > +++ b/arch/arm64/include/asm/mmu_context.h > @@ -101,7 +101,7 @@ static inline void cpu_set_default_tcr_t0sz(void) > #define destroy_context(mm) do { } while(0) > void check_and_switch_context(struct mm_struct *mm, unsigned int cpu); > > -#define init_new_context(tsk,mm) ({ atomic64_set(&mm->context.id, 0); 0; }) > +#define init_new_context(tsk,mm) ({ atomic64_set(&(mm)->context.id, 0); 0; }) > > /* > * This is called when "tsk" is about to enter lazy TLB mode. > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > index de46b50f4cdf..fc5508e0df57 100644 > --- a/arch/arm64/kernel/efi.c > +++ b/arch/arm64/kernel/efi.c > @@ -224,6 +224,8 @@ static bool __init efi_virtmap_init(void) > { > efi_memory_desc_t *md; > > + init_new_context(NULL, &efi_mm); > + > for_each_efi_memory_desc(&memmap, md) { > u64 paddr, npages, size; > pgprot_t prot; > @@ -254,7 +256,8 @@ static bool __init efi_virtmap_init(void) > else > prot = PAGE_KERNEL; > > - create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot); > + create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, > + __pgprot(pgprot_val(prot) | PTE_NG)); > } > return true; > } > @@ -329,14 +332,7 @@ core_initcall(arm64_dmi_init); > > static void efi_set_pgd(struct mm_struct *mm) > { > - if (mm == &init_mm) > - cpu_set_reserved_ttbr0(); > - else > - cpu_switch_mm(mm->pgd, mm); > - > - local_flush_tlb_all(); > - if (icache_is_aivivt()) > - __local_flush_icache_all(); > + switch_mm(NULL, mm, NULL); > } > > void efi_virtmap_load(void) > -- > 1.9.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 17 November 2015 at 18:08, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote: >> As pointed out by Russell King in response to the proposed ARM version >> of this code, the sequence to switch between the UEFI runtime mapping >> and current's actual userland mapping (and vice versa) is potentially >> unsafe, since it leaves a time window between the switch to the new >> page tables and the TLB flush where speculative accesses may hit on >> stale global TLB entries. >> >> So instead, use non-global mappings, and perform the switch via the >> ordinary ASID-aware context switch routines. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> arch/arm64/include/asm/mmu_context.h | 2 +- >> arch/arm64/kernel/efi.c | 14 +++++--------- >> 2 files changed, 6 insertions(+), 10 deletions(-) > > Acked-by: Will Deacon <will.deacon@arm.com> > > Please do *not* tag this for stable! ;) > OK, thanks for clarifying. So for stable, should we keep the global mappings and do something like this instead? """ cpu_set_reserved_ttbr0(); local_flush_tlb_all(); if (icache_is_aivivt()) __local_flush_icache_all(); if (mm != &init_mm) cpu_switch_mm(mm->pgd, mm); """ >> diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h >> index c0e87898ba96..24165784b803 100644 >> --- a/arch/arm64/include/asm/mmu_context.h >> +++ b/arch/arm64/include/asm/mmu_context.h >> @@ -101,7 +101,7 @@ static inline void cpu_set_default_tcr_t0sz(void) >> #define destroy_context(mm) do { } while(0) >> void check_and_switch_context(struct mm_struct *mm, unsigned int cpu); >> >> -#define init_new_context(tsk,mm) ({ atomic64_set(&mm->context.id, 0); 0; }) >> +#define init_new_context(tsk,mm) ({ atomic64_set(&(mm)->context.id, 0); 0; }) >> >> /* >> * This is called when "tsk" is about to enter lazy TLB mode. >> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c >> index de46b50f4cdf..fc5508e0df57 100644 >> --- a/arch/arm64/kernel/efi.c >> +++ b/arch/arm64/kernel/efi.c >> @@ -224,6 +224,8 @@ static bool __init efi_virtmap_init(void) >> { >> efi_memory_desc_t *md; >> >> + init_new_context(NULL, &efi_mm); >> + >> for_each_efi_memory_desc(&memmap, md) { >> u64 paddr, npages, size; >> pgprot_t prot; >> @@ -254,7 +256,8 @@ static bool __init efi_virtmap_init(void) >> else >> prot = PAGE_KERNEL; >> >> - create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot); >> + create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, >> + __pgprot(pgprot_val(prot) | PTE_NG)); >> } >> return true; >> } >> @@ -329,14 +332,7 @@ core_initcall(arm64_dmi_init); >> >> static void efi_set_pgd(struct mm_struct *mm) >> { >> - if (mm == &init_mm) >> - cpu_set_reserved_ttbr0(); >> - else >> - cpu_switch_mm(mm->pgd, mm); >> - >> - local_flush_tlb_all(); >> - if (icache_is_aivivt()) >> - __local_flush_icache_all(); >> + switch_mm(NULL, mm, NULL); >> } >> >> void efi_virtmap_load(void) >> -- >> 1.9.1 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >> > For backporting, I'm not sure that this is necessarily safe prior to > >> > Will's rework of the ASID allocator. I think we can IPI in this context, > >> > and it looks like the cpu_set_reserved_ttbr0() in flush_context() would > >> > save us from the problem described above, but I may have missed > >> > something. > >> > > >> > Will, are you aware of anything that could bite us here? > >> > >> Can we guarantee that efi_virtmap_{load,unload} are called with interrupts > >> enabled? > > > > Unfortuantely, it looks like we can guarantee interrupts are _disabled_. > > > > Every function in drivers/firmware/efi/runtime-wrappers.c which uses > > efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a > > spin_lock_irq{save,restore} pair. Those appear to be the only uses of > > efi_call_virt. > > > > There is actually no need from the UEFI pov to invoke the UEFI runtime > services with interrupts disabled, this is simply an implementation > detail of the kernel support, and I think it is primarily for x86 (but > I have to dig up the old thread for the details) > > And even if we stick with spin_lock_irqsave(), we could refactor the > runtime wrappers to perform the mm switch outside of them. Ok. I'm only thinking about stable here. In the context of a stable backport, I think the simplest thing to do is always go via the resesrved ttbr0 to perform the TLB flush, and hand-code the save/restore of the active mm's TTBR0_EL1 value rather than going through cpu_switch_mm (which I believe we can't call with interrupts disabled). It doesn't look like it's easy to stash the value given efi_virtmap_{load,unload} are separate functions, and I don't think we can just restore from current->active_mm in case there was a concurrent rollover on another CPU. Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Nov 17, 2015 at 06:11:56PM +0100, Ard Biesheuvel wrote: > On 17 November 2015 at 18:08, Will Deacon <will.deacon@arm.com> wrote: > > On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote: > >> As pointed out by Russell King in response to the proposed ARM version > >> of this code, the sequence to switch between the UEFI runtime mapping > >> and current's actual userland mapping (and vice versa) is potentially > >> unsafe, since it leaves a time window between the switch to the new > >> page tables and the TLB flush where speculative accesses may hit on > >> stale global TLB entries. > >> > >> So instead, use non-global mappings, and perform the switch via the > >> ordinary ASID-aware context switch routines. > >> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >> --- > >> arch/arm64/include/asm/mmu_context.h | 2 +- > >> arch/arm64/kernel/efi.c | 14 +++++--------- > >> 2 files changed, 6 insertions(+), 10 deletions(-) > > > > Acked-by: Will Deacon <will.deacon@arm.com> > > > > Please do *not* tag this for stable! ;) > > > > OK, thanks for clarifying. > > So for stable, should we keep the global mappings and do something > like this instead? > > """ > cpu_set_reserved_ttbr0(); > > local_flush_tlb_all(); > if (icache_is_aivivt()) > __local_flush_icache_all(); > > if (mm != &init_mm) > cpu_switch_mm(mm->pgd, mm); > """ That looks good to me, but I think we'd want to give it a good testing given that we're solving a problem that has been spotted by code inspection as opposed to real failures on hardware. There's even an argument that it's not worth doing anything for -stable. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 17 November 2015 at 18:17, Mark Rutland <mark.rutland@arm.com> wrote: >> >> > For backporting, I'm not sure that this is necessarily safe prior to >> >> > Will's rework of the ASID allocator. I think we can IPI in this context, >> >> > and it looks like the cpu_set_reserved_ttbr0() in flush_context() would >> >> > save us from the problem described above, but I may have missed >> >> > something. >> >> > >> >> > Will, are you aware of anything that could bite us here? >> >> >> >> Can we guarantee that efi_virtmap_{load,unload} are called with interrupts >> >> enabled? >> > >> > Unfortuantely, it looks like we can guarantee interrupts are _disabled_. >> > >> > Every function in drivers/firmware/efi/runtime-wrappers.c which uses >> > efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a >> > spin_lock_irq{save,restore} pair. Those appear to be the only uses of >> > efi_call_virt. >> > >> >> There is actually no need from the UEFI pov to invoke the UEFI runtime >> services with interrupts disabled, this is simply an implementation >> detail of the kernel support, and I think it is primarily for x86 (but >> I have to dig up the old thread for the details) >> >> And even if we stick with spin_lock_irqsave(), we could refactor the >> runtime wrappers to perform the mm switch outside of them. > > Ok. > > I'm only thinking about stable here. > > In the context of a stable backport, I think the simplest thing to do is > always go via the resesrved ttbr0 to perform the TLB flush, and > hand-code the save/restore of the active mm's TTBR0_EL1 value rather > than going through cpu_switch_mm (which I believe we can't call with > interrupts disabled). > I think calling cpu_switch_mm() should be fine. It does set the ASID to #0 (which shouldn't matter since we only create global mappings) but does not invoke any of the logic regarding ASID assignment or rollover. > It doesn't look like it's easy to stash the value given > efi_virtmap_{load,unload} are separate functions, and I don't think we > can just restore from current->active_mm in case there was a concurrent > rollover on another CPU. > OK, so the fact that we switch to the UEFI page tables and back with interrupts disabled is keeping us safe here, and whether we use global mappings or not is completely irrelevant. In any case, I am going to check with Matt if we can leave interrupts enabled during the UEFI Runtime Services (since the execution time is not bounded by the spec), but I will keep this in mind in case anyone tries to backport that. -- Ard. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Nov 17, 2015 at 09:53:31AM +0100, Ard Biesheuvel wrote: > As pointed out by Russell King in response to the proposed ARM version > of this code, the sequence to switch between the UEFI runtime mapping > and current's actual userland mapping (and vice versa) is potentially > unsafe, since it leaves a time window between the switch to the new > page tables and the TLB flush where speculative accesses may hit on > stale global TLB entries. > > So instead, use non-global mappings, and perform the switch via the > ordinary ASID-aware context switch routines. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/include/asm/mmu_context.h | 2 +- > arch/arm64/kernel/efi.c | 14 +++++--------- > 2 files changed, 6 insertions(+), 10 deletions(-) Patch applied. Thanks. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Nov 18, 2015 at 07:42:27AM +0100, Ard Biesheuvel wrote: > On 17 November 2015 at 18:17, Mark Rutland <mark.rutland@arm.com> wrote: > >> >> > For backporting, I'm not sure that this is necessarily safe prior to > >> >> > Will's rework of the ASID allocator. I think we can IPI in this context, > >> >> > and it looks like the cpu_set_reserved_ttbr0() in flush_context() would > >> >> > save us from the problem described above, but I may have missed > >> >> > something. > >> >> > > >> >> > Will, are you aware of anything that could bite us here? > >> >> > >> >> Can we guarantee that efi_virtmap_{load,unload} are called with interrupts > >> >> enabled? > >> > > >> > Unfortuantely, it looks like we can guarantee interrupts are _disabled_. > >> > > >> > Every function in drivers/firmware/efi/runtime-wrappers.c which uses > >> > efi_call_virt (and hence efi_virtmap_{load,unload}) wraps the call in a > >> > spin_lock_irq{save,restore} pair. Those appear to be the only uses of > >> > efi_call_virt. > >> > > >> > >> There is actually no need from the UEFI pov to invoke the UEFI runtime > >> services with interrupts disabled, this is simply an implementation > >> detail of the kernel support, and I think it is primarily for x86 (but > >> I have to dig up the old thread for the details) > >> > >> And even if we stick with spin_lock_irqsave(), we could refactor the > >> runtime wrappers to perform the mm switch outside of them. > > > > Ok. > > > > I'm only thinking about stable here. > > > > In the context of a stable backport, I think the simplest thing to do is > > always go via the resesrved ttbr0 to perform the TLB flush, and > > hand-code the save/restore of the active mm's TTBR0_EL1 value rather > > than going through cpu_switch_mm (which I believe we can't call with > > interrupts disabled). > > > > I think calling cpu_switch_mm() should be fine. It does set the ASID > to #0 (which shouldn't matter since we only create global mappings) > but does not invoke any of the logic regarding ASID assignment or > rollover. Sorry, I got confused between switch_mm and cpu_switch_mm. I was also worried about a concurrent rollover of the same mm's ASID (e.g. in another thread) on another CPU, but I think that's fine. With the old allocator the CPU handling the rollover would block until the CPU doing the EFI call returned and enabled interrupts. > > It doesn't look like it's easy to stash the value given > > efi_virtmap_{load,unload} are separate functions, and I don't think we > > can just restore from current->active_mm in case there was a concurrent > > rollover on another CPU. > > > > OK, so the fact that we switch to the UEFI page tables and back with > interrupts disabled is keeping us safe here, and whether we use global > mappings or not is completely irrelevant. You are correct. I'd misunderstood the old allocator. The CPU handling the rollover will block until all other CPUs handled their IPI, so disabling interrupts saves us from problems with concurrent rollover. > In any case, I am going to check with Matt if we can leave interrupts > enabled during the UEFI Runtime Services (since the execution time is > not bounded by the spec), but I will keep this in mind in case anyone > tries to backport that. Sounds good; sorry for the noise! Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h index c0e87898ba96..24165784b803 100644 --- a/arch/arm64/include/asm/mmu_context.h +++ b/arch/arm64/include/asm/mmu_context.h @@ -101,7 +101,7 @@ static inline void cpu_set_default_tcr_t0sz(void) #define destroy_context(mm) do { } while(0) void check_and_switch_context(struct mm_struct *mm, unsigned int cpu); -#define init_new_context(tsk,mm) ({ atomic64_set(&mm->context.id, 0); 0; }) +#define init_new_context(tsk,mm) ({ atomic64_set(&(mm)->context.id, 0); 0; }) /* * This is called when "tsk" is about to enter lazy TLB mode. diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index de46b50f4cdf..fc5508e0df57 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -224,6 +224,8 @@ static bool __init efi_virtmap_init(void) { efi_memory_desc_t *md; + init_new_context(NULL, &efi_mm); + for_each_efi_memory_desc(&memmap, md) { u64 paddr, npages, size; pgprot_t prot; @@ -254,7 +256,8 @@ static bool __init efi_virtmap_init(void) else prot = PAGE_KERNEL; - create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot); + create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, + __pgprot(pgprot_val(prot) | PTE_NG)); } return true; } @@ -329,14 +332,7 @@ core_initcall(arm64_dmi_init); static void efi_set_pgd(struct mm_struct *mm) { - if (mm == &init_mm) - cpu_set_reserved_ttbr0(); - else - cpu_switch_mm(mm->pgd, mm); - - local_flush_tlb_all(); - if (icache_is_aivivt()) - __local_flush_icache_all(); + switch_mm(NULL, mm, NULL); } void efi_virtmap_load(void)
As pointed out by Russell King in response to the proposed ARM version of this code, the sequence to switch between the UEFI runtime mapping and current's actual userland mapping (and vice versa) is potentially unsafe, since it leaves a time window between the switch to the new page tables and the TLB flush where speculative accesses may hit on stale global TLB entries. So instead, use non-global mappings, and perform the switch via the ordinary ASID-aware context switch routines. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/include/asm/mmu_context.h | 2 +- arch/arm64/kernel/efi.c | 14 +++++--------- 2 files changed, 6 insertions(+), 10 deletions(-) -- 1.9.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel