Hi,
Kernel test bot reported that the ia64 build error on stable 4.19.y because of the commit d3380de483d5 ("ia64: kprobes: Use generic kretprobe trampoline handler"). I also found that this commit was involved by the backporting of commit f5f96e3643dc ("ia64: kprobes: Fix to pass correct trampoline address to the handler"), and this 2nd commit was backported wrong way. Actually, this 2nd commit aimed to use dereference_function_descriptor() in kprobes@ia64, but the comment (and Fixes tag) points the 1st commit. Thus I guess this mistake happened.
So I re-backport the upstream commit a7fe2378454c ("ia64: kprobes: Fix to pass correct trampoline address to the handler") correctly, without involving the 1st commit.
Thank you,
---
Masami Hiramatsu (3): Revert "ia64: kprobes: Fix to pass correct trampoline address to the handler" Revert "ia64: kprobes: Use generic kretprobe trampoline handler" ia64: kprobes: Fix to pass correct trampoline address to the handler
arch/ia64/kernel/kprobes.c | 78 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 75 insertions(+), 3 deletions(-)
-- Masami Hiramatsu (Linaro) mhiramat@kernel.org
This reverts commit f5f96e3643dc33d6117cf7047e73512046e4858b.
The commit f5f96e3643dc ("ia64: kprobes: Fix to pass correct trampoline address to the handler") was wrongly backported. It involves another commit which is a part of another bigger series, so it should not be backported to the stable tree.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- arch/ia64/kernel/kprobes.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c index 9cfd3ac027b7..8207b897b49d 100644 --- a/arch/ia64/kernel/kprobes.c +++ b/arch/ia64/kernel/kprobes.c @@ -411,8 +411,7 @@ static void kretprobe_trampoline(void)
int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs) { - regs->cr_iip = __kretprobe_trampoline_handler(regs, - dereference_function_descriptor(kretprobe_trampoline), NULL); + regs->cr_iip = __kretprobe_trampoline_handler(regs, kretprobe_trampoline, NULL); /* * By returning a non-zero value, we are telling * kprobe_handler() that we don't want the post_handler @@ -428,7 +427,7 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, ri->fp = NULL;
/* Replace the return addr with trampoline addr */ - regs->b0 = (unsigned long)dereference_function_descriptor(kretprobe_trampoline); + regs->b0 = ((struct fnptr *)kretprobe_trampoline)->ip; }
/* Check the instruction in the slot is break */ @@ -958,14 +957,14 @@ static struct kprobe trampoline_p = { int __init arch_init_kprobes(void) { trampoline_p.addr = - dereference_function_descriptor(kretprobe_trampoline); + (kprobe_opcode_t *)((struct fnptr *)kretprobe_trampoline)->ip; return register_kprobe(&trampoline_p); }
int __kprobes arch_trampoline_kprobe(struct kprobe *p) { if (p->addr == - dereference_function_descriptor(kretprobe_trampoline)) + (kprobe_opcode_t *)((struct fnptr *)kretprobe_trampoline)->ip) return 1;
return 0;
This reverts commit d3380de483d55d904fb94a241406b34ed2fada7d.
Since this commit is a part of generic kretprobe trampoline handler series, without the other patches in that series, this causes a build error on ia64.
Reported-by: kernel test robot lkp@intel.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- arch/ia64/kernel/kprobes.c | 77 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 2 deletions(-)
diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c index 8207b897b49d..aa41bd5cf9b7 100644 --- a/arch/ia64/kernel/kprobes.c +++ b/arch/ia64/kernel/kprobes.c @@ -409,9 +409,83 @@ static void kretprobe_trampoline(void) { }
+/* + * At this point the target function has been tricked into + * returning into our trampoline. Lookup the associated instance + * and then: + * - call the handler function + * - cleanup by marking the instance as unused + * - long jump back to the original return address + */ int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs) { - regs->cr_iip = __kretprobe_trampoline_handler(regs, kretprobe_trampoline, NULL); + struct kretprobe_instance *ri = NULL; + struct hlist_head *head, empty_rp; + struct hlist_node *tmp; + unsigned long flags, orig_ret_address = 0; + unsigned long trampoline_address = + ((struct fnptr *)kretprobe_trampoline)->ip; + + INIT_HLIST_HEAD(&empty_rp); + kretprobe_hash_lock(current, &head, &flags); + + /* + * It is possible to have multiple instances associated with a given + * task either because an multiple functions in the call path + * have a return probe installed on them, and/or more than one return + * return probe was registered for a target function. + * + * We can handle this because: + * - instances are always inserted at the head of the list + * - when multiple return probes are registered for the same + * function, the first instance's ret_addr will point to the + * real return address, and all the rest will point to + * kretprobe_trampoline + */ + hlist_for_each_entry_safe(ri, tmp, head, hlist) { + if (ri->task != current) + /* another task is sharing our hash bucket */ + continue; + + orig_ret_address = (unsigned long)ri->ret_addr; + if (orig_ret_address != trampoline_address) + /* + * This is the real return address. Any other + * instances associated with this task are for + * other calls deeper on the call stack + */ + break; + } + + regs->cr_iip = orig_ret_address; + + hlist_for_each_entry_safe(ri, tmp, head, hlist) { + if (ri->task != current) + /* another task is sharing our hash bucket */ + continue; + + if (ri->rp && ri->rp->handler) + ri->rp->handler(ri, regs); + + orig_ret_address = (unsigned long)ri->ret_addr; + recycle_rp_inst(ri, &empty_rp); + + if (orig_ret_address != trampoline_address) + /* + * This is the real return address. Any other + * instances associated with this task are for + * other calls deeper on the call stack + */ + break; + } + kretprobe_assert(ri, orig_ret_address, trampoline_address); + + kretprobe_hash_unlock(current, &flags); + + hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) { + hlist_del(&ri->hlist); + kfree(ri); + } /* * By returning a non-zero value, we are telling * kprobe_handler() that we don't want the post_handler @@ -424,7 +498,6 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *)regs->b0; - ri->fp = NULL;
/* Replace the return addr with trampoline addr */ regs->b0 = ((struct fnptr *)kretprobe_trampoline)->ip;
commit a7fe2378454cf46cd5e2776d05e72bbe8f0a468c upstream.
The following commit:
Commit e792ff804f49 ("ia64: kprobes: Use generic kretprobe trampoline handler")
Passed the wrong trampoline address to __kretprobe_trampoline_handler(): it passes the descriptor address instead of function entry address.
Pass the right parameter.
Also use correct symbol dereference function to get the function address from 'kretprobe_trampoline' - an IA64 special.
Link: https://lkml.kernel.org/r/163163042696.489837.12551102356265354730.stgit@dev...
Fixes: e792ff804f49 ("ia64: kprobes: Use generic kretprobe trampoline handler") Cc: Josh Poimboeuf jpoimboe@redhat.com Cc: Ingo Molnar mingo@kernel.org Cc: X86 ML x86@kernel.org Cc: Daniel Xu dxu@dxuuu.xyz Cc: Thomas Gleixner tglx@linutronix.de Cc: Borislav Petkov bp@alien8.de Cc: Peter Zijlstra peterz@infradead.org Cc: Abhishek Sagar sagar.abhishek@gmail.com Cc: Andrii Nakryiko andrii.nakryiko@gmail.com Cc: Paul McKenney paulmck@kernel.org Cc: stable@vger.kernel.org Signed-off-by: Masami Hiramatsu mhiramat@kernel.org Signed-off-by: Steven Rostedt (VMware) rostedt@goodmis.org --- arch/ia64/kernel/kprobes.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/ia64/kernel/kprobes.c b/arch/ia64/kernel/kprobes.c index aa41bd5cf9b7..7fc0806bbdc9 100644 --- a/arch/ia64/kernel/kprobes.c +++ b/arch/ia64/kernel/kprobes.c @@ -424,7 +424,7 @@ int __kprobes trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs) struct hlist_node *tmp; unsigned long flags, orig_ret_address = 0; unsigned long trampoline_address = - ((struct fnptr *)kretprobe_trampoline)->ip; + (unsigned long)dereference_function_descriptor(kretprobe_trampoline);
INIT_HLIST_HEAD(&empty_rp); kretprobe_hash_lock(current, &head, &flags); @@ -500,7 +500,7 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, ri->ret_addr = (kprobe_opcode_t *)regs->b0;
/* Replace the return addr with trampoline addr */ - regs->b0 = ((struct fnptr *)kretprobe_trampoline)->ip; + regs->b0 = (unsigned long)dereference_function_descriptor(kretprobe_trampoline); }
/* Check the instruction in the slot is break */ @@ -1030,14 +1030,14 @@ static struct kprobe trampoline_p = { int __init arch_init_kprobes(void) { trampoline_p.addr = - (kprobe_opcode_t *)((struct fnptr *)kretprobe_trampoline)->ip; + dereference_function_descriptor(kretprobe_trampoline); return register_kprobe(&trampoline_p); }
int __kprobes arch_trampoline_kprobe(struct kprobe *p) { if (p->addr == - (kprobe_opcode_t *)((struct fnptr *)kretprobe_trampoline)->ip) + dereference_function_descriptor(kretprobe_trampoline)) return 1;
return 0;
On Tue, Apr 26, 2022 at 11:25:54PM +0900, Masami Hiramatsu wrote:
Hi,
Kernel test bot reported that the ia64 build error on stable 4.19.y because of the commit d3380de483d5 ("ia64: kprobes: Use generic kretprobe trampoline handler"). I also found that this commit was involved by the backporting of commit f5f96e3643dc ("ia64: kprobes: Fix to pass correct trampoline address to the handler"), and this 2nd commit was backported wrong way. Actually, this 2nd commit aimed to use dereference_function_descriptor() in kprobes@ia64, but the comment (and Fixes tag) points the 1st commit. Thus I guess this mistake happened.
So I re-backport the upstream commit a7fe2378454c ("ia64: kprobes: Fix to pass correct trampoline address to the handler") correctly, without involving the 1st commit.
Thank you,
All now queued up, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Masami Hiramatsu