Skip to contentSkip to conversation

kprobes: Unify kretprobe trampoline handlers and make kretprobe lockless

PATCH Superseded Masami Hiramatsu 5 participants 41 messages Aug 28, 2020 – Aug 29, 2020

Subsystems (20)
DRM drivers Arm (32-bit) x86 PowerPC +16 more subsystems

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Masami Hiramatsu

author maintainer
#0 lore raw

v4 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]>

author maintainer
#1 lore raw

PATCH 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
index 9be1bff4f586..c6a913e608b7 100644
@@ -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

author maintainer
#2 lore raw

PATCH 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
index fdadc37d72af..882b95313ad6 100644
@@ -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 = &regs->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, &regs->sp);
 }
 NOKPROBE_SYMBOL(trampoline_handler);
 
author maintainer
#3 lore raw

PATCH v4 3/23 arm: kprobes: Use generic kretprobe trampoline handler

In reply to Masami Hiramatsu

Use 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
index feefa2055eba..a9653117ca0d 100644
@@ -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;
author maintainer
#4 lore raw

PATCH v4 4/23 arm64: kprobes: Use generic kretprobe trampoline handler

In reply to Masami Hiramatsu

Use 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
index 5290f17a4d80..deba738142ed 100644
@@ -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;
reviewer
#5 lore raw

Re: [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.

---
 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.

#6 lore raw

Re: [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.

---
 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?

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.

reviewer
#7 lore raw

Re: [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.

---
 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?

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->rp is 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.

author maintainer
#8 lore raw

Re: [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.

---
 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?

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->rp is 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]>
author maintainer
#9 lore raw

PATCH 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
index 7d3efe83cba7..cabef45f11df 100644
@@ -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
author maintainer
#10 lore raw

PATCH 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
index f0f733b7ac5a..589f090f48b9 100644
@@ -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;
 }
 
author maintainer
#11 lore raw

PATCH 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
index 7a7df944d798..fc1ff8a4d7de 100644
@@ -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;
author maintainer
#12 lore raw

PATCH 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
index d043c2f897fc..54dfba8fa77c 100644
@@ -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
author maintainer
#13 lore raw

PATCH 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
index 77ec51818916..6d21a515eea5 100644
@@ -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;
author maintainer
#14 lore raw

PATCH 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
index 6ab9b4d037c3..01ab2163659e 100644
@@ -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);
author maintainer
#15 lore raw

PATCH 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
index d2a71d872638..fc30e799bd84 100644
@@ -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
author maintainer
#16 lore raw

PATCH 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
index 318296f48f1a..756100b01e84 100644
@@ -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)
author maintainer
#17 lore raw

PATCH 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
index dfbca2470536..217c21a6986a 100644
@@ -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
author maintainer
#18 lore raw

PATCH v4 14/23 kprobes: Remove NMI context check

In reply to Masami Hiramatsu

Since 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
index 46510e5000ff..c8de76d230e3 100644
@@ -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);
author maintainer
#19 lore raw

PATCH v4 15/23 kprobes: Free kretprobe_instance with rcu callback

In reply to Masami Hiramatsu

Free 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
index c6a913e608b7..663be8debf25 100644
@@ -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
index c8de76d230e3..807d4429e8a2 100644
@@ -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)
author maintainer
#20 lore raw

PATCH v4 16/23 kprobes: Make local used functions static

In reply to Masami Hiramatsu

Since 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
index 663be8debf25..9c880c8a4e80 100644
@@ -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
index 807d4429e8a2..d0b4b7e89fa6 100644
@@ -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) {
author maintainer
#21 lore raw

PATCH v4 17/23 llist: Add nonatomic __llist_add()

In reply to Masami Hiramatsu

From: 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
index 0b2fe55e6194..0e851b925c8c 100644
@@ -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
index 2e9c7215882b..c17893dcc591 100644
@@ -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
author maintainer
#22 lore raw

PATCH v4 18/23 sched: Fix try_invoke_on_locked_down_task() semantics

In reply to Masami Hiramatsu

From: Peter Zijlstra <[email protected]>

---
 kernel/sched/core.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
kernel/sched/core.c +4 -5
index 2d95dc3f4644..13b0db2d0be2 100644
@@ -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;
 }
 
