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

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

[Raoni Fassina Firmino]

Repeated the same tests as the upstream code on top of v5.10.14 and
v5.9.16, 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. I hope I had tagged this
correctly in the patch.

The commit message bellow is cherry-picked from the upstream commit, I
am not sure what should I do with the footer, I left it as-is and just
added a [rff: Backported] at the end.

---- 8< ----

commit 24321ac668e452a4942598533d267805f291fdc9 upstream.

This backport differ from the upstream patch in the way to set the
sigtramp offsets, after 5.10 VDSO symbols offsets are retrieved at
buildtime and before, in this patch it uses the runtime generated
offsets logic.

Commit 0138ba5783ae ("powerpc/64/signal: Balance return predictor
stack in signal trampoline") changed __kernel_sigtramp_rt64() VDSO and
trampoline code, and introduced a regression in the way glibc's
backtrace()[1] detects the signal-handler stack frame. Apart from the
practical implications, __kernel_sigtramp_rt64() was a VDSO function
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 behaviour of 0138ba5783ae by adding a new symbol
to serve as the jump target from the kernel to the trampoline. Now the
trampoline has two parts, a new entry point and the old return point.

[1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-January/223194.html

Fixes: 0138ba5783ae ("powerpc/64/signal: Balance return predictor stack in signal trampoline")
Cc: stable@vger.kernel.org # v5.9+
Signed-off-by: Raoni Fassina Firmino <raoni@linux.ibm.com>
Acked-by: Nicholas Piggin <npiggin@gmail.com>
[mpe: Minor tweaks to change log formatting, add stable tag]
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Link: https://lore.kernel.org/r/20210201200505.iz46ubcizipnkcxe@work-tp
[rff: Backported]
---
 arch/powerpc/kernel/vdso.c              |  2 +-
 arch/powerpc/kernel/vdso64/sigtramp.S   | 11 ++++++++++-
 arch/powerpc/kernel/vdso64/vdso64.lds.S |  1 +
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
index 8dad44262e75..495ffc9cf5e2 100644
--- a/arch/powerpc/kernel/vdso.c
+++ b/arch/powerpc/kernel/vdso.c
@@ -475,7 +475,7 @@ static __init void vdso_setup_trampolines(struct lib32_elfinfo *v32,
 	 */
 
 #ifdef CONFIG_PPC64
-	vdso64_rt_sigtramp = find_function64(v64, "__kernel_sigtramp_rt64");
+	vdso64_rt_sigtramp = find_function64(v64, "__kernel_start_sigtramp_rt64");
 #endif
 	vdso32_sigtramp	   = find_function32(v32, "__kernel_sigtramp32");
 	vdso32_rt_sigtramp = find_function32(v32, "__kernel_sigtramp_rt32");
diff --git a/arch/powerpc/kernel/vdso64/sigtramp.S b/arch/powerpc/kernel/vdso64/sigtramp.S
index bbf68cd01088..2d4067561293 100644
--- a/arch/powerpc/kernel/vdso64/sigtramp.S
+++ b/arch/powerpc/kernel/vdso64/sigtramp.S
@@ -15,11 +15,20 @@
 
 	.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.
+ */
 	.balign 8
 	.balign IFETCH_ALIGN_BYTES
-V_FUNCTION_BEGIN(__kernel_sigtramp_rt64)
+V_FUNCTION_BEGIN(__kernel_start_sigtramp_rt64)
 .Lsigrt_start:
 	bctrl	/* call the handler */
+V_FUNCTION_END(__kernel_start_sigtramp_rt64)
+V_FUNCTION_BEGIN(__kernel_sigtramp_rt64)
 	addi	r1, r1, __SIGNAL_FRAMESIZE
 	li	r0,__NR_rt_sigreturn
 	sc
diff --git a/arch/powerpc/kernel/vdso64/vdso64.lds.S b/arch/powerpc/kernel/vdso64/vdso64.lds.S
index 256fb9720298..bd120f590b9e 100644
--- a/arch/powerpc/kernel/vdso64/vdso64.lds.S
+++ b/arch/powerpc/kernel/vdso64/vdso64.lds.S
@@ -150,6 +150,7 @@ VERSION
 		__kernel_get_tbfreq;
 		__kernel_sync_dicache;
 		__kernel_sync_dicache_p5;
+		__kernel_start_sigtramp_rt64;
 		__kernel_sigtramp_rt64;
 		__kernel_getcpu;
 		__kernel_time;

base-commit: b0c8835fc649454c33371f4617111cb5d60463e1
-- 
2.26.2

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

[Greg KH]

On Tue, Feb 09, 2021 at 12:02:40PM -0300, Raoni Fassina Firmino wrote:
> Repeated the same tests as the upstream code on top of v5.10.14 and
> v5.9.16, 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. I hope I had tagged this
> correctly in the patch.

Now added to 5.10.y, 5.9.y is long end-of-life so there is nothing we
can do there, sorry.

thanks for the backport,

greg k-h

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

[Raoni Fassina Firmino]

On Wed, Feb 10, 2021 at 03:27:05PM +0100, Greg KH wrote:
> On Tue, Feb 09, 2021 at 12:02:40PM -0300, Raoni Fassina Firmino wrote:
> > Repeated the same tests as the upstream code on top of v5.10.14 and
> > v5.9.16, 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. I hope I had tagged this
> > correctly in the patch.
> 
> Now added to 5.10.y, 5.9.y is long end-of-life so there is nothing we
> can do there, sorry.

No problem, I didn't know 5.9.y was already EOL, that is on me.

Thanks,

o/
Raoni

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

[Greg KH]

On Thu, Feb 11, 2021 at 08:28:09AM -0300, Raoni Fassina Firmino wrote:
> On Wed, Feb 10, 2021 at 03:27:05PM +0100, Greg KH wrote:
> > On Tue, Feb 09, 2021 at 12:02:40PM -0300, Raoni Fassina Firmino wrote:
> > > Repeated the same tests as the upstream code on top of v5.10.14 and
> > > v5.9.16, 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. I hope I had tagged this
> > > correctly in the patch.
> > 
> > Now added to 5.10.y, 5.9.y is long end-of-life so there is nothing we
> > can do there, sorry.
> 
> No problem, I didn't know 5.9.y was already EOL, that is on me.

Hint, in the future www.kernel.org shows you this :)

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

[Raoni Fassina Firmino]

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< ----

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.  */
 	.balign 8
 	.balign IFETCH_ALIGN_BYTES
-V_FUNCTION_BEGIN(__kernel_sigtramp_rt64)
+V_FUNCTION_BEGIN(__kernel_start_sigtramp_rt64)
 .Lsigrt_start:
 	bctrl	/* call the handler */
+V_FUNCTION_END(__kernel_start_sigtramp_rt64)
+V_FUNCTION_BEGIN(__kernel_sigtramp_rt64)
 	addi	r1, r1, __SIGNAL_FRAMESIZE
 	li	r0,__NR_rt_sigreturn
 	sc
diff --git a/arch/powerpc/kernel/vdso64/vdso64.lds.S b/arch/powerpc/kernel/vdso64/vdso64.lds.S
index 6164d1a1ba11..2f3c359cacd3 100644
--- a/arch/powerpc/kernel/vdso64/vdso64.lds.S
+++ b/arch/powerpc/kernel/vdso64/vdso64.lds.S
@@ -131,4 +131,4 @@ VERSION
 /*
  * Make the sigreturn code visible to the kernel.
  */
-VDSO_sigtramp_rt64	= __kernel_sigtramp_rt64;
+VDSO_sigtramp_rt64	= __kernel_start_sigtramp_rt64;

base-commit: 76c057c84d286140c6c416c3b4ba832cd1d8984e
-- 
2.26.2

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

[Nicholas Piggin]

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.

Thanks,
Nick

> 
> 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! :)

Thanks,
Nick

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

[Michael Ellerman]

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

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

[Raoni Fassina Firmino]

On Tue, Feb 02, 2021 at 05:41:35PM +1000, Nicholas Piggin wrote:
> 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).

For glibc from the beginning I planned to send a patch as well and
fortunately it got in[1] in time for yesterday's 2.33 release.  That
patch is meant to address kernels 5.9 and 5.10 but is also compatible
with older kernels and with this patch. So everyone should be compatible
with everyone else :-) (except unpatched kernels 5.9 and 5.10 with
unpatched glibcs prior to 2.33)

I don't know about musl, I took a look and maybe wrong here but didn't
find any backtrace() implementation and it seems that it uses its own
return code for the trampoline, but I am not sure.

Rich mentioned[2] that he didn't see how it would break musl and was
waiting for feedback from ppc users to be sure.

Thanks for the review :-)

o/
Raoni

[1] https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=5ee506ed35a2c9184bcb1fb5e79b6cceb9bb0dd1
[2] https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-January/223198.html

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

[Michael Ellerman]

On Mon, 1 Feb 2021 17:05:05 -0300, Raoni Fassina Firmino wrote:
> 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.
> 
> [...]

Applied to powerpc/fixes.

[1/1] powerpc/64/signal: Fix regression in __kernel_sigtramp_rt64() semantics
      https://git.kernel.org/powerpc/c/24321ac668e452a4942598533d267805f291fdc9

cheers