kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless
Subsystems (20)
Masami Hiramatsu
maintainerv4 cover kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless
Hi,
Here is the 4th version of the series to unify the kretprobe trampoline handler and make kretprobe lockless.
Previous version is here;
https://lkml.kernel.org/r/159854631442.736475.5062989489155389472.stgit@devnote2
In this version, I updated the generic trampoline handler a bit, merge the Peter's lockless patches(*), and add an RFC "remove task scan" patch as [20/23].
(*) https://lkml.kernel.org/r/[email protected]
I ran some tests and ftracetest on x86-64. Mostly OK, but hit a BUG in the trampoline handler once. I'm trying to reproduce it but not succeeded yet. So this may need a careful review and tests.
I did something like:
mount -t debugfs debug /sys/kernel/debug
cd /sys/kernel/debug/tracing/
echo r:event1 vfs_read >> kprobe_events
echo r:event2 vfs_read %ax >> kprobe_events
echo r:event3 rw_verify_area %ax >> kprobe_events
echo 1 > events/kprobes/enable
sleep 1
less trace
cat ../kprobes/list
cd ~/linux/tools/testing/selftests/ftrace
./ftracetest
Then hits a BUG_ON at kernel/kprobes.c:1893 (no test executed, maybe it happened when removing kretprobes?)
Thank you,
---
Masami Hiramatsu (17):
kprobes: Add generic kretprobe trampoline handler x86/kprobes: Use generic kretprobe trampoline handler
arm: kprobes: Use generic kretprobe trampoline handler
arm64: kprobes: Use generic kretprobe trampoline handler
arc: kprobes: Use generic kretprobe trampoline handler
csky: kprobes: Use generic kretprobe trampoline handler
ia64: kprobes: Use generic kretprobe trampoline handler
mips: kprobes: Use generic kretprobe trampoline handler
parisc: kprobes: Use generic kretprobe trampoline handler
powerpc: kprobes: Use generic kretprobe trampoline handler
s390: kprobes: Use generic kretprobe trampoline handler
sh: kprobes: Use generic kretprobe trampoline handler
sparc: kprobes: Use generic kretprobe trampoline handler
kprobes: Remove NMI context check
kprobes: Free kretprobe_instance with rcu callback
kprobes: Make local used functions static
[RFC] kprobes: Remove task scan for updating kretprobe_instance
Peter Zijlstra (6):
llist: Add nonatomic __llist_add()
sched: Fix try_invoke_on_locked_down_task() semantics
kprobes: Remove kretprobe hash asm-generic/atomic: Add try_cmpxchg() fallbacks
freelist: Lock less freelist
kprobes: Replace rp->free_instance with freelist
arch/arc/kernel/kprobes.c | 54 ------ arch/arm/probes/kprobes/core.c | 78 --------- arch/arm64/kernel/probes/kprobes.c | 78 --------- arch/csky/kernel/probes/kprobes.c | 77 -------- arch/ia64/kernel/kprobes.c | 77 -------- arch/mips/kernel/kprobes.c | 54 ------ arch/parisc/kernel/kprobes.c | 76 -------- arch/powerpc/kernel/kprobes.c | 53 ------ arch/s390/kernel/kprobes.c | 79 --------- arch/sh/kernel/kprobes.c | 58 ------ arch/sparc/kernel/kprobes.c | 51 ------ arch/x86/include/asm/atomic.h | 2 arch/x86/include/asm/atomic64_64.h | 2 arch/x86/include/asm/cmpxchg.h | 2 arch/x86/kernel/kprobes/core.c | 108 ------------ drivers/gpu/drm/i915/i915_request.c | 6 - include/asm-generic/atomic-instrumented.h | 216 ++++++++++++++---------- include/linux/atomic-arch-fallback.h | 90 +++++++++- include/linux/atomic-fallback.h | 90 +++++++++- include/linux/freelist.h | 129 ++++++++++++++ include/linux/kprobes.h | 73 +++++--- include/linux/llist.h | 15 ++ include/linux/sched.h | 4 kernel/fork.c | 4 kernel/kprobes.c | 263 +++++++++++++---------------- kernel/sched/core.c | 9 - kernel/trace/trace_kprobe.c | 3 scripts/atomic/gen-atomic-fallback.sh | 63 ++++++- scripts/atomic/gen-atomic-instrumented.sh | 29 +++ 29 files changed, 729 insertions(+), 1114 deletions(-) create mode 100644 include/linux/freelist.h
--
Masami Hiramatsu (Linaro) <[email protected]>
Masami Hiramatsu
maintainerPATCH v4 1/23 kprobes: Add generic kretprobe trampoline handler
Add a generic kretprobe trampoline handler for unifying the all cloned /arch/* kretprobe trampoline handlers.
The generic kretprobe trampoline handler is based on the x86 implementation, because it is the latest implementation. It has frame pointer checking, kprobe_busy_begin/end and return address fixup for user handlers.
---
Changes in v4:
- Remove orig_ret_address
- Change the type of trampoline_address to void *
--- include/linux/kprobes.h | 32 ++++++++++++++-- kernel/kprobes.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 4 deletions(-)
include/linux/kprobes.h +28 -4
@@ -187,10 +187,38 @@ static inline int kprobes_built_in(void)return 1 ; } +extern struct kprobe kprobe_busy; +void kprobe_busy_begin (void ); +void kprobe_busy_end (void ); +#ifdef CONFIG_KRETPROBES extern void arch_prepare_kretprobe (struct kretprobe_instance *ri,struct pt_regs *regs);extern int arch_trampoline_kprobe (struct kprobe *p); + +/* If the trampoline handler called from a kprobe, use this version */ +unsigned long __kretprobe_trampoline_handler (struct pt_regs *regs, +void *trampoline_address, +void *frame_pointer); + +static nokprobe_inline +unsigned long kretprobe_trampoline_handler (struct pt_regs *regs, +void *trampoline_address, +void *frame_pointer) +{ +unsigned long ret; +/* +* Set a dummy kprobe for avoiding kretprobe recursion. +* Since kretprobe never runs in kprobe handler, any kprobe must not +* be running at this point. +*/ +kprobe_busy_begin (); + ret =__kretprobe_trampoline_handler (regs, trampoline_address, frame_pointer); +kprobe_busy_end (); + +return ret; +} +#else /* CONFIG_KRETPROBES */ static inline void arch_prepare_kretprobe (struct kretprobe *rp,struct pt_regs *regs)
@@ -354,10 +382,6 @@ static inline struct kprobe_ctlblk *get_kprobe_ctlblk(void)return this_cpu_ptr (&kprobe_ctlblk); } -extern struct kprobe kprobe_busy; -void kprobe_busy_begin (void ); -void kprobe_busy_end (void ); -kprobe_opcode_t *kprobe_lookup_name (const char *name,unsigned int offset);int register_kprobe (struct kprobe *p);void unregister_kprobe (struct kprobe *p);
kernel/kprobes.c +96 -0
102 lines not on this page · raw message
Masami Hiramatsu
maintainerPATCH v4 2/23 x86/kprobes: Use generic kretprobe trampoline handler
In reply to Masami Hiramatsu--- arch/x86/kernel/kprobes/core.c | 108 +--------------------------------------- 1 file changed, 3 insertions(+), 105 deletions(-)
arch/x86/kernel/kprobes/core.c +3 -105
@@ -767,124 +767,22 @@ asm(NOKPROBE_SYMBOL (kretprobe_trampoline);STACK_FRAME_NON_STANDARD (kretprobe_trampoline); +/* * Called from kretprobe_trampoline */ __used __visible void *trampoline_handler(struct pt_regs *regs) { -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 = (unsigned long )&kretprobe_trampoline; -kprobe_opcode_t *correct_ret_addr =NULL ; -void *frame_pointer; -bool skipped = false; - -/* -* Set a dummy kprobe for avoiding kretprobe recursion. -* Since kretprobe never run in kprobe handler, kprobe must not -* be running at this point. -*/ -kprobe_busy_begin (); - -INIT_HLIST_HEAD (&empty_rp); -kretprobe_hash_lock (current, &head, &flags);/* fixup registers */ regs->cs = __KERNEL_CS;#ifdef CONFIG_X86_32 regs->cs |=get_kernel_rpl (); regs->gs =0 ;#endif -/* We use pt_regs->sp for return address holder. */ - frame_pointer = ®s->sp; - regs->ip = trampoline_address; + regs->ip = (unsigned long )&kretprobe_trampoline; regs->orig_ax = ~0UL ; -/* -* It is possible to have multiple instances associated with a given -* task either because multiple functions in the call path have -* return probes installed on them, and/or more than one -* return probe was registered for a target function. -* -* We can handle this because: -* - instances are always pushed into the head of the list -* - when multiple return probes are registered for the same -* function, the (chronologically) first instance's ret_addr -* will be the real return address, and all the rest will -* point to kretprobe_trampoline. -*/ -hlist_for_each_entry (ri, head, hlist) { -if (ri->task != current) -/* another task is sharing our hash bucket */ -continue ; -/* -* Return probes must be pushed on this hash list correct -* order (same as return order) so that it can be popped -* correctly. However, if we find it is pushed it incorrect -* order, this means we find a function which should not be -* probed, because the wrong order entry is pushed on the -* path of processing other kretprobe itself. -*/ -if (ri->fp != frame_pointer) { -if (!skipped) -pr_warn ("kretprobe is stacked incorrectly. Trying to fixup.\n" ); - skipped = true; -continue ; - } - - orig_ret_address = (unsigned long )ri->ret_addr; -if (skipped) -pr_warn ("%ps must be blacklisted because of incorrect kretprobe order\n" , - ri->rp->kp.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 ; - } - -kretprobe_assert (ri, orig_ret_address, trampoline_address); - - correct_ret_addr = ri->ret_addr; -hlist_for_each_entry_safe (ri, tmp, head, hlist) { -if (ri->task != current) -/* another task is sharing our hash bucket */ -continue ; -if (ri->fp != frame_pointer) -continue ; - - orig_ret_address = (unsigned long )ri->ret_addr; -if (ri->rp && ri->rp->handler) { -__this_cpu_write (current_kprobe, &ri->rp->kp); - ri->ret_addr = correct_ret_addr; - ri->rp->handler (ri, regs); -__this_cpu_write (current_kprobe, &kprobe_busy); - } - -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_hash_unlock (current, &flags); - -kprobe_busy_end (); - -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - } -return (void *)orig_ret_address; +return (void *)kretprobe_trampoline_handler (regs, &kretprobe_trampoline, ®s->sp); }NOKPROBE_SYMBOL (trampoline_handler);
Masami Hiramatsu
maintainerPATCH v4 3/23 arm: kprobes: Use generic kretprobe trampoline handler
In reply to Masami HiramatsuUse the generic kretprobe trampoline handler. Use regs->ARM_fp for framepointer verification.
---
Changes in v2:
- Fix to cast frame_pointer type to void *.
--- arch/arm/probes/kprobes/core.c | 78 ++-------------------------------------- 1 file changed, 3 insertions(+), 75 deletions(-)
arch/arm/probes/kprobes/core.c +3 -75
@@ -413,87 +413,15 @@ void __naked __kprobes kretprobe_trampoline(void)/* Called from kretprobe_trampoline */ static __used __kprobes void *trampoline_handler (struct pt_regs *regs) { -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 = (unsigned long )&kretprobe_trampoline; -kprobe_opcode_t *correct_ret_addr =NULL ; - -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 multiple functions in the call path have -* a return probe installed on them, and/or more than one 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 ; - } - -kretprobe_assert (ri, orig_ret_address, trampoline_address); - - correct_ret_addr = ri->ret_addr; -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 (ri->rp && ri->rp->handler) { -__this_cpu_write (current_kprobe, &ri->rp->kp); -get_kprobe_ctlblk ()->kprobe_status =KPROBE_HIT_ACTIVE ; - ri->ret_addr = correct_ret_addr; - ri->rp->handler (ri, regs); -__this_cpu_write (current_kprobe,NULL ); - } - -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_hash_unlock (current, &flags); - -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - } - -return (void *)orig_ret_address; +return (void *)kretprobe_trampoline_handler (regs, &kretprobe_trampoline, + (void *)regs->ARM_fp); }void __kprobes arch_prepare_kretprobe (struct kretprobe_instance *ri,struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *)regs->ARM_lr; + ri->fp = (void *)regs->ARM_fp;/* Replace the return addr with trampoline addr. */ regs->ARM_lr = (unsigned long )&kretprobe_trampoline;
Masami Hiramatsu
maintainerPATCH v4 4/23 arm64: kprobes: Use generic kretprobe trampoline handler
In reply to Masami HiramatsuUse the generic kretprobe trampoline handler, and use the kernel_stack_pointer(regs) for framepointer verification.
--- arch/arm64/kernel/probes/kprobes.c | 78 +----------------------------------- 1 file changed, 3 insertions(+), 75 deletions(-)
arch/arm64/kernel/probes/kprobes.c +3 -75
@@ -464,87 +464,15 @@ int __init arch_populate_kprobe_blacklist(void)void __kprobes __used *trampoline_probe_handler (struct pt_regs *regs) { -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 = - (unsigned long )&kretprobe_trampoline; -kprobe_opcode_t *correct_ret_addr =NULL ; - -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 multiple functions in the call path have -* return probes installed on them, and/or more than one -* return probe was registered for a target function. -* -* We can handle this because: -* - instances are always pushed into the head of the list -* - when multiple return probes are registered for the same -* function, the (chronologically) first instance's ret_addr -* will be 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 ; - } - -kretprobe_assert (ri, orig_ret_address, trampoline_address); - - correct_ret_addr = ri->ret_addr; -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 (ri->rp && ri->rp->handler) { -__this_cpu_write (current_kprobe, &ri->rp->kp); -get_kprobe_ctlblk ()->kprobe_status =KPROBE_HIT_ACTIVE ; - ri->ret_addr = correct_ret_addr; - ri->rp->handler (ri, regs); -__this_cpu_write (current_kprobe,NULL ); - } - -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_hash_unlock (current, &flags); - -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - } -return (void *)orig_ret_address; +return (void *)kretprobe_trampoline_handler (regs, &kretprobe_trampoline, + (void *)kernel_stack_pointer (regs)); }void __kprobes arch_prepare_kretprobe (struct kretprobe_instance *ri,struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *)regs->regs[30 ]; + ri->fp = (void *)kernel_stack_pointer (regs);/* replace return addr (x30) with trampoline */ regs->regs[30 ] = (long )&kretprobe_trampoline;
Mark Rutland
reviewerRe: [PATCH v4 04/23] arm64: kprobes: Use generic kretprobe trampoline handler
Hi,
Quoted 8 lines · Masami Hiramatsu
Use the generic kretprobe trampoline handler, and use the
kernel_stack_pointer(regs)for framepointer verification.Signed-off-by Masami Hiramatsu <[email protected]>--- arch/arm64/kernel/probes/kprobes.c | 78 +----------------------------------- 1 file changed, 3 insertions(+), 75 deletions(-)
arch/arm64/kernel/probes/kprobes.c int __init arch_populate_kprobe_blacklist(void) · line 475
Quoted 9 lines
arch/arm64/kernel/probes/kprobes.c +3
-0+return (void *)kretprobe_trampoline_handler (regs, &kretprobe_trampoline, + (void *)kernel_stack_pointer (regs)); }void __kprobes arch_prepare_kretprobe (struct kretprobe_instance *ri,struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *)regs->regs[30 ]; + ri->fp = (void *)kernel_stack_pointer (regs);
This is probably a nomenclature problem, but what exactly is kretprobe_instance::fp used for?
I ask because arm64's frame pointer lives in x29 (and is not necessarily the same as the stack pointer at function boundaries), so the naming is potentially misleading and possibly worth a comment or rename.
Thanks,
Mark.
[email protected]
maintainerRe: [PATCH v4 04/23] arm64: kprobes: Use generic kretprobe trampoline handler
Quoted 24 lines · Mark Rutland
Hi,
Quoted 8 lines · Masami Hiramatsu
Use the generic kretprobe trampoline handler, and use the
kernel_stack_pointer(regs)for framepointer verification.Signed-off-by Masami Hiramatsu <[email protected]>--- arch/arm64/kernel/probes/kprobes.c | 78 +----------------------------------- 1 file changed, 3 insertions(+), 75 deletions(-)arch/arm64/kernel/probes/kprobes.c int __init arch_populate_kprobe_blacklist(void) · line 475
Quoted 9 lines
arch/arm64/kernel/probes/kprobes.c +3
-0+return (void *)kretprobe_trampoline_handler (regs, &kretprobe_trampoline, + (void *)kernel_stack_pointer (regs)); }void __kprobes arch_prepare_kretprobe (struct kretprobe_instance *ri,struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *)regs->regs[30 ]; + ri->fp = (void *)kernel_stack_pointer (regs);This is probably a nomenclature problem, but what exactly is
kretprobe_instance::fpused for?
Quoted 3 lines
I ask because arm64's frame pointer lives in x29 (and is not necessarily the same as the stack pointer at function boundaries), so the naming is potentially misleading and possibly worth a comment or rename.
IIUC ri->rp is used for matching up the frame between the moment we install the return trampoline on the stack and actually hitting that trampoline.
Mark Rutland
reviewerRe: [PATCH v4 04/23] arm64: kprobes: Use generic kretprobe trampoline handler
Quoted 25 lines · On Fri, Aug 28, 2020 at 03:37:18PM +0200, [email protected] wrote:
Quoted 24 lines · Mark Rutland
Hi,
Quoted 8 lines · Masami Hiramatsu
Use the generic kretprobe trampoline handler, and use the
kernel_stack_pointer(regs)for framepointer verification.Signed-off-by Masami Hiramatsu <[email protected]>--- arch/arm64/kernel/probes/kprobes.c | 78 +----------------------------------- 1 file changed, 3 insertions(+), 75 deletions(-)arch/arm64/kernel/probes/kprobes.c int __init arch_populate_kprobe_blacklist(void) · line 475
Quoted 9 lines
arch/arm64/kernel/probes/kprobes.c +3
-0+return (void *)kretprobe_trampoline_handler (regs, &kretprobe_trampoline, + (void *)kernel_stack_pointer (regs)); }void __kprobes arch_prepare_kretprobe (struct kretprobe_instance *ri,struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *)regs->regs[30 ]; + ri->fp = (void *)kernel_stack_pointer (regs);This is probably a nomenclature problem, but what exactly is
kretprobe_instance::fpused for?
Quoted 7 lines
Quoted 3 lines
I ask because arm64's frame pointer lives in x29 (and is not necessarily the same as the stack pointer at function boundaries), so the naming is potentially misleading and possibly worth a comment or rename.
IIUC
ri->rpis used for matching up the frame between the moment we install the return trampoline on the stack and actually hitting that trampoline.
Hmm, I see (and this is not a new problem with this series, just existing weirdness).
It looks like that's inconsistently an arch's stack pointer (e.g. x86, arm64) or frame pointer (e.g. arm). IMO it'd be nicer if we could make that the stack pointer consistently, but that's a separate cleanup, and apparently not a big deal.
Thanks,
Mark.
Masami Hiramatsu
maintainerRe: [PATCH v4 04/23] arm64: kprobes: Use generic kretprobe trampoline handler
In reply to [email protected]Quoted 25 lines · On Fri, 28 Aug 2020 15:37:18 +0200 [email protected] wrote:
Quoted 24 lines · Mark Rutland
Hi,
Quoted 8 lines · Masami Hiramatsu
Use the generic kretprobe trampoline handler, and use the
kernel_stack_pointer(regs)for framepointer verification.Signed-off-by Masami Hiramatsu <[email protected]>--- arch/arm64/kernel/probes/kprobes.c | 78 +----------------------------------- 1 file changed, 3 insertions(+), 75 deletions(-)arch/arm64/kernel/probes/kprobes.c int __init arch_populate_kprobe_blacklist(void) · line 475
Quoted 9 lines
arch/arm64/kernel/probes/kprobes.c +3
-0+return (void *)kretprobe_trampoline_handler (regs, &kretprobe_trampoline, + (void *)kernel_stack_pointer (regs)); }void __kprobes arch_prepare_kretprobe (struct kretprobe_instance *ri,struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *)regs->regs[30 ]; + ri->fp = (void *)kernel_stack_pointer (regs);This is probably a nomenclature problem, but what exactly is
kretprobe_instance::fpused for?
Quoted 7 lines
Quoted 3 lines
I ask because arm64's frame pointer lives in x29 (and is not necessarily the same as the stack pointer at function boundaries), so the naming is potentially misleading and possibly worth a comment or rename.
IIUC
ri->rpis used for matching up the frame between the moment we install the return trampoline on the stack and actually hitting that trampoline.
Yeah, it is confusing, sorry. On x86, CONFIG_FRAME_POINTER can be disabled, so we used the address of stack entry where arch_prepare_kretprobe() stores the trampoline address.
For arm64 which doesn't use a stack entry for call, I decided to use the stack address when the target function is called.
Indeed, it should be commented.
Thank you,
Signature · 1 line
Masami Hiramatsu <[email protected]>
Masami Hiramatsu
maintainerPATCH v4 5/23 arc: kprobes: Use generic kretprobe trampoline handler
In reply to Masami Hiramatsu--- arch/arc/kernel/kprobes.c | 54 ++------------------------------------------- 1 file changed, 2 insertions(+), 52 deletions(-)
arch/arc/kernel/kprobes.c +2 -52
@@ -388,6 +388,7 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, { ri->ret_addr = (kprobe_opcode_t *) regs->blink; + ri->fp =NULL ;/* Replace the return addr with trampoline addr */ regs->blink = (unsigned long )&kretprobe_trampoline;
@@ -396,58 +397,7 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,static int __kprobes trampoline_probe_handler (struct kprobe *p,struct pt_regs *regs) { -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 = (unsigned long )&kretprobe_trampoline; - -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 ; - -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); - regs->ret = orig_ret_address; - -kretprobe_hash_unlock (current, &flags); - -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - } + regs->ret =__kretprobe_trampoline_handler (regs, &kretprobe_trampoline,NULL );/* By returning a non zero value, we are telling the kprobe handler * that we don't want the post_handler to run
Masami Hiramatsu
maintainerPATCH v4 6/23 csky: kprobes: Use generic kretprobe trampoline handler
In reply to Masami Hiramatsu--- arch/csky/kernel/probes/kprobes.c | 77 +------------------------------------ 1 file changed, 2 insertions(+), 75 deletions(-)
arch/csky/kernel/probes/kprobes.c +2 -75
@@ -404,87 +404,14 @@ int __init arch_populate_kprobe_blacklist(void)void __kprobes __used *trampoline_probe_handler (struct pt_regs *regs) { -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 = - (unsigned long )&kretprobe_trampoline; -kprobe_opcode_t *correct_ret_addr =NULL ; - -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 multiple functions in the call path have -* return probes installed on them, and/or more than one -* return probe was registered for a target function. -* -* We can handle this because: -* - instances are always pushed into the head of the list -* - when multiple return probes are registered for the same -* function, the (chronologically) first instance's ret_addr -* will be 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 ; - } - -kretprobe_assert (ri, orig_ret_address, trampoline_address); - - correct_ret_addr = ri->ret_addr; -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 (ri->rp && ri->rp->handler) { -__this_cpu_write (current_kprobe, &ri->rp->kp); -get_kprobe_ctlblk ()->kprobe_status =KPROBE_HIT_ACTIVE ; - ri->ret_addr = correct_ret_addr; - ri->rp->handler (ri, regs); -__this_cpu_write (current_kprobe,NULL ); - } - -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_hash_unlock (current, &flags); - -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - } -return (void *)orig_ret_address; +return (void *)kretprobe_trampoline_handler (regs, &kretprobe_trampoline,NULL ); }void __kprobes arch_prepare_kretprobe (struct kretprobe_instance *ri,struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *)regs->lr; + ri->fp =NULL ; regs->lr = (unsigned long ) &kretprobe_trampoline; }
Masami Hiramatsu
maintainerPATCH v4 7/23 ia64: kprobes: Use generic kretprobe trampoline handler
In reply to Masami Hiramatsu--- arch/ia64/kernel/kprobes.c | 77 +------------------------------------------- 1 file changed, 2 insertions(+), 75 deletions(-)
arch/ia64/kernel/kprobes.c +2 -75
@@ -396,83 +396,9 @@ 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) { -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); - } + 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
@@ -485,6 +411,7 @@ 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;
Masami Hiramatsu
maintainerPATCH v4 8/23 mips: kprobes: Use generic kretprobe trampoline handler
In reply to Masami Hiramatsu--- arch/mips/kernel/kprobes.c | 54 ++------------------------------------------ 1 file changed, 3 insertions(+), 51 deletions(-)
arch/mips/kernel/kprobes.c +3 -51
@@ -477,6 +477,7 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *) regs->regs[31 ]; + ri->fp =NULL ;/* Replace the return addr with trampoline addr */ regs->regs[31 ] = (unsigned long )kretprobe_trampoline;
@@ -488,57 +489,8 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,static int __kprobes trampoline_probe_handler (struct kprobe *p,struct pt_regs *regs) { -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 = (unsigned long )kretprobe_trampoline; - -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 ; - -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); -instruction_pointer (regs) = orig_ret_address; - -kretprobe_hash_unlock (current, &flags); - -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - } +instruction_pointer (regs) =__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
Masami Hiramatsu
maintainerPATCH v4 9/23 parisc: kprobes: Use generic kretprobe trampoline handler
In reply to Masami Hiramatsu--- arch/parisc/kernel/kprobes.c | 76 ++---------------------------------------- 1 file changed, 4 insertions(+), 72 deletions(-)
arch/parisc/kernel/kprobes.c +4 -72
@@ -191,80 +191,11 @@ static struct kprobe trampoline_p = {static int __kprobes trampoline_probe_handler (struct kprobe *p,struct pt_regs *regs) { -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 = (unsigned long )trampoline_p.addr; -kprobe_opcode_t *correct_ret_addr =NULL ; - -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 multiple functions in the call path have -* a return probe installed on them, and/or more than one 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 ; - } - -kretprobe_assert (ri, orig_ret_address, trampoline_address); - - correct_ret_addr = ri->ret_addr; -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 (ri->rp && ri->rp->handler) { -__this_cpu_write (current_kprobe, &ri->rp->kp); -get_kprobe_ctlblk ()->kprobe_status =KPROBE_HIT_ACTIVE ; - ri->ret_addr = correct_ret_addr; - ri->rp->handler (ri, regs); -__this_cpu_write (current_kprobe,NULL ); - } - -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 ; - } +unsigned long orig_ret_address; -kretprobe_hash_unlock (current, &flags); - -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - } + orig_ret_address =__kretprobe_trampoline_handler (regs, trampoline_p.addr,NULL );instruction_pointer_set (regs, orig_ret_address); +return 1 ; }
@@ -272,6 +203,7 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *)regs->gr[2 ]; + ri->fp =NULL ;/* Replace the return addr with trampoline addr. */ regs->gr[2 ] = (unsigned long )trampoline_p.addr;
Masami Hiramatsu
maintainerPATCH v4 10/23 powerpc: kprobes: Use generic kretprobe trampoline handler
In reply to Masami Hiramatsu---
Changes in v2:
Fix to use correct trampoline_address.
--- arch/powerpc/kernel/kprobes.c | 53 ++--------------------------------------- 1 file changed, 3 insertions(+), 50 deletions(-)
arch/powerpc/kernel/kprobes.c +3 -50
@@ -218,6 +218,7 @@ bool arch_kprobe_on_func_entry(unsigned long offset)void arch_prepare_kretprobe (struct kretprobe_instance *ri,struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *)regs->link; + ri->fp =NULL ;/* Replace the return addr with trampoline addr */ regs->link = (unsigned long )kretprobe_trampoline;
@@ -396,50 +397,9 @@ asm(".global kretprobe_trampoline\n"*/ static int trampoline_probe_handler (struct kprobe *p,struct pt_regs *regs) { -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 =(unsigned long )&kretprobe_trampoline; - -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 ; - -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); +unsigned long orig_ret_address; + orig_ret_address =__kretprobe_trampoline_handler (regs, &kretprobe_trampoline,NULL );/* * We get here through one of two paths: * 1. by taking a trap -> kprobe_handler() -> here
@@ -458,13 +418,6 @@ static int trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs) regs->nip = orig_ret_address -4 ; regs->link = orig_ret_address; -kretprobe_hash_unlock (current, &flags); - -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - } -return 0 ; }NOKPROBE_SYMBOL (trampoline_probe_handler);
Masami Hiramatsu
maintainerPATCH v4 11/23 s390: kprobes: Use generic kretprobe trampoline handler
In reply to Masami Hiramatsu--- arch/s390/kernel/kprobes.c | 79 +------------------------------------------- 1 file changed, 2 insertions(+), 77 deletions(-)
arch/s390/kernel/kprobes.c +2 -77
@@ -228,6 +228,7 @@ NOKPROBE_SYMBOL(pop_kprobe);void arch_prepare_kretprobe (struct kretprobe_instance *ri,struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *) regs->gprs[14 ]; + ri->fp =NULL ;/* Replace the return addr with trampoline addr */ regs->gprs[14 ] = (unsigned long ) &kretprobe_trampoline;
@@ -331,83 +332,7 @@ static void __used kretprobe_trampoline_holder(void)*/ static int trampoline_probe_handler (struct kprobe *p,struct pt_regs *regs) { -struct kretprobe_instance *ri; -struct hlist_head *head, empty_rp; -struct hlist_node *tmp; -unsigned long flags, orig_ret_address; -unsigned long trampoline_address; -kprobe_opcode_t *correct_ret_addr; - -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 -*/ - ri =NULL ; - orig_ret_address =0 ; - correct_ret_addr =NULL ; - trampoline_address = (unsigned long ) &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 ; - } - -kretprobe_assert (ri, orig_ret_address, trampoline_address); - - correct_ret_addr = ri->ret_addr; -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 (ri->rp && ri->rp->handler) { - ri->ret_addr = correct_ret_addr; - ri->rp->handler (ri, regs); - } - -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 ; - } - - regs->psw.addr = orig_ret_address; - -kretprobe_hash_unlock (current, &flags); - -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - } + regs->psw.addr =__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
Masami Hiramatsu
maintainerPATCH v4 12/23 sh: kprobes: Use generic kretprobe trampoline handler
In reply to Masami Hiramatsu--- arch/sh/kernel/kprobes.c | 58 ++-------------------------------------------- 1 file changed, 3 insertions(+), 55 deletions(-)
arch/sh/kernel/kprobes.c +3 -55
@@ -204,6 +204,7 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *) regs->pr; + ri->fp =NULL ;/* Replace the return addr with trampoline addr */ regs->pr = (unsigned long )kretprobe_trampoline;
@@ -302,62 +303,9 @@ static void __used kretprobe_trampoline_holder(void)*/ int __kprobes trampoline_probe_handler (struct kprobe *p,struct pt_regs *regs) { -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 = (unsigned long )&kretprobe_trampoline; + regs->pc =__kretprobe_trampoline_handler (regs, &kretprobe_trampoline,NULL ); -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 then 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 ; - -if (ri->rp && ri->rp->handler) { -__this_cpu_write (current_kprobe, &ri->rp->kp); - ri->rp->handler (ri, regs); -__this_cpu_write (current_kprobe,NULL ); - } - - 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); - - regs->pc = orig_ret_address; -kretprobe_hash_unlock (current, &flags); - -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - } - -return orig_ret_address; +return 1 ; }static int __kprobes post_kprobe_handler (struct pt_regs *regs)
Masami Hiramatsu
maintainerPATCH v4 13/23 sparc: kprobes: Use generic kretprobe trampoline handler
In reply to Masami Hiramatsu--- arch/sparc/kernel/kprobes.c | 51 +++---------------------------------------- 1 file changed, 3 insertions(+), 48 deletions(-)
arch/sparc/kernel/kprobes.c +3 -48
@@ -453,6 +453,7 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs) { ri->ret_addr = (kprobe_opcode_t *)(regs->u_regs[UREG_RETPC ] +8 ); + ri->fp =NULL ;/* Replace the return addr with trampoline addr */ regs->u_regs[UREG_RETPC] =
@@ -465,58 +466,12 @@ void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,static int __kprobes trampoline_probe_handler (struct kprobe *p,struct pt_regs *regs) { -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 =(unsigned long )&kretprobe_trampoline; +unsigned long orig_ret_address =0 ; -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 ; - -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); + orig_ret_address =__kretprobe_trampoline_handler (regs, &kretprobe_trampoline,NULL ); regs->tpc = orig_ret_address; regs->tnpc = orig_ret_address +4 ; -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
Masami Hiramatsu
maintainerPATCH v4 14/23 kprobes: Remove NMI context check
In reply to Masami HiramatsuSince the commit 9b38cc704e84 ("kretprobe: Prevent triggering kretprobe from within kprobe_flush_task") sets a dummy current kprobe in the trampoline handler by kprobe_busy_begin/end(), it is not possible to run a kretprobe pre handler in kretprobe trampoline handler context even with the NMI. If the NMI interrupts a kretprobe_trampoline_handler() and it hits a kretprobe, the 2nd kretprobe will detect recursion correctly and it will be skipped.
This means we have almost no double-lock issue on kretprobes by NMI.
The last one point is in cleanup_rp_inst() which also takes kretprobe_table_lock without setting up current kprobes. So adding kprobe_busy_begin/end() there allows us to remove in_nmi() check.
The above commit applies kprobe_busy_begin/end() on x86, but now all arch implementation are unified to generic one, we can safely remove the in_nmi() check from arch independent code.
--- kernel/kprobes.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
kernel/kprobes.c +4 -12
@@ -1359,7 +1359,8 @@ static void cleanup_rp_inst(struct kretprobe *rp)struct hlist_node *next;struct hlist_head *head; -/* No race here */ +/* To avoid recursive kretprobe by NMI, set kprobe busy here */ +kprobe_busy_begin ();for (hash =0 ; hash <KPROBE_TABLE_SIZE ; hash++) {kretprobe_table_lock (hash, &flags); head = &kretprobe_inst_table[hash];
@@ -1369,6 +1370,8 @@ static void cleanup_rp_inst(struct kretprobe *rp) }kretprobe_table_unlock (hash, &flags); } +kprobe_busy_end (); +free_rp_inst (rp); }NOKPROBE_SYMBOL (cleanup_rp_inst);
@@ -2033,17 +2036,6 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)unsigned long hash, flags =0 ;struct kretprobe_instance *ri; -/* -* To avoid deadlocks, prohibit return probing in NMI contexts, -* just skip the probe and increase the (inexact) 'nmissed' -* statistical counter, so that the user is informed that -* something happened: -*/ -if (unlikely (in_nmi ())) { - rp->nmissed++; -return 0 ; - } -/* TODO: consider to only swap the RA after the last pre_handler fired */ hash =hash_ptr (current,KPROBE_HASH_BITS );raw_spin_lock_irqsave (&rp->lock, flags);
Masami Hiramatsu
maintainerPATCH v4 15/23 kprobes: Free kretprobe_instance with rcu callback
In reply to Masami HiramatsuFree kretprobe_instance with rcu callback instead of directly freeing the object in the kretprobe handler context.
This will make kretprobe run safer in NMI context.
---
Changes in v3:
- Stick the rcu_head with hlist_node in kretprobe_instance
- Make recycle_rp_inst() static
--- include/linux/kprobes.h | 6 ++++-- kernel/kprobes.c | 25 ++++++------------------- 2 files changed, 10 insertions(+), 21 deletions(-)
include/linux/kprobes.h +4 -2
@@ -156,7 +156,10 @@ struct kretprobe { };struct kretprobe_instance { -struct hlist_node hlist; +union { +struct hlist_node hlist; +struct rcu_head rcu; + };struct kretprobe *rp;kprobe_opcode_t *ret_addr;struct task_struct *task;
@@ -395,7 +398,6 @@ int register_kretprobes(struct kretprobe **rps, int num);void unregister_kretprobes (struct kretprobe **rps,int num);void kprobe_flush_task (struct task_struct *tk); -void recycle_rp_inst (struct kretprobe_instance *ri,struct hlist_head *head);int disable_kprobe (struct kprobe *kp);int enable_kprobe (struct kprobe *kp);
kernel/kprobes.c +6 -19
@@ -1223,8 +1223,7 @@ void kprobes_inc_nmissed_count(struct kprobe *p) }NOKPROBE_SYMBOL (kprobes_inc_nmissed_count); -void recycle_rp_inst (struct kretprobe_instance *ri, -struct hlist_head *head) +static void recycle_rp_inst (struct kretprobe_instance *ri) {struct kretprobe *rp = ri->rp;
@@ -1236,8 +1235,7 @@ void recycle_rp_inst(struct kretprobe_instance *ri,hlist_add_head (&ri->hlist, &rp->free_instances);raw_spin_unlock (&rp->lock); }else -/* Unregistering */ -hlist_add_head (&ri->hlist, head); +kfree_rcu (ri, rcu); }NOKPROBE_SYMBOL (recycle_rp_inst);
@@ -1313,7 +1311,7 @@ void kprobe_busy_end(void)void kprobe_flush_task (struct task_struct *tk) {struct kretprobe_instance *ri; -struct hlist_head *head, empty_rp; +struct hlist_head *head;struct hlist_node *tmp;unsigned long hash, flags =0 ;
@@ -1323,19 +1321,14 @@ void kprobe_flush_task(struct task_struct *tk)kprobe_busy_begin (); -INIT_HLIST_HEAD (&empty_rp); hash =hash_ptr (tk,KPROBE_HASH_BITS ); head = &kretprobe_inst_table[hash];kretprobe_table_lock (hash, &flags);hlist_for_each_entry_safe (ri, tmp, head, hlist) {if (ri->task == tk) -recycle_rp_inst (ri, &empty_rp); +recycle_rp_inst (ri); }kretprobe_table_unlock (hash, &flags); -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - }kprobe_busy_end (); }
@@ -1936,13 +1929,12 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs, void *frame_pointer) {struct kretprobe_instance *ri =NULL , *last =NULL ; -struct hlist_head *head, empty_rp; +struct hlist_head *head;struct hlist_node *tmp;unsigned long flags;kprobe_opcode_t *correct_ret_addr =NULL ;bool skipped = false; -INIT_HLIST_HEAD (&empty_rp);kretprobe_hash_lock (current, &head, &flags);/*
@@ -2009,7 +2001,7 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,__this_cpu_write (current_kprobe, &kprobe_busy); } -recycle_rp_inst (ri, &empty_rp); +recycle_rp_inst (ri);if (ri == last)break ;
@@ -2017,11 +2009,6 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,kretprobe_hash_unlock (current, &flags); -hlist_for_each_entry_safe (ri, tmp, &empty_rp, hlist) { -hlist_del (&ri->hlist); -kfree (ri); - } -return (unsigned long )correct_ret_addr; }NOKPROBE_SYMBOL (__kretprobe_trampoline_handler)
Masami Hiramatsu
maintainerPATCH v4 16/23 kprobes: Make local used functions static
In reply to Masami HiramatsuSince we unified the kretprobe trampoline handler from arch/* code, some functions and objects no need to be exported anymore.
---
Changes in v4:
- Convert kretprobe_assert() into a BUG_ON().
--- include/linux/kprobes.h | 15 --------------- kernel/kprobes.c | 9 ++++----- 2 files changed, 4 insertions(+), 20 deletions(-)
include/linux/kprobes.h +0 -15
@@ -190,7 +190,6 @@ static inline int kprobes_built_in(void)return 1 ; } -extern struct kprobe kprobe_busy;void kprobe_busy_begin (void );void kprobe_busy_end (void );
@@ -235,16 +234,6 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)extern struct kretprobe_blackpoint kretprobe_blacklist[]; -static inline void kretprobe_assert (struct kretprobe_instance *ri, -unsigned long orig_ret_address,unsigned long trampoline_address) -{ -if (!orig_ret_address || (orig_ret_address == trampoline_address)) { -printk ("kretprobe BUG!: Processing kretprobe %p @ %p\n" , - ri->rp, ri->rp->kp.addr); -BUG (); - } -} -#ifdef CONFIG_KPROBES_SANITY_TEST extern int init_test_probes (void );#else
@@ -364,10 +353,6 @@ int arch_check_ftrace_location(struct kprobe *p);/* Get the kprobe at this addr (if any) - called with preemption disabled */ struct kprobe *get_kprobe (void *addr); -void kretprobe_hash_lock (struct task_struct *tsk, -struct hlist_head **head,unsigned long *flags); -void kretprobe_hash_unlock (struct task_struct *tsk,unsigned long *flags); -struct hlist_head *kretprobe_inst_table_head (struct task_struct *tsk);/* kprobe_running() will just return the current_kprobe on this CPU */ static inline struct kprobe *kprobe_running (void )
kernel/kprobes.c +4 -5
@@ -1239,7 +1239,7 @@ static void recycle_rp_inst(struct kretprobe_instance *ri) }NOKPROBE_SYMBOL (recycle_rp_inst); -void kretprobe_hash_lock (struct task_struct *tsk, +static void kretprobe_hash_lock (struct task_struct *tsk,struct hlist_head **head,unsigned long *flags)__acquires (hlist_lock) {
@@ -1261,7 +1261,7 @@ __acquires(hlist_lock) }NOKPROBE_SYMBOL (kretprobe_table_lock); -void kretprobe_hash_unlock (struct task_struct *tsk, +static void kretprobe_hash_unlock (struct task_struct *tsk,unsigned long *flags)__releases (hlist_lock) {
@@ -1282,7 +1282,7 @@ __releases(hlist_lock) }NOKPROBE_SYMBOL (kretprobe_table_unlock); -struct kprobe kprobe_busy = { +static struct kprobe kprobe_busy = { .addr = (void *) get_kprobe, };
@@ -1983,8 +1983,7 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,break ; } -kretprobe_assert (ri, (unsigned long )correct_ret_addr, - (unsigned long )trampoline_address); +BUG_ON (!correct_ret_addr || (correct_ret_addr == trampoline_address)); last = ri;hlist_for_each_entry_safe (ri, tmp, head, hlist) {
Masami Hiramatsu
maintainerPATCH v4 17/23 llist: Add nonatomic __llist_add()
In reply to Masami HiramatsuFrom: Peter Zijlstra <[email protected]>
--- drivers/gpu/drm/i915/i915_request.c | 6 ------ include/linux/llist.h | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 6 deletions(-)
drivers/gpu/drm/i915/i915_request.c +0 -6
@@ -357,12 +357,6 @@ void i915_request_retire_upto(struct i915_request *rq) }while (i915_request_retire (tmp) && tmp != rq); } -static void __llist_add (struct llist_node *node,struct llist_head *head) -{ - node->next = head->first; - head->first = node; -} -static struct i915_request *const *__engine_active (struct intel_engine_cs *engine) {
include/linux/llist.h +15 -0
@@ -197,6 +197,16 @@ static inline struct llist_node *llist_next(struct llist_node *node)extern bool llist_add_batch (struct llist_node *new_first,struct llist_node *new_last,struct llist_head *head); + +static inline bool __llist_add_batch (struct llist_node *new_first, +struct llist_node *new_last, +struct llist_head *head) +{ + new_last->next = head->first; + head->first = new_first; +return new_last->next ==NULL ; +} +/** * llist_add - add a new entry * @new: new entry to be added
@@ -209,6 +219,11 @@ static inline bool llist_add(struct llist_node *new, struct llist_head *head)return llist_add_batch (new, new, head); } +static inline bool __llist_add (struct llist_node *new,struct llist_head *head) +{ +return __llist_add_batch (new, new, head); +} +/** * llist_del_all - delete all entries from lock-less list * @head: the head of lock-less list to delete all entries
Masami Hiramatsu
maintainerPATCH v4 18/23 sched: Fix try_invoke_on_locked_down_task() semantics
In reply to Masami HiramatsuFrom: Peter Zijlstra <[email protected]>
--- kernel/sched/core.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
kernel/sched/core.c +4 -5
@@ -3006,15 +3006,14 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)*/ bool try_invoke_on_locked_down_task (struct task_struct *p,bool (*func)(struct task_struct *t,void *arg),void *arg) { -bool ret = false;struct rq_flags rf; +bool ret = false;struct rq *rq; -lockdep_assert_irqs_enabled (); -raw_spin_lock_irq (&p->pi_lock); +raw_spin_lock_irqsave (&p->pi_lock, rf.flags);if (p->on_rq) { rq =__task_rq_lock (p, &rf); -if (task_rq (p) == rq) +if (task_rq (p) == rq && rq->curr != p) ret =func (p, arg);rq_unlock (rq, &rf); }else {
@@ -3028,7 +3027,7 @@ bool try_invoke_on_locked_down_task(struct task_struct *p, bool (*func)(struct t ret =func (p, arg); } } -raw_spin_unlock_irq (&p->pi_lock); +raw_spin_unlock_irqrestore (&p->pi_lock, rf.flags);return ret; }
Masami Hiramatsu
maintainerRe: [PATCH v4 18/23] sched: Fix try_invoke_on_locked_down_task() semantics
Quoted 1 line · On Fri, 28 Aug 2020 21:29:55 +0900 Masami Hiramatsu <[email protected]> wrote:
From: Peter Zijlstra <[email protected]>
In the next version I will drop this since I will merge the kretprobe_holder things into removing kretporbe hash patch.
However, this patch itself seems fixing a bug of commit 2beaf3280e57 ("sched/core: Add function to sample state of locked-down task"). Peter, could you push this separately?
Thank you,
Quoted 39 lines
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/sched/core.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)kernel/sched/core.c +4
-5@@ -3006,15 +3006,14 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)*/ bool try_invoke_on_locked_down_task (struct task_struct *p,bool (*func)(struct task_struct *t,void *arg),void *arg) { -bool ret = false;struct rq_flags rf; +bool ret = false;struct rq *rq; -lockdep_assert_irqs_enabled (); -raw_spin_lock_irq (&p->pi_lock); +raw_spin_lock_irqsave (&p->pi_lock, rf.flags);if (p->on_rq) { rq =__task_rq_lock (p, &rf); -if (task_rq (p) == rq) +if (task_rq (p) == rq && rq->curr != p) ret =func (p, arg);rq_unlock (rq, &rf); }else {@@ -3028,7 +3027,7 @@ bool try_invoke_on_locked_down_task(struct task_struct *p, bool (*func)(struct t ret =func (p, arg); } } -raw_spin_unlock_irq (&p->pi_lock); +raw_spin_unlock_irqrestore (&p->pi_lock, rf.flags);return ret; }
Signature · 1 line
Masami Hiramatsu <[email protected]>
[email protected]
maintainerRe: [PATCH v4 18/23] sched: Fix try_invoke_on_locked_down_task() semantics
Quoted 11 lines · Masami Hiramatsu
Quoted 1 line · On Fri, 28 Aug 2020 21:29:55 +0900 Masami Hiramatsu <[email protected]> wrote:
From: Peter Zijlstra <[email protected]>
In the next version I will drop this since I will merge the
kretprobe_holderthings into removing kretporbe hash patch.However, this patch itself seems fixing a bug of commit
2beaf3280e57("sched/core: Add function to sample state of locked-down task"). Peter, could you push this separately?
Yeah, Paul and me have a slightly different version for that, this also changes semantics we're still bickering over ;-)
But yes, I'll take care of it.
Re: [PATCH v4 18/23] sched: Fix try_invoke_on_locked_down_task() semantics
Quoted 9 lines · On Sat, Aug 29, 2020 at 09:30:49AM +0200, [email protected] wrote:
Quoted 8 lines · Masami Hiramatsu
Quoted 1 line · On Fri, 28 Aug 2020 21:29:55 +0900 Masami Hiramatsu <[email protected]> wrote:
From: Peter Zijlstra <[email protected]>
In the next version I will drop this since I will merge the
kretprobe_holderthings into removing kretporbe hash patch.
Quoted 8 lines
Quoted 3 lines
However, this patch itself seems fixing a bug of commit
2beaf3280e57("sched/core: Add function to sample state of locked-down task"). Peter, could you push this separately?Yeah, Paul and me have a slightly different version for that, this also changes semantics we're still bickering over ;-)
But yes, I'll take care of it.
For whatever it is worth, I ended up back at your original patch with one change to the header comment, as shown below. Does this work for you?
Thanx, Paul
------------------------------------------------------------------------
commit 3f73a1137f8e999a606357064ebd914cf5f2c897
Author: Peter Zijlstra <[email protected]> Date: Sat Aug 29 10:22:24 2020 -0700
sched/core: Allow try_invoke_on_locked_down_task() with irqs disabled
The try_invoke_on_locked_down_task() function currently requires that interrupts be enabled, but it is called with interrupts disabled from rcu_print_task_stall(), resulting in an "IRQs not enabled as expected" diagnostic. This commit therefore updates try_invoke_on_locked_down_task() to use raw_spin_lock_irqsave() instead of raw_spin_lock_irq(), thus allowing use from either context.
kernel/sched/core.c +4 -5
@@ -2988,7 +2988,7 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)/** * try_invoke_on_locked_down_task - Invoke a function on task in fixed state -* @p: Process for which the function is to be invoked. +* @p: Process for which the function is to be invoked, can be @current. * @func: Function to invoke. * @arg: Argument to function. *
@@ -3006,12 +3006,11 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)*/ bool try_invoke_on_locked_down_task (struct task_struct *p,bool (*func)(struct task_struct *t,void *arg),void *arg) { -bool ret = false;struct rq_flags rf; +bool ret = false;struct rq *rq; -lockdep_assert_irqs_enabled (); -raw_spin_lock_irq (&p->pi_lock); +raw_spin_lock_irqsave (&p->pi_lock, rf.flags);if (p->on_rq) { rq =__task_rq_lock (p, &rf);if (task_rq (p) == rq)
@@ -3028,7 +3027,7 @@ bool try_invoke_on_locked_down_task(struct task_struct *p, bool (*func)(struct t ret =func (p, arg); } } -raw_spin_unlock_irq (&p->pi_lock); +raw_spin_unlock_irqrestore (&p->pi_lock, rf.flags);return ret; }
Masami Hiramatsu
maintainerPATCH v4 19/23 kprobes: Remove kretprobe hash
In reply to Masami HiramatsuFrom: Peter Zijlstra <[email protected]>
The kretprobe hash is mostly superfluous, replace it with a per-task variable.
This gets rid of the task hash and it's related locking.
The whole invalidate_rp_inst() is tedious and could go away once we drop rp specific ri size.
---
Changes:
- [MH] ported on Masami's latest version
- [MH] remove unneeded last node checking and unused variables
- [MH] Fix to remove unneeded hlist_del from recycle_rp_inst()
--- include/linux/kprobes.h | 1 include/linux/sched.h | 4 + kernel/fork.c | 4 + kernel/kprobes.c | 232 ++++++++++++++++++----------------------------- 4 files changed, 100 insertions(+), 141 deletions(-)
include/linux/kprobes.h +1 -0
@@ -157,6 +157,7 @@ struct kretprobe {struct kretprobe_instance {union { +struct llist_node llist;struct hlist_node hlist;struct rcu_head rcu; };
include/linux/sched.h +4 -0
@@ -1315,6 +1315,10 @@ struct task_struct {struct callback_head mce_kill_me;#endif +#ifdef CONFIG_KRETPROBES +struct llist_head kretprobe_instances; +#endif +/* * New fields for task_struct should be added above here, so that * they are included in the randomized portion of task_struct.
kernel/fork.c +4 -0
@@ -2161,6 +2161,10 @@ static __latent_entropy struct task_struct *copy_process(INIT_LIST_HEAD (&p->thread_group); p->task_works =NULL ; +#ifdef CONFIG_KRETPROBES + p->kretprobe_instances.first =NULL ; +#endif +/* * Ensure that the cgroup subsystem policies allow the new process to be * forked. It should be noted the the new process's css_set can be changed
kernel/kprobes.c +91 -141
338 lines not on this page · raw message
Masami Hiramatsu
maintainerRe: [PATCH v4 19/23] kprobes: Remove kretprobe hash
Quoted 9 lines · On Fri, 28 Aug 2020 21:30:06 +0900 Masami Hiramatsu <[email protected]> wrote:
From: Peter Zijlstra <[email protected]>
The kretprobe hash is mostly superfluous, replace it with a per-task variable.
This gets rid of the task hash and it's related locking.
The whole
invalidate_rp_inst()is tedious and could go away once we drop rp specific ri size.
OK, something wrong with this patch. Now I can reproduce it always, it takes around 330 second from setting up the kretprobe event on schedule(). Maybe some timer or timeout value affect?
Anyway, before this patch, I can not reproduce the bug yet. After applying this patch, I hit the bug always. So something wrong with this.
----
cd /sys/kernel/debug/tracing/
echo r:schedule schedule >> kprobe_events
echo 1 > events/kprobes/enable
sleep 333
----
And show this.
/sys/kernel/debug/tracing # [ 336.718043] ------------[ cut here ]------------
[ 336.719041] kernel BUG at kernel/kprobes.c:1950! [ 336.720212] invalid opcode: 0000 [#1] PREEMPT SMP PTI [ 336.721206] CPU: 0 PID: 85 Comm: kworker/0:2 Not tainted 5.9.0-rc2+ #58 [ 336.722557] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1 04/01/2014 [ 336.724310] Workqueue: 0x0 (events) [ 336.726997] RIP: 0010:__kretprobe_trampoline_handler+0xe5/0xf0 [ 336.728084] Code: e8 20 04 79 00 65 48 c7 05 ec 4a ec 7e 00 cd 28 82 4c 89 e7 e8 ac fe ff ff 48 85 db 75 97 5b 4c 89 e8 41 5c 41 5d 41 5e 5d c3 <0f> 0b 66 0f 1f 84 00 00 00 00 00 55 48 89 e5 41 54 49 89 fc 53 48 [ 336.731574] RSP: 0018:ffffc90000307dc8 EFLAGS: 00010246 [ 336.732608] RAX: ffff88807d600000 RBX: 0000000000000000 RCX: 0000000000000000 [ 336.733896] RDX: ffffc90000307ea8 RSI: ffffffff810471e0 RDI: ffff88807c579700 [ 336.735205] RBP: ffffc90000307de8 R08: 0000000000000001 R09: 0000000000000001 [ 336.736506] R10: 0000000000000000 R11: 0000000000000001 R12: ffffc90000307e10 [ 336.737820] R13: ffff88807d62a440 R14: ffffc90000307e10 R15: ffff88807cfb5300 [ 336.739125] FS: 0000000000000000(0000) GS:ffff88807d600000(0000) knlGS:0000000000000000 [ 336.740705] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 336.741773] CR2: 000000000212cd38 CR3: 000000007a732000 CR4: 00000000000006b0 [ 336.742953] Call Trace: [ 336.743523] trampoline_handler+0x43/0x60 [ 336.744317] kretprobe_trampoline+0x2a/0x50 [ 336.745098] RIP: 0010:kretprobe_trampoline+0x0/0x50 [ 336.745961] Code: c7 39 2e 04 82 e8 a0 f2 0d 00 5d c3 31 f6 e9 79 ff ff ff be 01 00 00 00 e9 6f ff ff ff cc cc cc cc cc cc cc cc cc cc cc cc cc <54> 9c 48 83 ec 18 57 56 52 51 50 41 50 41 51 41 52 41 53 53 55 41 [ 336.749100] RSP: 7c579700:ffffc90000307eb0 EFLAGS: 00000246 [ 336.750077] RAX: ffff88807cfb5300 RBX: ffff88807d62a440 RCX: 0000000000000000 [ 336.751233] RDX: 0000000000000000 RSI: ffffffff818e5334 RDI: ffff88807c579700 [ 336.752391] RBP: ffffc90000307f00 R08: 0000000000000001 R09: 0000000000000001 [ 336.753563] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88807cfb5328 [ 336.754718] R13: ffff88807d62a440 R14: ffffc90000033d58 R15: ffff88807cfb5300 [ 336.756005] ? schedule+0x54/0x100 [ 336.756684] ? process_one_work+0x5c0/0x5c0 [ 336.757504] kthread+0x13c/0x180 [ 336.758102] ? kthread_park+0x90/0x90 [ 336.758931] ret_from_fork+0x22/0x30 [ 336.759719] Modules linked in: [ 336.760456] ---[ end trace a7f6025840267136 ]--- [ 336.761448] RIP: 0010:__kretprobe_trampoline_handler+0xe5/0xf0 [ 336.762767] Code: e8 20 04 79 00 65 48 c7 05 ec 4a ec 7e 00 cd 28 82 4c 89 e7 e8 ac fe ff ff 48 85 db 75 97 5b 4c 89 e8 41 5c 41 5d 41 5e 5d c3 <0f> 0b 66 0f 1f 84 00 00 00 00 00 55 48 89 e5 41 54 49 89 fc 53 48 [ 336.766128] RSP: 0018:ffffc90000307dc8 EFLAGS: 00010246 [ 336.767112] RAX: ffff88807d600000 RBX: 0000000000000000 RCX: 0000000000000000 [ 336.768321] RDX: ffffc90000307ea8 RSI: ffffffff810471e0 RDI: ffff88807c579700 [ 336.769539] RBP: ffffc90000307de8 R08: 0000000000000001 R09: 0000000000000001 [ 336.770744] R10: 0000000000000000 R11: 0000000000000001 R12: ffffc90000307e10 [ 336.771947] R13: ffff88807d62a440 R14: ffffc90000307e10 R15: ffff88807cfb5300 [ 336.773201] FS: 0000000000000000(0000) GS:ffff88807d600000(0000) knlGS:0000000000000000 [ 336.774706] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 336.775577] CR2: 000000000212cd38 CR3: 000000007a732000 CR4: 00000000000006b0 [ 336.776699] Kernel panic - not syncing: Fatal exception [ 336.777829] Kernel Offset: disabled [ 336.778526] ---[ end Kernel panic - not syncing: Fatal exception ]---
Thank you,
Signature · 1 line
Masami Hiramatsu <[email protected]>
[email protected]
maintainerRe: [PATCH v4 19/23] kprobes: Remove kretprobe hash
Quoted 6 lines · Masami Hiramatsu
cd
/sys/kernel/debug/tracing/echo r:schedule schedule >>
kprobe_events
echo 1 > events/kprobes/enablesleep 333
Thanks! that does indeed trigger it reliably. Let me go have dinner and then I'll try and figure out where it goes side-ways.
RE: [PATCH v4 19/23] kprobes: Remove kretprobe hash
In reply to Masami HiramatsuQuoted 18 lines
-----Original Message-----
From: Masami Hiramatsu <[email protected]>kernel/kprobes.c +2
-4@@ -1311,24 +1257,23 @@ void kprobe_busy_end(void)void kprobe_flush_task (struct task_struct *tk) {struct kretprobe_instance *ri; -struct hlist_head *head; -struct hlist_node *tmp; -unsigned long hash, flags =0 ; +struct llist_node *node; +/* Early boot, not yet initialized. */ if (unlikely (!kprobes_initialized)) -/* Early boot. kretprobe_table_locks not yet initialized. */ return ;
kernel/kprobes.c void kprobe_busy_end(void) · line 1269
Quoted 10 lines
kernel/kprobes.c +2
-6kprobe_busy_begin (); - hash =hash_ptr (tk,KPROBE_HASH_BITS ); - head = &kretprobe_inst_table[hash]; -kretprobe_table_lock (hash, &flags); -hlist_for_each_entry_safe (ri, tmp, head, hlist) { -if (ri->task == tk) -recycle_rp_inst (ri); + node = current->kretprobe_instances.first; + current->kretprobe_instances.first =NULL ;
I think we are flushing tk instead of current here. After fixing this to tk, the NULL pointer deference is gone!
kernel/kprobes.c void kprobe_busy_end(void) · line 1279
Quoted 11 lines
kernel/kprobes.c +6
-1+ +while (node) { + ri = container_of(node, struct kretprobe_instance, llist); + node = node->next; + +recycle_rp_inst (ri); } -kretprobe_table_unlock (hash, &flags);kprobe_busy_end (); }
TREND MICRO EMAIL NOTICE
The information contained in this email and any attachments is confidential and may be subject to copyright or other intellectual property protection. If you are not the intended recipient, you are not authorized to use or disclose this information, and we request that you notify us by reply mail or telephone and delete the original message from your mail system.
For details about what personal information we collect and why, please see our Privacy Notice on our website at: Read privacy policy<http://www.trendmicro.com/privacy>
Peter Zijlstra
maintainerRe: [PATCH v4 19/23] kprobes: Remove kretprobe hash
Quoted 14 lines · On Fri, Aug 28, 2020 at 07:32:11PM +0000, [email protected] wrote:
Quoted 14 lines
-----Original Message-----
From: Masami Hiramatsu <[email protected]>(quoted diff) +2
-0@@ -1311,24 +1257,23 @@ void kprobe_busy_end(void) void kprobe_flush_task(struct task_struct *tk) { struct kretprobe_instance *ri; + struct llist_node *node; + /* Early boot, not yet initialized. */ if (unlikely(!kprobes_initialized)) return;
Quoted 7 lines
Review void kprobe_busy_end(void) · line 1269
Quoted 4 lines
(quoted diff) +2
-0kprobe_busy_begin(); + node = current->kretprobe_instances.first; + current->kretprobe_instances.first = NULL;I think we are flushing tk instead of current here. After fixing this to tk, the NULL pointer deference is gone!
Ah, very good! I can indeed confirm that that cures things.
---
kernel/kprobes.c +3 -6
@@ -1275,9 +1275,7 @@ void kprobe_flush_task(struct task_struckprobe_busy_begin (); - node = current->kretprobe_instances.first; - current->kretprobe_instances.first =NULL ; - + node ==__llist_del_all (&tk->kretprobe_instances);while (node) { ri = container_of(node, struct kretprobe_instance, llist); node = node->next;
@@ -1871,6 +1869,7 @@ unsigned long __kretprobe_trampoline_hanstruct llist_node *first, *node;struct kretprobe *rp; +/* Find all nodes for this frame. */ first = node = current->kretprobe_instances.first;while (node) { ri = container_of(node, struct kretprobe_instance, llist);
@@ -1900,7 +1899,7 @@ unsigned long __kretprobe_trampoline_hanwhile (first) { ri = container_of(first, struct kretprobe_instance, llist); rp =get_kretprobe (ri); - node = first->next; + first = first->next;if (rp && rp->handler) {__this_cpu_write (current_kprobe, &rp->kp);
@@ -1910,8 +1909,6 @@ unsigned long __kretprobe_trampoline_han }recycle_rp_inst (ri); - - first = node; }return (unsigned long )correct_ret_addr;
include/linux/llist.h +8 -0
@@ -237,6 +237,14 @@ static inline struct llist_node *llist_dreturn xchg (&head->first,NULL ); } +static inline struct llist_node *__llist_del_all (struct llist_head *head) +{ +struct llist_node *first = head->first; + + head->first =NULL ; +return first; +} +extern struct llist_node *llist_del_first (struct llist_head *head);struct llist_node *llist_reverse_order (struct llist_node *head);
Masami Hiramatsu
maintainerRe: [PATCH v4 19/23] kprobes: Remove kretprobe hash
Quoted 15 lines · On Fri, 28 Aug 2020 22:29:12 +0200 Peter Zijlstra <[email protected]> wrote:
Quoted 14 lines · On Fri, Aug 28, 2020 at 07:32:11PM +0000, [email protected] wrote:
Quoted 14 lines
-----Original Message-----
From: Masami Hiramatsu <[email protected]>(quoted diff) +2
-0@@ -1311,24 +1257,23 @@ void kprobe_busy_end(void) void kprobe_flush_task(struct task_struct *tk) { struct kretprobe_instance *ri; + struct llist_node *node; + /* Early boot, not yet initialized. */ if (unlikely(!kprobes_initialized)) return;
Quoted 8 lines
Quoted 7 lines
kernel/kprobes.c void kprobe_flush_task(struct task_struc
Quoted 4 lines
kernel/kprobes.c +2
-0kprobe_busy_begin (); + node = current->kretprobe_instances.first; + current->kretprobe_instances.first =NULL ;I think we are flushing tk instead of current here. After fixing this to tk, the NULL pointer deference is gone!
Cool!!
Quoted 1 line
Ah, very good! I can indeed confirm that that cures things.
OK, I'll merge this too.
Thank you,
Quoted 63 lines
---
Index:linux-2.6/kernel/kprobes.c
===================================================================kernel/kprobes.c +3
-6@@ -1275,9 +1275,7 @@ void kprobe_flush_task(struct task_struckprobe_busy_begin (); - node = current->kretprobe_instances.first; - current->kretprobe_instances.first =NULL ; - + node ==__llist_del_all (&tk->kretprobe_instances);while (node) { ri = container_of(node, struct kretprobe_instance, llist); node = node->next;@@ -1871,6 +1869,7 @@ unsigned long __kretprobe_trampoline_hanstruct llist_node *first, *node;struct kretprobe *rp; +/* Find all nodes for this frame. */ first = node = current->kretprobe_instances.first;while (node) { ri = container_of(node, struct kretprobe_instance, llist);@@ -1900,7 +1899,7 @@ unsigned long __kretprobe_trampoline_hanwhile (first) { ri = container_of(first, struct kretprobe_instance, llist); rp =get_kretprobe (ri); - node = first->next; + first = first->next;if (rp && rp->handler) {__this_cpu_write (current_kprobe, &rp->kp);@@ -1910,8 +1909,6 @@ unsigned long __kretprobe_trampoline_han }recycle_rp_inst (ri); - - first = node; }return (unsigned long )correct_ret_addr;Index:
linux-2.6/include/linux/llist.h
===================================================================include/linux/llist.h +8
-0@@ -237,6 +237,14 @@ static inline struct llist_node *llist_dreturn xchg (&head->first,NULL ); } +static inline struct llist_node *__llist_del_all (struct llist_head *head) +{ +struct llist_node *first = head->first; + + head->first =NULL ; +return first; +} +extern struct llist_node *llist_del_first (struct llist_head *head);struct llist_node *llist_reverse_order (struct llist_node *head);
Signature · 1 line
Masami Hiramatsu <[email protected]>
Masami Hiramatsu
maintainerRFC PATCH v4 20/23 kprobes: Remove task scan for updating kretprobe_instance
In reply to Masami HiramatsuRemove task scan for updating kretprobe_instance->rp when unregistering kretprobe. Instead, this introduces the kretprobe_holder which is a kretprobe pointer holder with refcount. When we unregister the kretprobe, we update the pointer value in the holder which means this kretprobe is already removed. When the used kretprobe instance hits when the target function return, it will decrement the holder's refcount and if it is the last one, it will free the holder.
Note that this may change the kprobes module-exported API for kretprobe handlers. If someone use out-of-tree kretprobe, they have to update the kretprobe handler to use get_kretprobe(ri) for accessing kretprobe data structure instead of ri->rp. Also, now we remove ri->task.
--- include/linux/kprobes.h | 17 ++++++ kernel/kprobes.c | 113 +++++++++++++------------------------------ kernel/trace/trace_kprobe.c | 3 + 3 files changed, 51 insertions(+), 82 deletions(-)
include/linux/kprobes.h +15 -2
@@ -27,6 +27,7 @@#include <linux/rcupdate.h> #include <linux/mutex.h> #include <linux/ftrace.h> +#include <linux/refcount.h> #include <asm/kprobes.h> #ifdef CONFIG_KPROBES
@@ -144,6 +145,11 @@ static inline int kprobe_ftrace(struct kprobe *p)* ignored, due to maxactive being too low. * */ +struct kretprobe_holder { +struct kretprobe *rp; +refcount_t ref; +}; +struct kretprobe {struct kprobe kp;kretprobe_handler_t handler;
@@ -152,6 +158,7 @@ struct kretprobe {int nmissed;size_t data_size;struct hlist_head free_instances; +struct kretprobe_holder *rph;raw_spinlock_t lock; };
@@ -161,9 +168,8 @@ struct kretprobe_instance {struct hlist_node hlist;struct rcu_head rcu; }; -struct kretprobe *rp; +struct kretprobe_holder *rph;kprobe_opcode_t *ret_addr; -struct task_struct *task;void *fp;char data[]; };
@@ -222,6 +228,13 @@ unsigned long kretprobe_trampoline_handler(struct pt_regs *regs,return ret; } +static nokprobe_inline struct kretprobe *get_kretprobe(struct kretprobe_instance *ri) +{ +/* rph->rp can be updated by unregister_kretprobe() on other cpu */ +smp_rmb (); +return ri->rph->rp; +} +#else /* CONFIG_KRETPROBES */ static inline void arch_prepare_kretprobe (struct kretprobe *rp,struct pt_regs *regs)
kernel/kprobes.c +34 -79
192 lines not on this page · raw message
kernel/trace/trace_kprobe.c +2 -1
@@ -1714,7 +1714,8 @@ NOKPROBE_SYMBOL(kprobe_dispatcher);static int kretprobe_dispatcher (struct kretprobe_instance *ri,struct pt_regs *regs) { - struct trace_kprobe *tk = container_of(ri->rp, struct trace_kprobe, rp); +struct kretprobe *rp =get_kretprobe (ri); + struct trace_kprobe *tk = container_of(rp, struct trace_kprobe, rp);raw_cpu_inc (*tk->nhit);
[email protected]
maintainerRe: [PATCH v4 20/23] [RFC] kprobes: Remove task scan for updating kretprobe_instance
If you do this, can you merge this into the previos patch and then delete the sched try_to_invoke..() patch?
Few comments below.
include/linux/kprobes.h unsigned long kretprobe_trampoline_handler(struct pt_regs *regs, · line 236
Quoted 6 lines · Masami Hiramatsu
include/linux/kprobes.h +6
-0+static nokprobe_inline struct kretprobe *get_kretprobe(struct kretprobe_instance *ri) +{ +/* rph->rp can be updated by unregister_kretprobe() on other cpu */ +smp_rmb (); +return ri->rph->rp; +}
That ordering doesn't really make sense, ordering requires at least two variables, here there is only 1. That said, get functions usually need an ACQUIRE order to make sure subsequent accesses are indeed done later.
Quoted 13 lines
#else /*
CONFIG_KRETPROBES*/
static inline void arch_prepare_kretprobe(struct kretprobe *rp,include/linux/kprobes.h +1
-0struct pt_regs *regs)@@ -1922,6 +1869,7 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,kprobe_opcode_t *correct_ret_addr =NULL ;struct kretprobe_instance *ri =NULL ;struct llist_node *first, *node; +struct kretprobe *rp; first = node = current->kretprobe_instances.first;while (node) {
include/linux/kprobes.h unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs, · line 1903
Quoted 5 lines
include/linux/kprobes.h +1
-0@@ -1951,12 +1899,13 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,/* Run them.. */ while (first) { ri = container_of(first, struct kretprobe_instance, llist); + rp =get_kretprobe (ri); node = first->next;
(A)
kernel/kprobes.c unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs, · line 1910
Quoted 9 lines
kernel/kprobes.c +3
-3-if (ri->rp && ri->rp->handler) { -__this_cpu_write (current_kprobe, &ri->rp->kp); +if (rp && rp->handler) { +__this_cpu_write (current_kprobe, &rp->kp); ri->ret_addr = correct_ret_addr; - ri->rp->handler (ri, regs); + rp->handler (ri, regs);__this_cpu_write (current_kprobe, &kprobe_busy); }
So here we're using get_kretprobe(), but what is to stop anybody from doing unregister_kretprobe() right at (A) such that we did observe our rp, but by the time we use it, it's a goner.
kernel/kprobes.c int register_kretprobe(struct kretprobe *rp) · line 2013
Quoted 2 lines
kernel/kprobes.c +2
-0+ rp->rph =kzalloc (sizeof (struct kretprobe_holder ),GFP_KERNEL ); + rp->rph->rp = rp;
I think you'll need to check the allocation succeeded, no? :-)
kernel/kprobes.c void unregister_kretprobes(struct kretprobe **rps, int num) · line 2075
Quoted 13 lines
kernel/kprobes.c +5
-1@@ -2114,16 +2065,20 @@ void unregister_kretprobes(struct kretprobe **rps, int num)if (num <=0 )return ;mutex_lock (&kprobe_mutex); -for (i =0 ; i < num; i++) +for (i =0 ; i < num; i++) {if (__unregister_kprobe_top (&rps[i]->kp) <0 ) rps[i]->kp.addr =NULL ; + rps[i]->rph->rp =NULL ; + } +/* Ensure the rph->rp updated after this */ +smp_wmb ();mutex_unlock (&kprobe_mutex);
That ordering is dodgy again, those barriers don't help anything if someone else is at (A) above.
kernel/kprobes.c void unregister_kretprobes(struct kretprobe **rps, int num) · line 2077
Quoted 2 lines
kernel/kprobes.c +0
-0synchronize_rcu ();
This one might help, this means we can do rcu_read_lock() around get_kretprobe() and it's usage. Can we call rp->handler() under RCU?
Quoted 8 lines
kernel/kprobes.c +1
-1for (i =0 ; i < num; i++) {if (rps[i]->kp.addr) {__unregister_kprobe_bottom (&rps[i]->kp); -cleanup_rp_inst (rps[i]); +free_rp_inst (rps[i]); } } }
Masami Hiramatsu
maintainerRe: [PATCH v4 20/23] [RFC] kprobes: Remove task scan for updating kretprobe_instance
Quoted 3 lines · On Fri, 28 Aug 2020 14:52:36 +0200 [email protected] wrote:
If you do this, can you merge this into the previos patch and then delete the sched try_to_invoke..() patch?
Yes, this is just for making code review easy. :)
Quoted 6 lines
Few comments below.
On Fri, Aug 28, 2020 at 09:30:17PM +0900, Masami Hiramatsu wrote:
Quoted 10 lines
include/linux/kprobes.h unsigned long kretprobe_trampoline_handler(struct pt_regs *regs, · line 236
Quoted 6 lines
include/linux/kprobes.h +6
-0+static nokprobe_inline struct kretprobe *get_kretprobe(struct kretprobe_instance *ri) +{ +/* rph->rp can be updated by unregister_kretprobe() on other cpu */ +smp_rmb (); +return ri->rph->rp; +}That ordering doesn't really make sense, ordering requires at least two variables, here there is only 1. That said, get functions usually need an ACQUIRE order to make sure subsequent accesses are indeed done later.
So,
return smp_load_acquire(ri->rph->rp);
will be enough?
Quoted 22 lines
Quoted 14 lines
#else /*
CONFIG_KRETPROBES*/
static inline void arch_prepare_kretprobe(struct kretprobe *rp,include/linux/kprobes.h +1
-0struct pt_regs *regs)@@ -1922,6 +1869,7 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,kprobe_opcode_t *correct_ret_addr =NULL ;struct kretprobe_instance *ri =NULL ;struct llist_node *first, *node; +struct kretprobe *rp; first = node = current->kretprobe_instances.first;while (node) {include/linux/kprobes.h unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs, · line 1903
Quoted 5 lines
include/linux/kprobes.h +1
-0@@ -1951,12 +1899,13 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,/* Run them.. */ while (first) { ri = container_of(first, struct kretprobe_instance, llist); + rp =get_kretprobe (ri); node = first->next;(A)
Quoted 13 lines
kernel/kprobes.c unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs, · line 1910
Quoted 9 lines
kernel/kprobes.c +3
-3-if (ri->rp && ri->rp->handler) { -__this_cpu_write (current_kprobe, &ri->rp->kp); +if (rp && rp->handler) { +__this_cpu_write (current_kprobe, &rp->kp); ri->ret_addr = correct_ret_addr; - ri->rp->handler (ri, regs); + rp->handler (ri, regs);__this_cpu_write (current_kprobe, &kprobe_busy); }So here we're using
get_kretprobe(), but what is to stop anybody from doingunregister_kretprobe()right at (A) such that we did observe our rp, but by the time we use it, it's a goner.
In kprobe_busy_begin() we disable preempt, so this block is not preemptive. And as you may know, the unregister_kretprobe() is waiting rcu grace period after it clear the rp->rph->rp. So, someone does unregister_kretprobe() at (A), rph->rp = NULL but rp itself is not released until all running trampoline_handlers exit.
Quoted 6 lines
kernel/kprobes.c int register_kretprobe(struct kretprobe *rp) · line 2013
Quoted 4 lines
kernel/kprobes.c +2
-0+ rp->rph =kzalloc (sizeof (struct kretprobe_holder ),GFP_KERNEL ); + rp->rph->rp = rp;I think you'll need to check the allocation succeeded, no? :-)
Oops, I had found it once but forgot to fix :(
Quoted 20 lines
Quoted 5 lines
kernel/kprobes.c +0
-0@@ -2114,16 +2065,20 @@ void unregister_kretprobes(struct kretprobe **rps, int num)if (num <=0 )return ;kernel/kprobes.c void unregister_kretprobes(struct kretprobe **rps, int num) · line 2075
Quoted 10 lines
kernel/kprobes.c +5
-1mutex_lock (&kprobe_mutex); -for (i =0 ; i < num; i++) +for (i =0 ; i < num; i++) {if (__unregister_kprobe_top (&rps[i]->kp) <0 ) rps[i]->kp.addr =NULL ; + rps[i]->rph->rp =NULL ; + } +/* Ensure the rph->rp updated after this */ +smp_wmb ();mutex_unlock (&kprobe_mutex);That ordering is dodgy again, those barriers don't help anything if someone else is at (A) above.
Quoted 1 line
Quoted 4 lines
Quoted 1 line
synchronize_rcu();This one might help, this means we can do
rcu_read_lock()aroundget_kretprobe()and it's usage. Can we callrp->handler()under RCU?
Yes, as I said above, the get_kretprobe() (and kretprobe handler) must be called under preempt-disabled.
Thank you,
Quoted 9 lines
Quoted 9 lines
kernel/kprobes.c +1
-1for (i =0 ; i < num; i++) {if (rps[i]->kp.addr) {__unregister_kprobe_bottom (&rps[i]->kp); -cleanup_rp_inst (rps[i]); +free_rp_inst (rps[i]); } } }
Signature · 1 line
Masami Hiramatsu <[email protected]>
[email protected]
maintainerRe: [PATCH v4 20/23] [RFC] kprobes: Remove task scan for updating kretprobe_instance
Quoted 10 lines · Masami Hiramatsu
Quoted 4 lines · On Fri, 28 Aug 2020 14:52:36 +0200 [email protected] wrote:
Quoted 1 line
synchronize_rcu();This one might help, this means we can do
rcu_read_lock()aroundget_kretprobe()and it's usage. Can we callrp->handler()under RCU?Yes, as I said above, the
get_kretprobe()(and kretprobe handler) must be called under preempt-disabled.
Then we don't need the ordering; we'll need READ_ONCE() (or rcu_derefernce()) to make sure the address dependency works on Alpha. And a comment/assertion that explains this might not go amiss in get_kretprobe().
Masami Hiramatsu
maintainerRe: [PATCH v4 20/23] [RFC] kprobes: Remove task scan for updating kretprobe_instance
Quoted 9 lines · On Fri, 28 Aug 2020 17:18:06 +0200 [email protected] wrote:
Quoted 8 lines · Masami Hiramatsu
Quoted 4 lines · On Fri, 28 Aug 2020 14:52:36 +0200 [email protected] wrote:
Quoted 1 line
synchronize_rcu();This one might help, this means we can do
rcu_read_lock()aroundget_kretprobe()and it's usage. Can we callrp->handler()under RCU?
Quoted 7 lines
Quoted 2 lines
Yes, as I said above, the
get_kretprobe()(and kretprobe handler) must be called under preempt-disabled.Then we don't need the ordering; we'll need
READ_ONCE()(orrcu_derefernce()) to make sure the address dependency works on Alpha. And a comment/assertion that explains this might not go amiss inget_kretprobe().
OK, I'll rewrite it with READ_ONCE() and rcu_read_lock_any_held().
Thank you,
Signature · 1 line
Masami Hiramatsu <[email protected]>
Masami Hiramatsu
maintainerPATCH v4 21/23 asm-generic/atomic: Add try_cmpxchg() fallbacks
In reply to Masami HiramatsuFrom: Peter Zijlstra <[email protected]>
Only x86 provides try_cmpxchg() outside of the atomic_t interfaces, provide generic fallbacks to create this interface from the widely available cmpxchg() function.
--- arch/x86/include/asm/atomic.h | 2 arch/x86/include/asm/atomic64_64.h | 2 arch/x86/include/asm/cmpxchg.h | 2 include/asm-generic/atomic-instrumented.h | 216 +++++++++++++++++------------ include/linux/atomic-arch-fallback.h | 90 +++++++++++- include/linux/atomic-fallback.h | 90 +++++++++++- scripts/atomic/gen-atomic-fallback.sh | 63 ++++++++ scripts/atomic/gen-atomic-instrumented.sh | 29 +++- 8 files changed, 372 insertions(+), 122 deletions(-)
arch/x86/include/asm/atomic.h +1 -1
@@ -199,7 +199,7 @@ static __always_inline int arch_atomic_cmpxchg(atomic_t *v, int old, int new) static __always_inline bool arch_atomic_try_cmpxchg(atomic_t *v, int *old, int new) { -return try_cmpxchg (&v->counter, old, new); +return arch_try_cmpxchg (&v->counter, old, new); }#define arch_atomic_try_cmpxchg arch_atomic_try_cmpxchg
arch/x86/include/asm/atomic64_64.h +1 -1
@@ -187,7 +187,7 @@ static inline s64 arch_atomic64_cmpxchg(atomic64_t *v, s64 old, s64 new) static __always_inline bool arch_atomic64_try_cmpxchg(atomic64_t *v, s64 *old, s64 new) { -return try_cmpxchg (&v->counter, old, new); +return arch_try_cmpxchg (&v->counter, old, new); }#define arch_atomic64_try_cmpxchg arch_atomic64_try_cmpxchg
arch/x86/include/asm/cmpxchg.h +1 -1
@@ -221,7 +221,7 @@ extern void __add_wrong_size(void)#define __try_cmpxchg (ptr, pold, new, size) \ __raw_try_cmpxchg((ptr), (pold), (new), (size), LOCK_PREFIX) -#define try_cmpxchg (ptr, pold, new) \ +#define arch_try_cmpxchg (ptr, pold, new) \ __try_cmpxchg((ptr), (pold), (new), sizeof(*(ptr)))/*
include/asm-generic/atomic-instrumented.h +130 -86
278 lines not on this page · raw message
include/linux/atomic-arch-fallback.h +80 -10
117 lines not on this page · raw message
include/linux/atomic-fallback.h +80 -10
117 lines not on this page · raw message
scripts/atomic/gen-atomic-fallback.sh +56 -7
88 lines not on this page · raw message
scripts/atomic/gen-atomic-instrumented.sh +23 -6
@@ -103,14 +103,31 @@ gen_xchg() local xchg="$1"; shift local mult="$1"; shift + if [ "${xchg%${xchg#try_cmpxchg}}" = "try_cmpxchg" ] ; then + +cat <<EOF +#define ${xchg}(ptr, oldp, ...) \\ +({ \\ + typeof(ptr) __ai_ptr = (ptr); \\ + typeof(oldp) __ai_oldp = (oldp); \\ + instrument_atomic_write(__ai_ptr, ${mult}sizeof(*__ai_ptr)); \\ + instrument_atomic_write(__ai_oldp, ${mult}sizeof(*__ai_oldp)); \\ + arch_${xchg}(__ai_ptr, __ai_oldp, __VA_ARGS__); \\ +}) +EOF + + else + cat <<EOF -#define ${xchg}(ptr, ...) \\ -({ \\ - typeof(ptr) __ai_ptr = (ptr); \\ - instrument_atomic_write(__ai_ptr, ${mult}sizeof(*__ai_ptr)); \\ - arch_${xchg}(__ai_ptr, __VA_ARGS__); \\ +#define ${xchg}(ptr, ...) \\ +({ \\ + typeof(ptr) __ai_ptr = (ptr); \\ + instrument_atomic_write(__ai_ptr, ${mult}sizeof(*__ai_ptr)); \\ + arch_${xchg}(__ai_ptr, __VA_ARGS__); \\ }) EOF + + fi } gen_optional_xchg()
@@ -160,7 +177,7 @@ grep '^[a-z]' "$1" | while read name meta args; do gen_proto "${meta}" "${name}" "atomic64" "s64" ${args} done -for xchg in "xchg" "cmpxchg" "cmpxchg64"; do +for xchg in "xchg" "cmpxchg" "cmpxchg64" "try_cmpxchg"; do for order in "" "_acquire" "_release" "_relaxed"; do gen_optional_xchg "${xchg}" "${order}" done
Masami Hiramatsu
maintainerPATCH v4 22/23 freelist: Lock less freelist
In reply to Masami HiramatsuFrom: Peter Zijlstra <[email protected]>
--- include/linux/freelist.h | 129 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 include/linux/freelist.h
include/linux/freelist.h new file +129 -0
@@ -0,0 +1,129 @@ +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ +#ifndef FREELIST_H +#define FREELIST_H + +#include <linux/atomic.h> + +/* +* Copyright: [email protected] +* +* A simple CAS-based lock-free free list. Not the fastest thing in the world +* under heavy contention, but simple and correct (assuming nodes are never +* freed until after the free list is destroyed), and fairly speedy under low +* contention. +* +* Adapted from: https://moodycamel.com/blog/2014/solving-the-aba-problem-for-lock-free-free-lists +*/ + +struct freelist_node { +atomic_t refs; +struct freelist_node *next; +}; + +struct freelist_head { +struct freelist_node *head; +}; + +#define REFS_ON_FREELIST 0x80000000 +#define REFS_MASK 0x7FFFFFFF + +static inline void __freelist_add (struct freelist_node *node,struct freelist_head *list) +{ +/* +* Since the refcount is zero, and nobody can increase it once it's +* zero (except us, and we run only one copy of this method per node at +* a time, i.e. the single thread case), then we know we can safely +* change the next pointer of the node; however, once the refcount is +* back above zero, then other threads could increase it (happens under +* heavy contention, when the refcount goes to zero in between a load +* and a refcount increment of a node in try_get, then back up to +* something non-zero, then the refcount increment is done by the other +* thread) -- so if the CAS to add the node to the actual list fails, +* decrese the refcount and leave the add operation to the next thread +* who puts the refcount back to zero (which could be us, hence the +* loop). +*/ +struct freelist_node *head =READ_ONCE (list->head); + +for (;;) { +WRITE_ONCE (node->next, head); +atomic_set_release (&node->refs,1 ); + +if (!try_cmpxchg_release (&list->head, &head, node)) { +/* +* Hmm, the add failed, but we can only try again when +* the refcount goes back to zero. +*/ +if (atomic_fetch_add_release (REFS_ON_FREELIST -1 , &node->refs) ==1 ) +continue ; + } +return ; + } +} + +static inline void freelist_add (struct freelist_node *node,struct freelist_head *list) +{ +/* +* We know that the should-be-on-freelist bit is 0 at this point, so +* it's safe to set it using a fetch_add. +*/ +if (!atomic_fetch_add_release (REFS_ON_FREELIST , &node->refs)) { +/* +* Oh look! We were the last ones referencing this node, and we +* know we want to add it to the free list, so let's do it! +*/ +__freelist_add (node, list); + } +} + +static inline struct freelist_node *freelist_try_get (struct freelist_head *list) +{ +struct freelist_node *prev, *next, *head =smp_load_acquire (&list->head); +unsigned int refs; + +while (head) { + prev = head; + refs =atomic_read (&head->refs); +if ((refs &REFS_MASK ) ==0 || + !atomic_try_cmpxchg_acquire (&head->refs, &refs, refs+1 )) { + head =smp_load_acquire (&list->head); +continue ; + } + +/* +* Good, reference count has been incremented (it wasn't at +* zero), which means we can read the next and not worry about +* it changing between now and the time we do the CAS. +*/ + next =READ_ONCE (head->next); +if (try_cmpxchg_acquire (&list->head, &head, next)) { +/* +* Yay, got the node. This means it was on the list, +* which means should-be-on-freelist must be false no +* matter the refcount (because nobody else knows it's +* been taken off yet, it can't have been put back on). +*/ +WARN_ON_ONCE (atomic_read (&head->refs) &REFS_ON_FREELIST ); + +/* +* Decrease refcount twice, once for our ref, and once +* for the list's ref. +*/ +atomic_fetch_add (-2 , &head->refs); + +return head; + } + +/* +* OK, the head must have changed on us, but we still need to decrement +* the refcount we increased. +*/ + refs =atomic_fetch_add (-1 , &prev->refs); +if (refs ==REFS_ON_FREELIST +1 ) +__freelist_add (prev, list); + } + +return NULL ; +} + +#endif /* FREELIST_H */
Masami Hiramatsu
maintainerPATCH v4 23/23 kprobes: Replace rp->free_instance with freelist
In reply to Masami HiramatsuFrom: Peter Zijlstra <[email protected]>
Gets rid of rp->lock, and as a result kretprobes are now fully lockless.
---
Changes
- [MH] expel the llist from anon union in kretprobe_instance
--- include/linux/kprobes.h | 8 +++---- kernel/kprobes.c | 56 ++++++++++++++++++++--------------------------- 2 files changed, 28 insertions(+), 36 deletions(-)
include/linux/kprobes.h +4 -4
@@ -28,6 +28,7 @@#include <linux/mutex.h> #include <linux/ftrace.h> #include <linux/refcount.h> +#include <linux/freelist.h> #include <asm/kprobes.h> #ifdef CONFIG_KPROBES
@@ -157,17 +158,16 @@ struct kretprobe {int maxactive;int nmissed;size_t data_size; -struct hlist_head free_instances; +struct freelist_head freelist;struct kretprobe_holder *rph; -raw_spinlock_t lock; };struct kretprobe_instance {union { -struct llist_node llist; -struct hlist_node hlist; +struct freelist_node freelist;struct rcu_head rcu; }; +struct llist_node llist;struct kretprobe_holder *rph;kprobe_opcode_t *ret_addr;void *fp;
kernel/kprobes.c +24 -32
94 lines not on this page · raw message
Masami Hiramatsu
maintainerQuoted 13 lines · On Fri, 28 Aug 2020 21:26:38 +0900 Masami Hiramatsu <[email protected]> wrote:
Hi,
Here is the 4th version of the series to unify the kretprobe trampoline handler and make kretprobe lockless.
Previous version is here;
https://lkml.kernel.org/r/159854631442.736475.5062989489155389472.stgit@devnote2
In this version, I updated the generic trampoline handler a bit, merge the Peter's lockless
patches(*), and add an RFC "remove task scan" patch as [20/23].
Quoted 5 lines
(*) https://lkml.kernel.org/r/[email protected]
I ran some tests and ftracetest on x86-64. Mostly OK, but hit a BUG in the trampoline handler once. I'm trying to reproduce it but not succeeded yet. So this may need a careful review and tests.
The series is also available ongit://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git lockless-kretprobe-v4
Thank you,
Signature · 1 line
Masami Hiramatsu <[email protected]>