Messages in this thread | | | Subject | Re: [PATCH 21/23] arm64: ilp32: introduce ilp32-specific handlers for sigframe and ucontext | From | "Zhangjian (Bamvor)" <> | Date | Thu, 16 Jun 2016 19:21:22 +0800 |
| |
Hi,
On 2016/6/13 1:44, Yury Norov wrote: > Hi Bamvor, > > Sorry, I missed this patch. > > On Sat, Jun 04, 2016 at 07:34:32PM +0800, Zhangjian (Bamvor) wrote: >> Hi, >> >> I found an issue of unwind with the following code. The correct backtrace >> should be: >> (gdb) where >> #0 0x004004d0 in my_sig (sig=11) at test_force3.c:16 >> #1 <signal handler called> >> #2 func2 (num=0) at test_force3.c:22 >> #3 0x00400540 in func1 (num=1) at test_force3.c:28 >> #4 0x00400574 in main (argc=1, argv=0xffd7bc04) at test_force3.c:33 >> >> Without my patch, the backtrace is: >> (gdb) where >> #0 0x00400490 in my_sig (sig=11) at test_force3.c:16 >> #1 <signal handler called> >> #2 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33 >> >> With my patch which fix the wrong frame pointer(setup_return calculate the offset >> of fp through ilp32_sigframe instead of sigfreame), the backtrace is: >> (gdb) where >> #0 0x00400490 in my_sig (sig=11) at test_force3.c:16 >> #1 <signal handler called> >> #2 func1 () at test_force3.c:28 >> #3 0x004004e4 in main (argc=1, argv=0xffe6f8f4) at test_force3.c:33 >> >> I am not sure there is still some issue in kernel. But it seem that the gdb of ilp32 >> does not work correctly when unwind without framepointer. >> >> The test code is: >> >> From 7e364a765097f57aed2d73f94c1688c2e7343e79 Mon Sep 17 00:00:00 2001 >> From: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com> >> Date: Sat, 4 Jun 2016 14:30:05 +0800 >> Subject: [PATCH] arm64: ilp32: fix for wrong fp offset when calculate the >> new fp >> >> ILP32 define its own sigframe(ilp32_sigframe) because of the >> difference uc_context. setup_return do not use ilp32 specific >> sigframe to calculate the new offset of fp which lead to wrong >> fp in signal handler. At this circumstance, gdb backtrace will miss >> one item: >> (gdb) where >> >> It should be: >> (gdb) where >> >> The test code is as follows: >> >> void my_sig(int sig) >> { >> printf("sig=%d\n", sig); >> *(int *)0 = 0x0; >> } >> >> void func2(int num) >> { >> printf("%s: %d\n", __FUNCTION__, num); >> *(int *)0 = 0x0; >> func2(num-1); >> } >> >> void func1(int num) >> { >> printf("%s\n", __FUNCTION__); >> func2(num - 1); >> } >> >> int main(int argc, char **argv) >> { >> signal(11, my_sig); >> func1(argc); >> return 0; >> } >> >> This patch fix this by passing the correct offset of fp to >> setup_return. >> Test pass on both ILP32 and LP64 in aarch64 EE. >> >> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@huawei.com> >> --- >> arch/arm64/include/asm/signal_common.h | 3 ++- >> arch/arm64/kernel/signal.c | 9 +++++---- >> arch/arm64/kernel/signal_ilp32.c | 4 ++-- >> 3 files changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/arch/arm64/include/asm/signal_common.h b/arch/arm64/include/asm/signal_common.h >> index de93c71..a5d7b63 100644 >> --- a/arch/arm64/include/asm/signal_common.h >> +++ b/arch/arm64/include/asm/signal_common.h >> @@ -29,6 +29,7 @@ int setup_sigcontex(struct sigcontext __user *uc_mcontext, >> struct pt_regs *regs); >> int restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sf); >> void setup_return(struct pt_regs *regs, struct k_sigaction *ka, >> - void __user *frame, off_t sigframe_off, int usig); >> + void __user *frame, off_t sigframe_off, off_t fp_off, >> + int usig); >> >> #endif /* __ASM_SIGNAL_COMMON_H */ >> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c >> index 038bebe..e66a6e9 100644 >> --- a/arch/arm64/kernel/signal.c >> +++ b/arch/arm64/kernel/signal.c >> @@ -256,14 +256,14 @@ static struct rt_sigframe __user *get_sigframe(struct ksignal *ksig, >> } >> >> void setup_return(struct pt_regs *regs, struct k_sigaction *ka, >> - void __user *frame, off_t sigframe_off, int usig) >> + void __user *frame, off_t sigframe_off, off_t fp_off, >> + int usig) >> { >> __sigrestore_t sigtramp; >> >> regs->regs[0] = usig; >> regs->sp = (unsigned long)frame; >> - regs->regs[29] = regs->sp + sigframe_off + >> - offsetof(struct sigframe, fp); >> + regs->regs[29] = regs->sp + sigframe_off + fp_off; > > I think you are right here. The only nitpick is what for we send 2 > offsets just to add one to another inside setup_return()? > We can do like this: > > void setup_return(struct pt_regs *regs, struct k_sigaction *ka, > void __user *frame, off_t fp_off, int usig) > { > __sigrestore_t sigtramp; > > regs->regs[0] = usig; > regs->sp = (unsigned long)frame; > regs->regs[29] = regs->sp + fp_off; > [...] > } > > Where fp_off calculation is done by caller. > > setup_return(regs, &ksig->ka, frame, > offsetof(struct rt_sigframe, sig) + offsetof(struct sigframe, fp), > usig); > > For me it's more clear to understand what happens with this approach. > I don't think struct rt_sigframe will grow, but we can even introduce > some helper for it: > #define RT_SIGFRAME_FP_POS (offsetof(struct rt_sigframe, sig) + offsetof(struct sigframe, fp)) > > If no objections, I'll apply your patch with my fix in next series. Sure. Thanks.
Regards
Bamvor > >> regs->pc = (unsigned long)ka->sa.sa_handler; >> >> if (ka->sa.sa_flags & SA_RESTORER) >> @@ -294,7 +294,8 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set, >> err |= setup_sigframe(&frame->sig, regs, set); >> if (err == 0) { >> setup_return(regs, &ksig->ka, frame, >> - offsetof(struct rt_sigframe, sig), usig); >> + offsetof(struct rt_sigframe, sig), >> + offsetof(struct sigframe, fp), usig); >> if (ksig->ka.sa.sa_flags & SA_SIGINFO) { >> err |= copy_siginfo_to_user(&frame->info, &ksig->info); >> regs->regs[1] = (unsigned long)&frame->info; >> diff --git a/arch/arm64/kernel/signal_ilp32.c b/arch/arm64/kernel/signal_ilp32.c >> index a8ea73e..9030f14 100644 >> --- a/arch/arm64/kernel/signal_ilp32.c >> +++ b/arch/arm64/kernel/signal_ilp32.c >> @@ -147,7 +147,6 @@ static struct ilp32_rt_sigframe __user *ilp32_get_sigframe(struct ksignal *ksig, >> struct ilp32_rt_sigframe __user *frame; >> >> sp = sp_top = sigsp(regs->sp, ksig); >> - >> sp = (sp - sizeof(struct ilp32_rt_sigframe)) & ~15; >> frame = (struct ilp32_rt_sigframe __user *)sp; >> >> @@ -183,7 +182,8 @@ int ilp32_setup_rt_frame(int usig, struct ksignal *ksig, >> err |= setup_ilp32_sigframe(&frame->sig, regs, set); >> if (err == 0) { >> setup_return(regs, &ksig->ka, frame, >> - offsetof(struct ilp32_rt_sigframe, sig), usig); >> + offsetof(struct ilp32_rt_sigframe, sig), >> + offsetof(struct ilp32_sigframe, fp), usig); >> regs->regs[1] = (unsigned long)&frame->info; >> regs->regs[2] = (unsigned long)&frame->sig.uc; >> } >> -- >> 1.8.4.5 >> >> Regards >> >> Bamvor >> >>
| |