author maintainer
#23 lore raw

Re: [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
---
 kernel/sched/core.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
kernel/sched/core.c +4 -5
index 2d95dc3f4644..13b0db2d0be2 100644
@@ -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]>
#24 lore raw

Re: [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_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?

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.

#25 lore raw

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_holder things 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
index 8471a0f..a814028 100644
@@ -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;
 }
 
author maintainer
#26 lore raw

PATCH v4 19/23 kprobes: Remove kretprobe hash

In reply to Masami Hiramatsu

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.

---
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
index 9c880c8a4e80..a30cccb07f21 100644
@@ -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
index 93ecd930efd3..0f2532f052a9 100644
@@ -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
index 4d32190861bd..2ff5cceb0732 100644
@@ -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

author maintainer
#27 lore raw

Re: [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]>
#28 lore raw

Re: [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/enable

sleep 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.

#29 lore raw

RE: [PATCH v4 19/23] kprobes: Remove kretprobe hash

In reply to Masami Hiramatsu
Quoted 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 -6
        kprobe_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>

maintainer
#30 lore raw

Re: [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 -0
        kprobe_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
Index: linux-2.6/kernel/kprobes.c
===================================================================
@@ -1275,9 +1275,7 @@ void kprobe_flush_task(struct task_struc
 
 	kprobe_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_han
 	struct 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_han
 	while (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
Index: linux-2.6/include/linux/llist.h
===================================================================
@@ -237,6 +237,14 @@ static inline struct llist_node *llist_d
 	return 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);
author maintainer
#31 lore raw

Re: [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 -0
        kprobe_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_struc
 
 	kprobe_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_han
 	struct 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_han
 	while (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_d
 	return 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]>
author maintainer
#32 lore raw

RFC PATCH v4 20/23 kprobes: Remove task scan for updating kretprobe_instance

In reply to Masami Hiramatsu

Remove 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
index a30cccb07f21..d7cdae2d8f2e 100644
@@ -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
index aefb6065b508..07baf6f6cecc 100644
@@ -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);
 
#33 lore raw

Re: [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 -0
 					struct 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 -0
 	synchronize_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 -1
 	for (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]);
 		}
 	}
 }
author maintainer
#34 lore raw

Re: [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 -0
 					struct 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 doing unregister_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 -1
 	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.

Quoted 1 line
Quoted 4 lines
Quoted 1 line

synchronize_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?

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 -1
 	for (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]>
#35 lore raw

Re: [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() around get_kretprobe() and it's usage. Can we call rp->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().

author maintainer
#36 lore raw

Re: [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() around get_kretprobe() and it's usage. Can we call rp->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() (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().

OK, I'll rewrite it with READ_ONCE() and rcu_read_lock_any_held().

Thank you,

Signature · 1 line
Masami Hiramatsu <[email protected]>
author maintainer
#37 lore raw

PATCH v4 21/23 asm-generic/atomic: Add try_cmpxchg() fallbacks

In reply to Masami Hiramatsu

From: 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
index b6cac6e9bb70..f732741ad7c7 100644
@@ -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
index 809bd010a751..7886d0578fc9 100644
@@ -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
index a8bfac131256..4d4ec5cbdc51 100644
@@ -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
index 6afadf73da17..85dc25685c0d 100755
@@ -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
author maintainer
#38 lore raw

PATCH v4 22/23 freelist: Lock less freelist

In reply to Masami Hiramatsu

From: 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
new file mode 100644
index 000000000000..fc1842b96469
@@ -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 */
author maintainer
#39 lore raw

PATCH v4 23/23 kprobes: Replace rp->free_instance with freelist

In reply to Masami Hiramatsu

From: 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
index d7cdae2d8f2e..aa330bfefefd 100644
@@ -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

author maintainer
#40 lore raw
In reply to Masami Hiramatsu
Quoted 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 on
git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git lockless-kretprobe-v4

Thank you,

Signature · 1 line
Masami Hiramatsu <[email protected]>