Re: [PATCH] powerpc/64/signal: Fix regression in __kernel_sigtramp_rt64 semantics

[Michael Ellerman <mpe@ellerman.id.au>]

Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Raoni Fassina Firmino's message of February 2, 2021 6:05 am:
>> Tested on powerpc64 and powerpc64le, with a glibc build and running the
>> affected glibc's testcase[2], inspected that glibc's backtrace() now gives
>> the correct result and gdb backtrace also keeps working as before.
>> 
>> I believe this should be backported to releases 5.9 and 5.10 as userspace
>> is affected in this releases.
>> 
>> ---- 8< ----
>
> Thanks for this, I don't know the glibc code but the kernel change seems 
> okay to me.

I turned this into an Acked-by from you.
 
>> A Change[1] in __kernel_sigtramp_rt64 VDSO and trampoline code introduced a
>> regression in the way glibc's backtrace()[2] detects the signal-handler
>> stack frame.  Apart from the practical implications, __kernel_sigtram_rt64
>> was a VDSO with the semantics that it is a function you can call from
>> userspace to end a signal handling.  Now this semantics are no longer
>> valid.
>> 
>> I believe the aforementioned change affects all releases since 5.9.
>> 
>> This patch tries to fix both the semantics and practical aspect of
>> __kernel_sigtramp_rt64 returning it to the previous code, whilst keeping
>> the intended behavior from[1] by adding a new symbol to serve as the jump
>> target from the kernel to the trampoline. Now the trampoline has two parts,
>> an new entry point and the old return point.
>> 
>> [1] commit 0138ba5783ae0dcc799ad401a1e8ac8333790df9 ("powerpc/64/signal:
>>     Balance return predictor stack in signal trampoline")
>> [2] https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-January/223194.html
>> 
>> Fixes: 0138ba5783ae ("powerpc/64/signal: Balance return predictor stack in signal trampoline")
>> Signed-off-by: Raoni Fassina Firmino <raoni@linux.ibm.com>
>> ---
>>  arch/powerpc/kernel/vdso64/sigtramp.S   | 9 ++++++++-
>>  arch/powerpc/kernel/vdso64/vdso64.lds.S | 2 +-
>>  2 files changed, 9 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/vdso64/sigtramp.S b/arch/powerpc/kernel/vdso64/sigtramp.S
>> index bbf68cd01088..f0fd8d2a9fc4 100644
>> --- a/arch/powerpc/kernel/vdso64/sigtramp.S
>> +++ b/arch/powerpc/kernel/vdso64/sigtramp.S
>> @@ -15,11 +15,18 @@
>>  
>>  	.text
>>  
>> +/* __kernel_start_sigtramp_rt64 and __kernel_sigtramp_rt64 together
>> +   are one function split in two parts. The kernel jumps to the former
>> +   and the signal handler indirectly (by blr) returns to the latter.
>> +   __kernel_sigtramp_rt64 needs to point to the return address so
>> +   glibc can correctly identify the trampoline stack frame.  */
>
> Are you planning to update glibc to cope with this as well? Any idea 
> about musl? If so, including version numbers would be good (not that
> it's really a problem to carry this patch around).
>
> I was just about to ask to turn the comment into kernel style, but the
> whole file has this style so nevermind about that! :)

Yeah, copying the existing style was the right thing to do.

... but I really can't deal with that comment style so I reformatted it
to match kernel style :)

Parts of that file use two-space indents as well, the whole thing could
do with a pass through clang-format or similar one day.

cheers