Re: [PATCH 3/3] powerpc/kexec: fix double get_cpu() imbalance in kexec_prepare_cpus

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

On 6/3/26 11:44 AM, Aboorva Devarajan wrote:
> Hi Shrikanth,
> 
> On Mon, 2026-05-18 at 11:32 +0530, Shrikanth Hegde wrote:
>> Hi Aboorva.
>>
>> On 5/18/26 10:38 AM, Aboorva Devarajan wrote:
>>> kexec_prepare_cpus_wait() calls get_cpu() internally to obtain the
>>> current CPU id. kexec_prepare_cpus() calls kexec_prepare_cpus_wait()
>>> twice -- once for KEXEC_STATE_IRQS_OFF and once for
>>> KEXEC_STATE_REAL_MODE -- but only issues a single put_cpu() at the end,
>>> leaving preempt_count elevated by one extra nesting level.
>>>
>>> In practice the imbalance does not trigger a 'scheduling while atomic'
>>> splat because the kexec path is a one-way trip: IRQs are already
>>> disabled, no schedule() occurs after the leak, and
>>> default_machine_kexec() overwrites preempt_count with HARDIRQ_OFFSET
>>> before jumping into kexec_sequence() which never returns. However the
>>> bookkeeping is still wrong.
>>>
>>> Lift the get_cpu()/put_cpu() pair into kexec_prepare_cpus() so it is
>>> called exactly once, and pass the CPU id to kexec_prepare_cpus_wait()
>>> as a parameter. This keeps preempt_count correctly balanced.
>>>
>>> Fixes: 1fc711f7ffb01 ("powerpc/kexec: Fix race in kexec shutdown")
>>> Signed-off-by: Aboorva Devarajan <aboorvad@linux.ibm.com>
>>> ---
>>>    arch/powerpc/kexec/core_64.c | 15 ++++++++-------
>>>    1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kexec/core_64.c b/arch/powerpc/kexec/core_64.c
>>> index 825ab8a88f18e..9d7e5a1e6e5b8 100644
>>> --- a/arch/powerpc/kexec/core_64.c
>>> +++ b/arch/powerpc/kexec/core_64.c
>>> @@ -164,12 +164,11 @@ static void kexec_smp_down(void *arg)
>>>    	/* NOTREACHED */
>>>    }
>>>    
>>> -static void kexec_prepare_cpus_wait(int wait_state)
>>> +static void kexec_prepare_cpus_wait(int wait_state, int my_cpu)
>>>    {
>>> -	int my_cpu, i, notified=-1;
>>> +	int i, notified = -1;
>>>    
>>>    	hw_breakpoint_disable();
>>> -	my_cpu = get_cpu();
>>>    	/* Make sure each CPU has at least made it to the state we need.
>>>    	 *
>>>    	 * FIXME: There is a (slim) chance of a problem if not all of the CPUs
>>> @@ -246,6 +245,8 @@ static void wake_offline_cpus(void)
>>>    
>>>    static void kexec_prepare_cpus(void)
>>>    {
>>> +	int my_cpu;
>>> +
>>>    	wake_offline_cpus();
>>>    	smp_call_function(kexec_smp_down, NULL, /* wait */0);
>>>    	local_irq_disable();
>>> @@ -254,7 +255,8 @@ static void kexec_prepare_cpus(void)
>>>    	mb(); /* make sure IRQs are disabled before we say they are */
>>>    	get_paca()->kexec_state = KEXEC_STATE_IRQS_OFF;
>>>    
>>> -	kexec_prepare_cpus_wait(KEXEC_STATE_IRQS_OFF);
>>> +	my_cpu = get_cpu();
> 
>> raw_smp_processor_id() is better here. All it needs is get current cpu?
>> caller does irq_disable above and that renders call for get_cpu un-necessary.
> 
> Agreed, get_cpu() is not needed here. kexec_prepare_cpus() already does
> local_irq_disable()/hard_irq_disable() before calling
> kexec_prepare_cpus_wait(), so we only need the current cpu id.
> 
> I will go ahead with smp_processor_id() rather than
> raw_smp_processor_id() to stay consistent with Patch 2 and to keep the
> CONFIG_DEBUG_PREEMPT check.


If the irq's are disabled then use raw_smp_processor_id() in both the places.
For patch2, just put a comment saying irq's are disabled when its get there.

> 
>>>    
>>> @@ -262,13 +264,12 @@ static void kexec_prepare_cpus(void)
>>>    	 * Before removing MMU mappings make sure all CPUs have entered real
>>>    	 * mode:
>>>    	 */
>>> -	kexec_prepare_cpus_wait(KEXEC_STATE_REAL_MODE);
>>> +	kexec_prepare_cpus_wait(KEXEC_STATE_REAL_MODE, my_cpu);
>>> +	put_cpu();
>>>    
>>>    	/* after we tell the others to go down */
>>>    	if (ppc_md.kexec_cpu_down)
>>>    		ppc_md.kexec_cpu_down(0, 0);
>>> -
>>> -	put_cpu();
>>>    }
>>>    
>>>    #else /* ! SMP */
> 
> Regards,
> Aboorva