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

[Will Deacon <will@kernel.org>]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel