| Message ID | 20190801154150.195959-3-james.morse@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: entry: Move ct_user_exit calls earlier | expand |
On Thu, Aug 01, 2019 at 04:41:50PM +0100, James Morse wrote: > When taking an exception from EL0 we unmask exceptions before calling > ct_user_exit. This means we could take an interrupt or be single-stepped > in the gap. The entry from EL1 code sees that we took an exception from > the kernel, whereas the context_tracking code believes we are still in > user-space. > > To keep these things consistent, move the ct_user_exit calls before > any unmask of exceptions. > > Fixes: 6c81fe7925cc4c42 ("arm64: enable context tracking") > Signed-off-by: James Morse <james.morse@arm.com> > --- > arch/arm64/kernel/entry.S | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index 28681034d599..88f4ab21cb66 100644 > --- a/arch/arm64/kernel/entry.S > +++ b/arch/arm64/kernel/entry.S > @@ -792,8 +792,8 @@ el0_cp15: > /* > * Trapped CP15 (MRC, MCR, MRRC, MCRR) instructions > */ > - enable_daif > ct_user_exit > + enable_daif > mov x0, x25 > mov x1, sp > bl do_cp15instr > @@ -805,8 +805,8 @@ el0_da: > * Data abort handling > */ > mrs x26, far_el1 > - enable_daif > ct_user_exit > + enable_daif This strikes me as a bit dodgy, since we end up in context_tracking_exit(), which calls local_irq_{save,restore}() and I think our accounting is probably off at this point. I think we need to call via user_{enter,exit}_irqoff() instead to make this work. Will
Hi Will, On 01/08/2019 17:39, Will Deacon wrote: > On Thu, Aug 01, 2019 at 04:41:50PM +0100, James Morse wrote: >> When taking an exception from EL0 we unmask exceptions before calling >> ct_user_exit. This means we could take an interrupt or be single-stepped >> in the gap. The entry from EL1 code sees that we took an exception from >> the kernel, whereas the context_tracking code believes we are still in >> user-space. >> >> To keep these things consistent, move the ct_user_exit calls before >> any unmask of exceptions. >> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S >> index 28681034d599..88f4ab21cb66 100644 >> --- a/arch/arm64/kernel/entry.S >> +++ b/arch/arm64/kernel/entry.S >> @@ -792,8 +792,8 @@ el0_cp15: >> /* >> * Trapped CP15 (MRC, MCR, MRRC, MCRR) instructions >> */ >> - enable_daif >> ct_user_exit >> + enable_daif >> mov x0, x25 >> mov x1, sp >> bl do_cp15instr >> @@ -805,8 +805,8 @@ el0_da: >> * Data abort handling >> */ >> mrs x26, far_el1 >> - enable_daif >> ct_user_exit >> + enable_daif > This strikes me as a bit dodgy, since we end up in context_tracking_exit(), > which calls local_irq_{save,restore}() and I think our accounting is > probably off at this point. Huh, I'm sure I had all those options turned on... ~ local_irq_save() calls trace_hardirqs_off() unconditionally, whereas local_irq_restore() uses the saved flags to decide interrupts aren't enabled by this restore call, so the accounting ends up being correct. It calls trace_hardirqs_off() a second time. The asserts check trace_hardirqs_off() is called with interrupts masked, which it is in this case. > I think we need to call via > user_{enter,exit}_irqoff() instead to make this work. Sure, that would be clearer. Thanks, James
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 28681034d599..88f4ab21cb66 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -792,8 +792,8 @@ el0_cp15: /* * Trapped CP15 (MRC, MCR, MRRC, MCRR) instructions */ - enable_daif ct_user_exit + enable_daif mov x0, x25 mov x1, sp bl do_cp15instr @@ -805,8 +805,8 @@ el0_da: * Data abort handling */ mrs x26, far_el1 - enable_daif ct_user_exit + enable_daif clear_address_tag x0, x26 mov x1, x25 mov x2, sp @@ -818,11 +818,11 @@ el0_ia: */ mrs x26, far_el1 gic_prio_kentry_setup tmp=x0 + ct_user_exit enable_da_f #ifdef CONFIG_TRACE_IRQFLAGS bl trace_hardirqs_off #endif - ct_user_exit mov x0, x26 mov x1, x25 mov x2, sp @@ -832,8 +832,8 @@ el0_fpsimd_acc: /* * Floating Point or Advanced SIMD access */ - enable_daif ct_user_exit + enable_daif mov x0, x25 mov x1, sp bl do_fpsimd_acc @@ -842,8 +842,8 @@ el0_sve_acc: /* * Scalable Vector Extension access */ - enable_daif ct_user_exit + enable_daif mov x0, x25 mov x1, sp bl do_sve_acc @@ -852,8 +852,8 @@ el0_fpsimd_exc: /* * Floating Point, Advanced SIMD or SVE exception */ - enable_daif ct_user_exit + enable_daif mov x0, x25 mov x1, sp bl do_fpsimd_exc @@ -868,11 +868,11 @@ el0_sp_pc: * Stack or PC alignment exception handling */ gic_prio_kentry_setup tmp=x0 + ct_user_exit enable_da_f #ifdef CONFIG_TRACE_IRQFLAGS bl trace_hardirqs_off #endif - ct_user_exit mov x0, x26 mov x1, x25 mov x2, sp @@ -882,8 +882,8 @@ el0_undef: /* * Undefined instruction */ - enable_daif ct_user_exit + enable_daif mov x0, sp bl do_undefinstr b ret_to_user @@ -891,8 +891,8 @@ el0_sys: /* * System instructions, for trapped cache maintenance instructions */ - enable_daif ct_user_exit + enable_daif mov x0, x25 mov x1, sp bl do_sysinstr @@ -912,8 +912,8 @@ el0_dbg: enable_da_f b ret_to_user el0_inv: - enable_daif ct_user_exit + enable_daif mov x0, sp mov x1, #BAD_SYNC mov x2, x25 @@ -926,13 +926,13 @@ el0_irq: kernel_entry 0 el0_irq_naked: gic_prio_irq_setup pmr=x20, tmp=x0 + ct_user_exit enable_da_f #ifdef CONFIG_TRACE_IRQFLAGS bl trace_hardirqs_off #endif - ct_user_exit #ifdef CONFIG_HARDEN_BRANCH_PREDICTOR tbz x22, #55, 1f bl do_el0_irq_bp_hardening
When taking an exception from EL0 we unmask exceptions before calling ct_user_exit. This means we could take an interrupt or be single-stepped in the gap. The entry from EL1 code sees that we took an exception from the kernel, whereas the context_tracking code believes we are still in user-space. To keep these things consistent, move the ct_user_exit calls before any unmask of exceptions. Fixes: 6c81fe7925cc4c42 ("arm64: enable context tracking") Signed-off-by: James Morse <james.morse@arm.com> --- arch/arm64/kernel/entry.S | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)