Re: [PATCH 09/11] arm64: entry: fix non-NMI kernel<->kernel transitions

Will Deacon
2020-11-30T11:23:37Z

On Thu, Nov 26, 2020 at 12:36:00PM +0000, Mark Rutland wrote:
> There are periods in kernel mode when RCU is not watching and/or the
> scheduler tick is disabled, but we can still take exceptions such as
> interrupts. The arm64 exception handlers do not account for this, and
> it's possible that RCU is not watching while an exception handler runs.
>
> The x86/generic entry code handles this by ensuring that all (non-NMI)
> kernel exception handlers call irqentry_enter() and irqentry_exit(),
> which handle RCU, lockdep, and IRQ flag tracing. We can't yet move to
> the generic entry code, and already hadnle the user<->kernel transitions
> elsewhere, so we add new kernel<->kernel transition helpers alog the
> lines of the generic entry code.
>
> Since we now track interrupts becoming masked when an exception is
> taken, local_daif_inherit() is modified to track interrupts becoming
> re-enabled when the original context is inherited. To balance the
> entry/exit paths, each handler masks all DAIF exceptions before
> exit_to_kernel_mode().
>
> 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/include/asm/daifflags.h | 3 ++
> arch/arm64/kernel/entry-common.c | 59 ++++++++++++++++++++++++++++++++++++--
> 2 files changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h
> index ec213b4a1650..1c26d7baa67f 100644
> --- a/arch/arm64/include/asm/daifflags.h
> +++ b/arch/arm64/include/asm/daifflags.h
> @@ -128,6 +128,9 @@ static inline void local_daif_inherit(struct pt_regs *regs)
> {
> unsigned long flags = regs->pstate & DAIF_MASK;
>
> + if (interrupts_enabled(regs))
> + trace_hardirqs_on();
> +
> /*
> * We can't use local_daif_restore(regs->pstate) here as
> * system_has_prio_mask_debugging() won't restore the I bit if it can
> diff --git a/arch/arm64/kernel/entry-common.c b/arch/arm64/kernel/entry-common.c
> index 49d1c1dd9baf..526e98cec86e 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -17,12 +17,50 @@
> #include <asm/mmu.h>
> #include <asm/sysreg.h>
>
> +static void noinstr enter_from_kernel_mode(struct pt_regs *regs)
> +{
> + regs->exit_rcu = false;
> +
> + if (!IS_ENABLED(CONFIG_TINY_RCU) && is_idle_task(current)) {
> + lockdep_hardirqs_off(CALLER_ADDR0);
> + rcu_irq_enter();
> + trace_hardirqs_off_finish();
> +
> + regs->exit_rcu = true;
> + return;
> + }
> +
> + lockdep_hardirqs_off(CALLER_ADDR0);
> + rcu_irq_enter_check_tick();
> + trace_hardirqs_off_finish();
> +}
> +
> +static void noinstr exit_to_kernel_mode(struct pt_regs *regs)
> +{
> + lockdep_assert_irqs_disabled();
> +
> + if (interrupts_enabled(regs)) {
> + if (regs->exit_rcu) {
> + trace_hardirqs_on_prepare();
> + lockdep_hardirqs_on_prepare(CALLER_ADDR0);
> + rcu_irq_exit();
> + lockdep_hardirqs_on(CALLER_ADDR0);
> + return;
> + }
> +
> + trace_hardirqs_on();
> + } else {
> + if (regs->exit_rcu)
> + rcu_irq_exit();
> + }
> +}

Hmm. I'd prefer to rework this to avoid the nested early return:

e.g:

// exit_to_kernel_mode()
if (!interrupts_enabled(regs)) {
if (regs->exit_rcu)
rcu_irq_exit()
} else if (regs->exit_rcu) {
trace_hardirqs_on_prepare();
...
} else {
trace_hardirqs_on();
}

but I see you're following the pattern in kernel/entry/common.c, which
makes sense given that the long-term goal should be to move over to that.

In which case, can you add a comment somewhere that this is deliberately
structured to map to the common code?

Cheers,

Will