Re: [PATCH] powerpc/ftrace: Don't restore r13 during ftrace_regs_caller

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

Hi Hari,

On 9/21/26 12:07 PM, Hari Bathini wrote:
> 
> 
> On 18/09/26 8:38 pm, Shrikanth Hegde wrote:
>> Michal reported a stack-protector failure and subsequent panic when
>> running kernel builds. This was observed with full/lazy preemption.
>> Initially it was suspected as KVM, but later turned out to be due
>> to a bcc tool running in parallel.
>>
>> Issue was recreated using a bcc tool.
>> For example, running below in parallel leads to crash.
>> ./funccount sched* -d 100 and make -j 64
>>
>> The same crash was observed when running kprobe for schedule() function,
>> while simpler function tracer for schedule() didn't cause the crash.
>> This helped to narrow it down to ftrace backed kprobes area.
>>
>> The crash occurs as follows:
>>
>> ftrace_regs_caller entry on CPU A
>>      |
>>      +-> save r13 = CPU A PACA into pt_regs
>>      |
>>      +-> call kprobe_ftrace_handler()
>>              |
>>              +-> ftrace_test_recursion_unlock()
>>                      |
>>                      +-> preempt_enable
>>                      +-> task can schedule and migrate to CPU B
>>                      +-> task resumes with live r13 = CPU B PACA
>>      |
>>      +-> REST_GPRS(2, 31)
>>              |
>>              +-> restore saved r13 = CPU A PACA
>>      |
>>      |-> The task then continues running on CPU B with r13 pointing
>>      |   to CPU A's PACA.
>>
>> The stack-protector canary is accessed through the PACA. After the task
>> migrates, CPU A may run a different task and update its PACA with that
>> task's canary. Restoring the saved r13 then causes the migrated task's
>> saved stack canary to be compared against the canary in CPU A's PACA,
>> resulting in a stack-protector failure.
>>
>> Similarly, current is resolved through the PACA. With a stale r13,
>> preempt_count() can access the state of the task referenced by CPU A's
>> PACA instead of the task running on CPU B. This results in corrupted
>> preempt-count warnings and scheduling-while-atomic failures.
>>
>> This path for example is called when using kprobes and parallel kernel builds
>> can cause preemptions during ftrace_test_recursion_unlock.
>>
>> Do not restore r13 from the saved register frame. If the task did not
>> migrate, the live r13 already has the saved value. If it migrated, the
>> live r13 contains the correct PACA pointer for the CPU on which the task
>> resumed.
>>
> 
> Looks good to me except for a minor nit below.
> 
> Reviewed-by: Hari Bathini <hbathini@linux.ibm.com>
> 
>> Fixes: 153086644fd1 ("powerpc/ftrace: Add support for -mprofile-kernel ftrace ABI")
>> Reported-by: Michal Suchánek <msuchanek@suse.de>
>> Closes: https://lore.kernel.org/all/aqKfsVArHHaIK6M9@kunlun.suse.cz/
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>> PS:
>> Fixes is the initial commit that introduced this restore regs almost
>> 10 years ago, all commit afterwords are code refactors changing the
>> code layout. Also backporting all the way maybe tricky.
>> Backport can easily happen till aebd1fb45c622.
>>
>>   arch/powerpc/kernel/trace/ftrace_entry.S | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/trace/ftrace_entry.S b/arch/powerpc/kernel/trace/ftrace_entry.S
>> index 6599fe3c6234..54c8727b48cd 100644
>> --- a/arch/powerpc/kernel/trace/ftrace_entry.S
>> +++ b/arch/powerpc/kernel/trace/ftrace_entry.S
>> @@ -220,7 +220,9 @@
>>       /* Restore gprs */
>>       .if \allregs == 1
> 
>> -    REST_GPRS(2, 31, r1)
>> +    REST_GPRS(2, 12, r1)
>> +    /* Do not restore a stale PACA pointer if the task migrated */
>> +    REST_GPRS(14, 31, r1)
> 
> Given that r13 is not paca on ppc32, shouldn't the above change
> only apply to PPC64?

Yes. Good catch. Will make the above change only for PPC64.

Thanks for reviewing.

>>       .else
>>       REST_GPRS(3, 10, r1)
>>   #if defined(CONFIG_LIVEPATCH_64) || defined(CONFIG_PPC_FTRACE_OUT_OF_LINE)
> 
> - Hari
>