| Message ID | 20191010163447.136049-1-james.morse@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: entry.S: Do not preempt from IRQ before all cpufeatures are enabled | expand |
On Thu, Oct 10, 2019 at 05:34:47PM +0100, James Morse wrote: > From: Julien Thierry <julien.thierry@arm.com> > > Preempting from IRQ-return means that the task has its PSTATE saved > on the stack, which will get restored when the task is resumed and does > the actual IRQ return. > > However, enabling some CPU features requires modifying the PSTATE. This > means that, if a task was scheduled out during an IRQ-return before all > CPU features are enabled, the task might restore a PSTATE that does not > include the feature enablement changes once scheduled back in. > > * Task 1: > > PAN == 0 ---| |--------------- > | |<- return from IRQ, PSTATE.PAN = 0 > | <- IRQ | > +--------+ <- preempt() +-- > ^ > | > reschedule Task 1, PSTATE.PAN == 1 > * Init: > --------------------+------------------------ > ^ > | > enable_cpu_features > set PSTATE.PAN on all CPUs > > Worse than this, since PSTATE is untouched when task switching is done, > a task missing the new bits in PSTATE might affect another task, if both > do direct calls to schedule() (outside of IRQ/exception contexts). > > Fix this by preventing preemption on IRQ-return until features are > enabled on all CPUs. > > This way the only PSTATE values that are saved on the stack are from > synchronous exceptions. These are expected to be fatal this early, the > exception is BRK for WARN_ON(), but as this uses do_debug_exception() > which keeps IRQs masked, it shouldn't call schedule(). > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > [Replaced a really cool hack, with a simpler branch->nop transition, > expanded commit message with Julien's cover-letter ascii art] > Signed-off-by: James Morse <james.morse@arm.com> > --- > I think we don't see this happen because cpufeature enable calls happen > early, when there is not a lot going on. I couldn't hit it when trying. > I believe Julien did, by adding sleep statements(?) to kthread(). > > If we want to send this to stable, the first feature that depended on this > was PAN: > Fixes: 7209c868600b ("arm64: mm: Set PSTATE.PAN from the cpu_enable_pan() call") > > > arch/arm64/kernel/cpufeature.c | 8 ++++++++ > arch/arm64/kernel/entry.S | 7 +++++++ > 2 files changed, 15 insertions(+) [...] > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index 84a822748c84..7ab139c44ca5 100644 > --- a/arch/arm64/kernel/entry.S > +++ b/arch/arm64/kernel/entry.S > @@ -670,6 +670,13 @@ el1_irq: > irq_handler > > #ifdef CONFIG_PREEMPT > +alternative_cb arm64_cpufeature_cb_nop > + /* > + * Prevent preempt from IRQ until cpufeatures are enabled. This branch > + * is replaced by a nop by the callback. > + */ > + b 1f > +alternative_cb_end Could we simplify this by intercepting the branch to preempt_schedule_irq() with a C function that looks at arm64_const_caps_ready? Will
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index 9323bcc40a58..3b8c3f1c6d3f 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -2071,6 +2071,14 @@ void __init setup_cpu_features(void) ARCH_DMA_MINALIGN); } +void __init arm64_cpufeature_cb_nop(struct alt_instr *alt, __le32 *origptr, + __le32 *updptr, int nr_inst) +{ + BUG_ON(nr_inst != 1); + + *updptr = cpu_to_le32(aarch64_insn_gen_nop()); +} + static bool __maybe_unused cpufeature_pan_not_uao(const struct arm64_cpu_capabilities *entry, int __unused) { diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 84a822748c84..7ab139c44ca5 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -670,6 +670,13 @@ el1_irq: irq_handler #ifdef CONFIG_PREEMPT +alternative_cb arm64_cpufeature_cb_nop + /* + * Prevent preempt from IRQ until cpufeatures are enabled. This branch + * is replaced by a nop by the callback. + */ + b 1f +alternative_cb_end ldr x24, [tsk, #TSK_TI_PREEMPT] // get preempt count alternative_if ARM64_HAS_IRQ_PRIO_MASKING /*