Messages in this thread Patch in this message | | | From | Guo Ren <> | Date | Mon, 17 Aug 2020 20:47:21 +0800 | Subject | Re: [PATCH v3 3/7] riscv: Fixup kprobes handler couldn't change pc |
| |
On Sat, Aug 15, 2020 at 6:36 AM Palmer Dabbelt <palmerdabbelt@google.com> wrote: > > On Mon, 13 Jul 2020 16:39:18 PDT (-0700), guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > > > The "Changing Execution Path" section in the Documentation/kprobes.txt > > said: > > > > Since kprobes can probe into a running kernel code, it can change the > > register set, including instruction pointer. > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > Cc: Palmer Dabbelt <palmerdabbelt@google.com> > > --- > > arch/riscv/kernel/mcount-dyn.S | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S > > index 35a6ed7..4b58b54 100644 > > --- a/arch/riscv/kernel/mcount-dyn.S > > +++ b/arch/riscv/kernel/mcount-dyn.S > > @@ -123,6 +123,7 @@ ENDPROC(ftrace_caller) > > sd ra, (PT_SIZE_ON_STACK+8)(sp) > > addi s0, sp, (PT_SIZE_ON_STACK+16) > > > > + sd ra, PT_EPC(sp) > > sd x1, PT_RA(sp) > > sd x2, PT_SP(sp) > > sd x3, PT_GP(sp) > > So that's definately not going to be EPC any more. I'm not sure that field is > sanely named, though, as it's really just the PC when it comes to other ptrace > stuff. > > > @@ -157,6 +158,7 @@ ENDPROC(ftrace_caller) > > .endm > > > > .macro RESTORE_ALL > > + ld ra, PT_EPC(sp) > > ld x1, PT_RA(sp) > > x1 is ra, so loading it twice doesn't seem reasonable. > > > ld x2, PT_SP(sp) > > ld x3, PT_GP(sp) > > @@ -190,7 +192,6 @@ ENDPROC(ftrace_caller) > > ld x31, PT_T6(sp) > > > > ld s0, (PT_SIZE_ON_STACK)(sp) > > - ld ra, (PT_SIZE_ON_STACK+8)(sp) > > addi sp, sp, (PT_SIZE_ON_STACK+16) > > .endm > > If you're dropping the load you should drop the store above as well. In > general this seems kind of mixed up, both before and after this patch.
This is a wrong patch, it should be:
diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S index 35a6ed7..d82b8f0 100644 --- a/arch/riscv/kernel/mcount-dyn.S +++ b/arch/riscv/kernel/mcount-dyn.S @@ -120,10 +120,10 @@ ENDPROC(ftrace_caller) .macro SAVE_ALL addi sp, sp, -(PT_SIZE_ON_STACK+16) sd s0, (PT_SIZE_ON_STACK)(sp) - sd ra, (PT_SIZE_ON_STACK+8)(sp) addi s0, sp, (PT_SIZE_ON_STACK+16)
- sd x1, PT_RA(sp) + sd ra, PT_EPC(sp) + sd ra, PT_RA(sp) sd x2, PT_SP(sp) sd x3, PT_GP(sp) sd x4, PT_TP(sp) @@ -157,7 +157,7 @@ ENDPROC(ftrace_caller) .endm
.macro RESTORE_ALL - ld x1, PT_RA(sp) + ld ra, PT_EPC(sp) ld x2, PT_SP(sp) ld x3, PT_GP(sp) ld x4, PT_TP(sp) @@ -190,7 +190,6 @@ ENDPROC(ftrace_caller) ld x31, PT_T6(sp)
ld s0, (PT_SIZE_ON_STACK)(sp) - ld ra, (PT_SIZE_ON_STACK+8)(sp) addi sp, sp, (PT_SIZE_ON_STACK+16) .endm
Now, I'm developing livepatch and they are so mixed features (kprobe, livepatch, ftrace, optprobes, STACK_WALK, -fpatchable-function-entry 'no -pg'). I'll test this patch in the next version of the patchset. Thx for the review.
-- Best Regards Guo Ren
ML: https://lore.kernel.org/linux-csky/
| |