* [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline
@ 2020-05-11 10:19 Nicholas Piggin
2020-07-16 12:56 ` Michael Ellerman
2021-01-22 11:27 ` Florian Weimer
0 siblings, 2 replies; 8+ messages in thread
From: Nicholas Piggin @ 2020-05-11 10:19 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Alan Modra
Returning from an interrupt or syscall to a signal handler currently
begins execution directly at the handler's entry point, with LR set to
the address of the sigreturn trampoline. When the signal handler
function returns, it runs the trampoline. It looks like this:
# interrupt at user address xyz
# kernel stuff... signal is raised
rfid
# void handler(int sig)
addis 2,12,.TOC.-.LCF0@ha
addi 2,2,.TOC.-.LCF0@l
mflr 0
std 0,16(1)
stdu 1,-96(1)
# handler stuff
ld 0,16(1)
mtlr 0
blr
# __kernel_sigtramp_rt64
addi r1,r1,__SIGNAL_FRAMESIZE
li r0,__NR_rt_sigreturn
sc
# kernel executes rt_sigreturn
rfid
# back to user address xyz
Note the blr with no matching bl. This can corrupt the return predictor.
Solve this by instead resuming execution at the signal trampoline which
then calls the signal handler. qtrace-tools link_stack checker confirms
the entire user/kernel/vdso cycle is balanced after this patch, whereas
it's not upstream.
Alan confirms the dwarf unwind info still looks good. gdb still
recognises the signal frame and can step into parent frames if it break
inside a signal handler.
Performance is pretty noisy, not a very significant change on a POWER9
here, but branch misses are consistently a lot lower on a microbenchmark:
Performance counter stats for './signal':
13,085.72 msec task-clock # 1.000 CPUs utilized
45,024,760,101 cycles # 3.441 GHz
65,102,895,542 instructions # 1.45 insn per cycle
11,271,673,787 branches # 861.372 M/sec
59,468,979 branch-misses # 0.53% of all branches
12,989.09 msec task-clock # 1.000 CPUs utilized
44,692,719,559 cycles # 3.441 GHz
65,109,984,964 instructions # 1.46 insn per cycle
11,282,136,057 branches # 868.585 M/sec
39,786,942 branch-misses # 0.35% of all branches
Cc: Alan Modra <amodra@gmail.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Since v1:
- Fix for the legacy on-stack trampoline path (TRAMP_TRACEBACK was not
updated to 4).
- Tested legacy case (must also disable no-exec stack to test this).
arch/powerpc/include/asm/ppc-opcode.h | 1 +
arch/powerpc/kernel/signal_64.c | 22 ++++++++++++----------
arch/powerpc/kernel/vdso64/sigtramp.S | 13 +++++--------
3 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index c1df75edde44..747b37f1ce09 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -329,6 +329,7 @@
#define PPC_INST_BLR 0x4e800020
#define PPC_INST_BLRL 0x4e800021
#define PPC_INST_BCTR 0x4e800420
+#define PPC_INST_BCTRL 0x4e800421
#define PPC_INST_MULLD 0x7c0001d2
#define PPC_INST_MULLW 0x7c0001d6
#define PPC_INST_MULHWU 0x7c000016
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index adfde59cf4ba..6bb14cdf256f 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -40,8 +40,8 @@
#define GP_REGS_SIZE min(sizeof(elf_gregset_t), sizeof(struct pt_regs))
#define FP_REGS_SIZE sizeof(elf_fpregset_t)
-#define TRAMP_TRACEBACK 3
-#define TRAMP_SIZE 6
+#define TRAMP_TRACEBACK 4
+#define TRAMP_SIZE 7
/*
* When we have signals to deliver, we set up on the user stack,
@@ -603,13 +603,15 @@ static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp)
int i;
long err = 0;
+ /* bctrl # call the handler */
+ err |= __put_user(PPC_INST_BCTRL, &tramp[0]);
/* addi r1, r1, __SIGNAL_FRAMESIZE # Pop the dummy stackframe */
err |= __put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) |
- (__SIGNAL_FRAMESIZE & 0xffff), &tramp[0]);
+ (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1]);
/* li r0, __NR_[rt_]sigreturn| */
- err |= __put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[1]);
+ err |= __put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2]);
/* sc */
- err |= __put_user(PPC_INST_SC, &tramp[2]);
+ err |= __put_user(PPC_INST_SC, &tramp[3]);
/* Minimal traceback info */
for (i=TRAMP_TRACEBACK; i < TRAMP_SIZE ;i++)
@@ -867,12 +869,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
/* Set up to return from userspace. */
if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) {
- regs->link = tsk->mm->context.vdso_base + vdso64_rt_sigtramp;
+ regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp;
} else {
err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
if (err)
goto badframe;
- regs->link = (unsigned long) &frame->tramp[0];
+ regs->nip = (unsigned long) &frame->tramp[0];
}
/* Allocate a dummy caller frame for the signal handler. */
@@ -881,8 +883,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
/* Set up "regs" so we "return" to the signal handler. */
if (is_elf2_task()) {
- regs->nip = (unsigned long) ksig->ka.sa.sa_handler;
- regs->gpr[12] = regs->nip;
+ regs->ctr = (unsigned long) ksig->ka.sa.sa_handler;
+ regs->gpr[12] = regs->ctr;
} else {
/* Handler is *really* a pointer to the function descriptor for
* the signal routine. The first entry in the function
@@ -892,7 +894,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
func_descr_t __user *funct_desc_ptr =
(func_descr_t __user *) ksig->ka.sa.sa_handler;
- err |= get_user(regs->nip, &funct_desc_ptr->entry);
+ err |= get_user(regs->ctr, &funct_desc_ptr->entry);
err |= get_user(regs->gpr[2], &funct_desc_ptr->toc);
}
diff --git a/arch/powerpc/kernel/vdso64/sigtramp.S b/arch/powerpc/kernel/vdso64/sigtramp.S
index a8cc0409d7d2..bbf68cd01088 100644
--- a/arch/powerpc/kernel/vdso64/sigtramp.S
+++ b/arch/powerpc/kernel/vdso64/sigtramp.S
@@ -6,6 +6,7 @@
* Copyright (C) 2004 Benjamin Herrenschmuidt (benh@kernel.crashing.org), IBM Corp.
* Copyright (C) 2004 Alan Modra (amodra@au.ibm.com)), IBM Corp.
*/
+#include <asm/cache.h> /* IFETCH_ALIGN_BYTES */
#include <asm/processor.h>
#include <asm/ppc_asm.h>
#include <asm/unistd.h>
@@ -14,21 +15,17 @@
.text
-/* The nop here is a hack. The dwarf2 unwind routines subtract 1 from
- the return address to get an address in the middle of the presumed
- call instruction. Since we don't have a call here, we artificially
- extend the range covered by the unwind info by padding before the
- real start. */
- nop
.balign 8
+ .balign IFETCH_ALIGN_BYTES
V_FUNCTION_BEGIN(__kernel_sigtramp_rt64)
-.Lsigrt_start = . - 4
+.Lsigrt_start:
+ bctrl /* call the handler */
addi r1, r1, __SIGNAL_FRAMESIZE
li r0,__NR_rt_sigreturn
sc
.Lsigrt_end:
V_FUNCTION_END(__kernel_sigtramp_rt64)
-/* The ".balign 8" above and the following zeros mimic the old stack
+/* The .balign 8 above and the following zeros mimic the old stack
trampoline layout. The last magic value is the ucontext pointer,
chosen in such a way that older libgcc unwind code returns a zero
for a sigcontext pointer. */
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline
2020-05-11 10:19 [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline Nicholas Piggin
@ 2020-07-16 12:56 ` Michael Ellerman
2021-01-22 11:27 ` Florian Weimer
1 sibling, 0 replies; 8+ messages in thread
From: Michael Ellerman @ 2020-07-16 12:56 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Alan Modra
On Mon, 11 May 2020 20:19:52 +1000, Nicholas Piggin wrote:
> Returning from an interrupt or syscall to a signal handler currently
> begins execution directly at the handler's entry point, with LR set to
> the address of the sigreturn trampoline. When the signal handler
> function returns, it runs the trampoline. It looks like this:
>
> # interrupt at user address xyz
> # kernel stuff... signal is raised
> rfid
> # void handler(int sig)
> addis 2,12,.TOC.-.LCF0@ha
> addi 2,2,.TOC.-.LCF0@l
> mflr 0
> std 0,16(1)
> stdu 1,-96(1)
> # handler stuff
> ld 0,16(1)
> mtlr 0
> blr
> # __kernel_sigtramp_rt64
> addi r1,r1,__SIGNAL_FRAMESIZE
> li r0,__NR_rt_sigreturn
> sc
> # kernel executes rt_sigreturn
> rfid
> # back to user address xyz
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/64/signal: Balance return predictor stack in signal trampoline
https://git.kernel.org/powerpc/c/0138ba5783ae0dcc799ad401a1e8ac8333790df9
cheers
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline
2020-05-11 10:19 [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline Nicholas Piggin
2020-07-16 12:56 ` Michael Ellerman
@ 2021-01-22 11:27 ` Florian Weimer
2021-01-22 14:44 ` [musl] " Rich Felker
2021-01-22 18:13 ` Raoni Fassina Firmino
1 sibling, 2 replies; 8+ messages in thread
From: Florian Weimer @ 2021-01-22 11:27 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: musl, libc-alpha, linuxppc-dev, Alan Modra
* Nicholas Piggin:
> diff --git a/arch/powerpc/kernel/vdso64/sigtramp.S b/arch/powerpc/kernel/vdso64/sigtramp.S
> index a8cc0409d7d2..bbf68cd01088 100644
> --- a/arch/powerpc/kernel/vdso64/sigtramp.S
> +++ b/arch/powerpc/kernel/vdso64/sigtramp.S
> @@ -6,6 +6,7 @@
> * Copyright (C) 2004 Benjamin Herrenschmuidt (benh@kernel.crashing.org), IBM Corp.
> * Copyright (C) 2004 Alan Modra (amodra@au.ibm.com)), IBM Corp.
> */
> +#include <asm/cache.h> /* IFETCH_ALIGN_BYTES */
> #include <asm/processor.h>
> #include <asm/ppc_asm.h>
> #include <asm/unistd.h>
> @@ -14,21 +15,17 @@
>
> .text
>
> -/* The nop here is a hack. The dwarf2 unwind routines subtract 1 from
> - the return address to get an address in the middle of the presumed
> - call instruction. Since we don't have a call here, we artificially
> - extend the range covered by the unwind info by padding before the
> - real start. */
> - nop
> .balign 8
> + .balign IFETCH_ALIGN_BYTES
> V_FUNCTION_BEGIN(__kernel_sigtramp_rt64)
> -.Lsigrt_start = . - 4
> +.Lsigrt_start:
> + bctrl /* call the handler */
> addi r1, r1, __SIGNAL_FRAMESIZE
> li r0,__NR_rt_sigreturn
> sc
> .Lsigrt_end:
> V_FUNCTION_END(__kernel_sigtramp_rt64)
> -/* The ".balign 8" above and the following zeros mimic the old stack
> +/* The .balign 8 above and the following zeros mimic the old stack
> trampoline layout. The last magic value is the ucontext pointer,
> chosen in such a way that older libgcc unwind code returns a zero
> for a sigcontext pointer. */
As far as I understand it, this breaks cancellation handling on musl and
future glibc because it is necessary to look at the signal delivery
location to see if a system call sequence has result in an action, and
that location is no longer in user code after this change.
We have a glibc test in preparation of our change, and it started
failing:
Linux 5.10 breaks sigcontext_get_pc on powerpc64
<https://sourceware.org/bugzilla/show_bug.cgi?id=27223>
Isn't it possible to avoid the return predictor desynchronization by
adding the appropriate hint?
Thanks,
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [musl] Re: [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline
2021-01-22 11:27 ` Florian Weimer
@ 2021-01-22 14:44 ` Rich Felker
2021-01-22 18:19 ` Raoni Fassina Firmino
2021-01-22 18:13 ` Raoni Fassina Firmino
1 sibling, 1 reply; 8+ messages in thread
From: Rich Felker @ 2021-01-22 14:44 UTC (permalink / raw)
To: Florian Weimer
Cc: musl, libc-alpha, linuxppc-dev, Nicholas Piggin, Alan Modra
On Fri, Jan 22, 2021 at 12:27:14PM +0100, Florian Weimer wrote:
> * Nicholas Piggin:
>
> > diff --git a/arch/powerpc/kernel/vdso64/sigtramp.S b/arch/powerpc/kernel/vdso64/sigtramp.S
> > index a8cc0409d7d2..bbf68cd01088 100644
> > --- a/arch/powerpc/kernel/vdso64/sigtramp.S
> > +++ b/arch/powerpc/kernel/vdso64/sigtramp.S
> > @@ -6,6 +6,7 @@
> > * Copyright (C) 2004 Benjamin Herrenschmuidt (benh@kernel.crashing.org), IBM Corp.
> > * Copyright (C) 2004 Alan Modra (amodra@au.ibm.com)), IBM Corp.
> > */
> > +#include <asm/cache.h> /* IFETCH_ALIGN_BYTES */
> > #include <asm/processor.h>
> > #include <asm/ppc_asm.h>
> > #include <asm/unistd.h>
> > @@ -14,21 +15,17 @@
> >
> > .text
> >
> > -/* The nop here is a hack. The dwarf2 unwind routines subtract 1 from
> > - the return address to get an address in the middle of the presumed
> > - call instruction. Since we don't have a call here, we artificially
> > - extend the range covered by the unwind info by padding before the
> > - real start. */
> > - nop
> > .balign 8
> > + .balign IFETCH_ALIGN_BYTES
> > V_FUNCTION_BEGIN(__kernel_sigtramp_rt64)
> > -.Lsigrt_start = . - 4
> > +.Lsigrt_start:
> > + bctrl /* call the handler */
> > addi r1, r1, __SIGNAL_FRAMESIZE
> > li r0,__NR_rt_sigreturn
> > sc
> > .Lsigrt_end:
> > V_FUNCTION_END(__kernel_sigtramp_rt64)
> > -/* The ".balign 8" above and the following zeros mimic the old stack
> > +/* The .balign 8 above and the following zeros mimic the old stack
> > trampoline layout. The last magic value is the ucontext pointer,
> > chosen in such a way that older libgcc unwind code returns a zero
> > for a sigcontext pointer. */
>
> As far as I understand it, this breaks cancellation handling on musl and
> future glibc because it is necessary to look at the signal delivery
> location to see if a system call sequence has result in an action, and
> that location is no longer in user code after this change.
>
> We have a glibc test in preparation of our change, and it started
> failing:
>
> Linux 5.10 breaks sigcontext_get_pc on powerpc64
> <https://sourceware.org/bugzilla/show_bug.cgi?id=27223>
>
> Isn't it possible to avoid the return predictor desynchronization by
> adding the appropriate hint?
Maybe I'm missing something but I don't see how this would break musl;
we just inspect the PC in the mcontext, which I don't see any changes
to and which should still point to the next instruction of the
interrupted context. I don't have a test environment though so I'll
have to wait for feedback from ppc users to be sure. Are there any
further details on how it's breaking glibc?
Rich
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline
2021-01-22 11:27 ` Florian Weimer
2021-01-22 14:44 ` [musl] " Rich Felker
@ 2021-01-22 18:13 ` Raoni Fassina Firmino
1 sibling, 0 replies; 8+ messages in thread
From: Raoni Fassina Firmino @ 2021-01-22 18:13 UTC (permalink / raw)
To: Florian Weimer
Cc: libc-alpha, musl, linuxppc-dev, Nicholas Piggin, Alan Modra
On Fri, Jan 22, 2021 at 12:27:14PM +0100, AL glibc-alpha wrote:
> * Nicholas Piggin:
>
> > diff --git a/arch/powerpc/kernel/vdso64/sigtramp.S b/arch/powerpc/kernel/vdso64/sigtramp.S
> > index a8cc0409d7d2..bbf68cd01088 100644
> > --- a/arch/powerpc/kernel/vdso64/sigtramp.S
> > +++ b/arch/powerpc/kernel/vdso64/sigtramp.S
> > @@ -6,6 +6,7 @@
> > * Copyright (C) 2004 Benjamin Herrenschmuidt (benh@kernel.crashing.org), IBM Corp.
> > * Copyright (C) 2004 Alan Modra (amodra@au.ibm.com)), IBM Corp.
> > */
> > +#include <asm/cache.h> /* IFETCH_ALIGN_BYTES */
> > #include <asm/processor.h>
> > #include <asm/ppc_asm.h>
> > #include <asm/unistd.h>
> > @@ -14,21 +15,17 @@
> >
> > .text
> >
> > -/* The nop here is a hack. The dwarf2 unwind routines subtract 1 from
> > - the return address to get an address in the middle of the presumed
> > - call instruction. Since we don't have a call here, we artificially
> > - extend the range covered by the unwind info by padding before the
> > - real start. */
> > - nop
> > .balign 8
> > + .balign IFETCH_ALIGN_BYTES
> > V_FUNCTION_BEGIN(__kernel_sigtramp_rt64)
> > -.Lsigrt_start = . - 4
> > +.Lsigrt_start:
> > + bctrl /* call the handler */
> > addi r1, r1, __SIGNAL_FRAMESIZE
> > li r0,__NR_rt_sigreturn
> > sc
> > .Lsigrt_end:
> > V_FUNCTION_END(__kernel_sigtramp_rt64)
> > -/* The ".balign 8" above and the following zeros mimic the old stack
> > +/* The .balign 8 above and the following zeros mimic the old stack
> > trampoline layout. The last magic value is the ucontext pointer,
> > chosen in such a way that older libgcc unwind code returns a zero
> > for a sigcontext pointer. */
>
> As far as I understand it, this breaks cancellation handling on musl and
> future glibc because it is necessary to look at the signal delivery
> location to see if a system call sequence has result in an action, and
> that location is no longer in user code after this change.
>
> We have a glibc test in preparation of our change, and it started
> failing:
>
> Linux 5.10 breaks sigcontext_get_pc on powerpc64
> <https://sourceware.org/bugzilla/show_bug.cgi?id=27223>
>
> Isn't it possible to avoid the return predictor desynchronization by
> adding the appropriate hint?
I also caught this regression, I believe it was introduced in the kernel
5.9.
I don't know enough to comment on Florian suggestion, but I am working
on some possible fixes:
On the kernel side we can keep `__kernel_sigtramp_rt64` in the original
place after `bctrl` and add a new symbol so the kernel can jump to the
right place before `bctrl`. This would ensure backward compatibility.
On the other side, this change exposed how fragile `backtrace()` is to
any changes in the trampoline code, which the libc has no control over
in this case. So maybe there is something that can be improved in how
backtrace decides that the return-address is to the trampoline. My fist
option is to test a range, after `__kernel_sigtramp_rt64` so see if the
address is inside the routine. This would be better if we can know the
size of the function, I know that the vdso.so has this info in the elf
but I don't know if it is exposed to the glibc.
As Nicholas mentioned in his patch, GDB seems to keep working just fine,
it is seems that GDB uses some heuristics to match the surround code of
the return address to identify that it is the trampoline code. So maybe
other option is to do something similar.
o/
Raoni Fassina
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [musl] Re: [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline
2021-01-22 14:44 ` [musl] " Rich Felker
@ 2021-01-22 18:19 ` Raoni Fassina Firmino
2021-01-22 18:31 ` Rich Felker
0 siblings, 1 reply; 8+ messages in thread
From: Raoni Fassina Firmino @ 2021-01-22 18:19 UTC (permalink / raw)
To: Rich Felker
Cc: Florian Weimer, libc-alpha, Alan Modra, musl, Nicholas Piggin,
linuxppc-dev
On Fri, Jan 22, 2021 at 09:44:05AM -0500, Rich Felker wrote:
> Maybe I'm missing something but I don't see how this would break musl;
> we just inspect the PC in the mcontext, which I don't see any changes
> to and which should still point to the next instruction of the
> interrupted context. I don't have a test environment though so I'll
> have to wait for feedback from ppc users to be sure. Are there any
> further details on how it's breaking glibc?
For glibc, backtrace() compares the return-address from each stack frame
to the value of `__kernel_sigtramp_rt64` to identify the frame with the
mcontext information, but now the return-address is not the start of the
routine, but the middle of it, so it fails to catch this special frame.
o/
Raoni Fassina
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [musl] Re: [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline
2021-01-22 18:19 ` Raoni Fassina Firmino
@ 2021-01-22 18:31 ` Rich Felker
2021-01-22 18:50 ` Raoni Fassina Firmino
0 siblings, 1 reply; 8+ messages in thread
From: Rich Felker @ 2021-01-22 18:31 UTC (permalink / raw)
To: Florian Weimer, musl, libc-alpha, linuxppc-dev, Nicholas Piggin,
Alan Modra
On Fri, Jan 22, 2021 at 03:19:22PM -0300, Raoni Fassina Firmino wrote:
> On Fri, Jan 22, 2021 at 09:44:05AM -0500, Rich Felker wrote:
> > Maybe I'm missing something but I don't see how this would break musl;
> > we just inspect the PC in the mcontext, which I don't see any changes
> > to and which should still point to the next instruction of the
> > interrupted context. I don't have a test environment though so I'll
> > have to wait for feedback from ppc users to be sure. Are there any
> > further details on how it's breaking glibc?
>
> For glibc, backtrace() compares the return-address from each stack frame
> to the value of `__kernel_sigtramp_rt64` to identify the frame with the
> mcontext information, but now the return-address is not the start of the
> routine, but the middle of it, so it fails to catch this special frame.
Is there a reason it's backtracing rather than just looking at the
interrupted context (pointed to by the third argument to the signal
handler)?
Rich
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [musl] Re: [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline
2021-01-22 18:31 ` Rich Felker
@ 2021-01-22 18:50 ` Raoni Fassina Firmino
0 siblings, 0 replies; 8+ messages in thread
From: Raoni Fassina Firmino @ 2021-01-22 18:50 UTC (permalink / raw)
To: Rich Felker
Cc: Florian Weimer, libc-alpha, Alan Modra, musl, Nicholas Piggin,
linuxppc-dev
On Fri, Jan 22, 2021 at 01:31:27PM -0500, Rich Felker wrote:
> On Fri, Jan 22, 2021 at 03:19:22PM -0300, Raoni Fassina Firmino wrote:
> > On Fri, Jan 22, 2021 at 09:44:05AM -0500, Rich Felker wrote:
> > > Maybe I'm missing something but I don't see how this would break musl;
> > > we just inspect the PC in the mcontext, which I don't see any changes
> > > to and which should still point to the next instruction of the
> > > interrupted context. I don't have a test environment though so I'll
> > > have to wait for feedback from ppc users to be sure. Are there any
> > > further details on how it's breaking glibc?
> >
> > For glibc, backtrace() compares the return-address from each stack frame
> > to the value of `__kernel_sigtramp_rt64` to identify the frame with the
> > mcontext information, but now the return-address is not the start of the
> > routine, but the middle of it, so it fails to catch this special frame.
>
> Is there a reason it's backtracing rather than just looking at the
> interrupted context (pointed to by the third argument to the signal
> handler)?
The regression is exposed in the backtrace() routine. More precisely,
when the backtrace() is called from inside a signal handling. What I
described is the way backtrace() uses to identify this special
situation. What is failling in glibc is the test for this.
o/
Raoni Fassina
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-22 18:53 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-11 10:19 [PATCH v2] powerpc/64/signal: balance return predictor stack in signal trampoline Nicholas Piggin
2020-07-16 12:56 ` Michael Ellerman
2021-01-22 11:27 ` Florian Weimer
2021-01-22 14:44 ` [musl] " Rich Felker
2021-01-22 18:19 ` Raoni Fassina Firmino
2021-01-22 18:31 ` Rich Felker
2021-01-22 18:50 ` Raoni Fassina Firmino
2021-01-22 18:13 ` Raoni Fassina Firmino
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.