Re: [PATCH] powerpc/970: fix nap return address corruption on async interrupt exit

[Shrikanth Hegde <sshegde@linux.ibm.com>]

Hi Mukesh.

On 7/7/26 10:54 PM, Mukesh Kumar Chaurasiya (IBM) wrote:
> On PowerMac G5 (PPC970, CONFIG_PPC_970_NAP) the system panics shortly
> after boot with symptoms including instruction fetch faults, kernel data
> access faults, and stack corruption, predominantly on SMP and always
> somewhere inside softirq processing.
> 
> The PPC970 idle path works by setting _TLF_NAPPING in the current
> thread's local flags before entering the MSR_POW nap loop.  When any
> async interrupt wakes the CPU, nap_adjust_return() is expected to detect
> _TLF_NAPPING, clear it, and rewrite regs->NIP to power4_idle_nap_return
> so that the interrupt returns cleanly to the caller of power4_idle_nap()
> rather than back into the nap spin loop.
> 
> DEFINE_INTERRUPT_HANDLER_ASYNC generates the following sequence:
> 
>      irq_enter_rcu();
>      ____func(regs);           /* timer_interrupt / do_IRQ body */
>      irq_exit_rcu();           /* softirqs run here, irqs re-enabled */
>      arch_interrupt_async_exit_prepare(regs); /* nap_adjust_return was here */
>      irqentry_exit(regs, state);
> 
> irq_exit_rcu() calls invoke_softirq() -> do_softirq_own_stack(), which
> runs softirqs with hardware interrupts re-enabled.  A nested async
> interrupt can therefore arrive while _TLF_NAPPING is still set.  That
> nested interrupt reaches nap_adjust_return() in its own
> arch_interrupt_async_exit_prepare() call, finds _TLF_NAPPING set, and
> redirects *its own* regs->NIP to power4_idle_nap_return.  Returning via
> that blr with an unrelated LR on the softirq stack jumps to a garbage
> address, causing the observed crashes.
> 
> The comment that previously lived in arch_interrupt_async_exit_prepare()
> even described this exact hazard ("must come before irq_exit()"), but
> nap_adjust_return() was placed after irq_exit_rcu() in the macro, so
> the protection was never effective.
> 
> Fix this by calling nap_adjust_return() inside DEFINE_INTERRUPT_HANDLER_ASYNC
> immediately before irq_exit_rcu(), ensuring _TLF_NAPPING is cleared and
> regs->NIP is adjusted before any code that can re-enable interrupts or
> invoke softirqs runs.  Move the explanatory comment into
> nap_adjust_return() itself and remove it from arch_interrupt_async_exit_prepare().
> 
> Fixes: bee25f97ad24 ("powerpc: Enable GENERIC_ENTRY feature")
> Closes: https://lore.kernel.org/all/87wlvazrdy.fsf@igel.home/
> Reported-by: Andreas Schwab <schwab@linux-m68k.org>
> Signed-off-by: Mukesh Kumar Chaurasiya (IBM) <mkchauras@gmail.com>
> ---
>   arch/powerpc/include/asm/entry-common.h | 15 +++++++--------
>   arch/powerpc/include/asm/interrupt.h    |  1 +
>   2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/entry-common.h b/arch/powerpc/include/asm/entry-common.h
> index fc636c42e89a..c5adb5006361 100644
> --- a/arch/powerpc/include/asm/entry-common.h
> +++ b/arch/powerpc/include/asm/entry-common.h
> @@ -66,6 +66,13 @@ static inline void srr_regs_clobbered(void)
>   static inline void nap_adjust_return(struct pt_regs *regs)
>   {
>   #ifdef CONFIG_PPC_970_NAP
> +	/*
> +	 * Adjust the nap return address before irq_exit_rcu(). irq_exit_rcu()
> +	 * may invoke softirqs with interrupts re-enabled, allowing a nested
> +	 * async interrupt to arrive. If _TLF_NAPPING is still set at that
> +	 * point, the nested interrupt would erroneously redirect its own
> +	 * return address to power4_idle_nap_return, corrupting the stack.
> +	 */
>   	if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
>   		/* Can avoid a test-and-clear because NMIs do not call this */
>   		clear_thread_local_flags(_TLF_NAPPING);
> @@ -286,14 +293,6 @@ static inline void arch_interrupt_async_enter_prepare(struct pt_regs *regs)
>   
>   static inline void arch_interrupt_async_exit_prepare(struct pt_regs *regs)
>   {
> -	/*
> -	 * Adjust at exit so the main handler sees the true NIA. This must
> -	 * come before irq_exit() because irq_exit can enable interrupts, and
> -	 * if another interrupt is taken before nap_adjust_return has run
> -	 * here, then that interrupt would return directly to idle nap return.
> -	 */
> -	nap_adjust_return(regs);
> -
>   	arch_interrupt_exit_prepare(regs);
>   }

This makes arch_interrupt_async_exit_prepare same as arch_interrupt_exit_prepare.
Maybe remove arch_interrupt_async_exit_prepare?

>   
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index fb42a664ae54..1b45a49e9bed 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -246,6 +246,7 @@ interrupt_handler void func(struct pt_regs *regs)			\
>   	instrumentation_begin();					\
>   	irq_enter_rcu();						\
>   	____##func (regs);						\
> +	nap_adjust_return(regs);					\
>   	irq_exit_rcu();							\
>   	instrumentation_end();						\
>   	arch_interrupt_async_exit_prepare(regs);			\