| Message ID | 20200228145942.10675-1-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | f0c0d4b74d59809568f560001c8f88e8211334a4 |
| Headers | show |
| Series | arm64: entry: unmask IRQ in el0_sp() | expand |
Hi Mark, On 28/02/2020 14:59, Mark Rutland wrote: > Currently, the EL0 SP alignment handler masks IRQs unnecessarily. It > does so due to historic code sharing of the EL0 SP and PC alignment > handlers, and branch predictor hardening applicable to the EL0 SP > handler. > > We began masking IRQs in the EL0 SP alignment handler in commit: > > 5dfc6ed27710c42c ("arm64: entry: Apply BP hardening for high-priority synchronous exception") > > ... as this shared code with the EL0 PC alignment handler, and branch > predictor hardening made it necessary to disable IRQs for early parts of > the EL0 PC alignment handler. It was not necessary to mask IRQs during > EL0 SP alignment exceptions, but it was not considered harmful to do so. > > This masking was carried forward into C code in commit: > > 582f95835a8fc812 ("arm64: entry: convert el0_sync to C") > > ... where the SP/PC cases were split into separate handlers, and the > masking duplicated. > > Subsequently the EL0 PC alignment handler was refactored to perform > branch predictor hardening before unmasking IRQs, in commit: > > bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening") > > ... but the redundant masking of IRQs was not removed from the EL0 SP > alignment handler. Bother. > Let's do so now, and make it interruptible as with most other > synchronous exception handlers. I think you want: Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening") on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was called before arm64_notify_die(), now its not. With that, Reviewed-by: James Morse <james.morse@arm.com> Thanks! James
On Fri, Feb 28, 2020 at 03:37:46PM +0000, James Morse wrote: > Hi Mark, > > On 28/02/2020 14:59, Mark Rutland wrote: > > Currently, the EL0 SP alignment handler masks IRQs unnecessarily. It > > does so due to historic code sharing of the EL0 SP and PC alignment > > handlers, and branch predictor hardening applicable to the EL0 SP > > handler. > > > > We began masking IRQs in the EL0 SP alignment handler in commit: > > > > 5dfc6ed27710c42c ("arm64: entry: Apply BP hardening for high-priority synchronous exception") > > > > ... as this shared code with the EL0 PC alignment handler, and branch > > predictor hardening made it necessary to disable IRQs for early parts of > > the EL0 PC alignment handler. It was not necessary to mask IRQs during > > EL0 SP alignment exceptions, but it was not considered harmful to do so. > > > > This masking was carried forward into C code in commit: > > > > 582f95835a8fc812 ("arm64: entry: convert el0_sync to C") > > > > ... where the SP/PC cases were split into separate handlers, and the > > masking duplicated. > > > > Subsequently the EL0 PC alignment handler was refactored to perform > > branch predictor hardening before unmasking IRQs, in commit: > > > > bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening") > > > > ... but the redundant masking of IRQs was not removed from the EL0 SP > > alignment handler. > > Bother. > > > > Let's do so now, and make it interruptible as with most other > > synchronous exception handlers. > > I think you want: > Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening") > > on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was > called before arm64_notify_die(), now its not. > > With that, > Reviewed-by: James Morse <james.morse@arm.com> Ah; I missed that subtlety. I assume that Catalin can fold those in when applying. Otherwise I'll add them for a v2. Thanks, Mark.
On Fri, Feb 28, 2020 at 04:08:09PM +0000, Mark Rutland wrote: > On Fri, Feb 28, 2020 at 03:37:46PM +0000, James Morse wrote: > > On 28/02/2020 14:59, Mark Rutland wrote: > > > Let's do so now, and make it interruptible as with most other > > > synchronous exception handlers. > > > > I think you want: > > Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening") > > > > on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was > > called before arm64_notify_die(), now its not. > > > > With that, > > Reviewed-by: James Morse <james.morse@arm.com> > > Ah; I missed that subtlety. > > I assume that Catalin can fold those in when applying. Otherwise I'll > add them for a v2. If you want v2 to go in as a fix, please can you explain why this is a problem (beyond disabling interrupts for longer than necessary) in the commit message? Cheers, Will
Hi Mark, Will, On 05/03/2020 20:30, Will Deacon wrote: > On Fri, Feb 28, 2020 at 04:08:09PM +0000, Mark Rutland wrote: >> On Fri, Feb 28, 2020 at 03:37:46PM +0000, James Morse wrote: >>> On 28/02/2020 14:59, Mark Rutland wrote: >>>> Let's do so now, and make it interruptible as with most other >>>> synchronous exception handlers. >>> >>> I think you want: >>> Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening") >>> >>> on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was >>> called before arm64_notify_die(), now its not. >>> >>> With that, >>> Reviewed-by: James Morse <james.morse@arm.com> >> >> Ah; I missed that subtlety. >> >> I assume that Catalin can fold those in when applying. Otherwise I'll >> add them for a v2. > > If you want v2 to go in as a fix, please can you explain why this is a > problem (beyond disabling interrupts for longer than necessary) in the > commit message? Good news, its not broken. I wrongly-assumed calling arm64_notify_die() and then force_sig_fault() with both IRQ-unmasked and IRQ-masked would lead to problems, but the guts of force_sig_fault() is all wrapped in spin_lock_irqsave(). Thanks, James
On Mon, Mar 09, 2020 at 04:04:58PM +0000, James Morse wrote: > Hi Mark, Will, > > On 05/03/2020 20:30, Will Deacon wrote: > > On Fri, Feb 28, 2020 at 04:08:09PM +0000, Mark Rutland wrote: > >> On Fri, Feb 28, 2020 at 03:37:46PM +0000, James Morse wrote: > >>> On 28/02/2020 14:59, Mark Rutland wrote: > >>>> Let's do so now, and make it interruptible as with most other > >>>> synchronous exception handlers. > >>> > >>> I think you want: > >>> Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening") > >>> > >>> on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was > >>> called before arm64_notify_die(), now its not. > >>> > >>> With that, > >>> Reviewed-by: James Morse <james.morse@arm.com> > >> > >> Ah; I missed that subtlety. > >> > >> I assume that Catalin can fold those in when applying. Otherwise I'll > >> add them for a v2. > > > > If you want v2 to go in as a fix, please can you explain why this is a > > problem (beyond disabling interrupts for longer than necessary) in the > > commit message? > > Good news, its not broken. I wrongly-assumed calling arm64_notify_die() and then > force_sig_fault() with both IRQ-unmasked and IRQ-masked would lead to problems, but the > guts of force_sig_fault() is all wrapped in spin_lock_irqsave(). Thanks for delving into that. Catalin, are you happy to queue the patch for v5.7 with James' R-b (but not the Fixes tag), and/or would you like me to send a v2 with that? Thanks, Mark.
On Tue, Mar 10, 2020 at 10:51:52AM +0000, Mark Rutland wrote: > On Mon, Mar 09, 2020 at 04:04:58PM +0000, James Morse wrote: > > Hi Mark, Will, > > > > On 05/03/2020 20:30, Will Deacon wrote: > > > On Fri, Feb 28, 2020 at 04:08:09PM +0000, Mark Rutland wrote: > > >> On Fri, Feb 28, 2020 at 03:37:46PM +0000, James Morse wrote: > > >>> On 28/02/2020 14:59, Mark Rutland wrote: > > >>>> Let's do so now, and make it interruptible as with most other > > >>>> synchronous exception handlers. > > >>> > > >>> I think you want: > > >>> Fixes: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening") > > >>> > > >>> on this as, bfe298745afc9548 changed the behaviour: local_daif_restore(DAIF_PROCCTX) was > > >>> called before arm64_notify_die(), now its not. > > >>> > > >>> With that, > > >>> Reviewed-by: James Morse <james.morse@arm.com> > > >> > > >> Ah; I missed that subtlety. > > >> > > >> I assume that Catalin can fold those in when applying. Otherwise I'll > > >> add them for a v2. > > > > > > If you want v2 to go in as a fix, please can you explain why this is a > > > problem (beyond disabling interrupts for longer than necessary) in the > > > commit message? > > > > Good news, its not broken. I wrongly-assumed calling arm64_notify_die() and then > > force_sig_fault() with both IRQ-unmasked and IRQ-masked would lead to problems, but the > > guts of force_sig_fault() is all wrapped in spin_lock_irqsave(). > > Thanks for delving into that. > > Catalin, are you happy to queue the patch for v5.7 with James' R-b (but > not the Fixes tag), and/or would you like me to send a v2 with that? No need to send a v2. Queued for 5.7. Thanks.
diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c index fde59981445c..c839b5bf1904 100644 --- a/arch/arm64/kernel/entry-common.c +++ b/arch/arm64/kernel/entry-common.c @@ -175,7 +175,7 @@ NOKPROBE_SYMBOL(el0_pc); static void notrace el0_sp(struct pt_regs *regs, unsigned long esr) { user_exit_irqoff(); - local_daif_restore(DAIF_PROCCTX_NOIRQ); + local_daif_restore(DAIF_PROCCTX); do_sp_pc_abort(regs->sp, esr, regs); } NOKPROBE_SYMBOL(el0_sp);
Currently, the EL0 SP alignment handler masks IRQs unnecessarily. It does so due to historic code sharing of the EL0 SP and PC alignment handlers, and branch predictor hardening applicable to the EL0 SP handler. We began masking IRQs in the EL0 SP alignment handler in commit: 5dfc6ed27710c42c ("arm64: entry: Apply BP hardening for high-priority synchronous exception") ... as this shared code with the EL0 PC alignment handler, and branch predictor hardening made it necessary to disable IRQs for early parts of the EL0 PC alignment handler. It was not necessary to mask IRQs during EL0 SP alignment exceptions, but it was not considered harmful to do so. This masking was carried forward into C code in commit: 582f95835a8fc812 ("arm64: entry: convert el0_sync to C") ... where the SP/PC cases were split into separate handlers, and the masking duplicated. Subsequently the EL0 PC alignment handler was refactored to perform branch predictor hardening before unmasking IRQs, in commit: bfe298745afc9548 ("arm64: entry-common: don't touch daif before bp-hardening") ... but the redundant masking of IRQs was not removed from the EL0 SP alignment handler. Let's do so now, and make it interruptible as with most other synchronous exception handlers. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: James Morse <james.morse@arm.com> Cc: Will Deacon <will@kernel.org> --- arch/arm64/kernel/entry-common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)