kprobes: Fix stacktrace with kretprobes on x86

Commit e792ff804f49 ("ia64: kprobes: Use generic kretprobe trampoline handler") missed to pass the wrong trampoline address (it passes the descriptor address instead of function entry address).
This fixes it to pass correct trampoline address to __kretprobe_trampoline_handler(). This also changes to use correct symbol dereference function to get the function address from the kretprobe_trampoline.

Fixes e792ff804f49 ("ia64: kprobes: Use generic kretprobe trampoline handler")

Signed-off-by Masami Hiramatsu <[email protected]>

Quoted 9 lines · Masami Hiramatsu

Commit e792ff804f49 ("ia64: kprobes: Use generic kretprobe trampoline handler") missed to pass the wrong trampoline address (it passes the descriptor address instead of function entry address).
This fixes it to pass correct trampoline address to __kretprobe_trampoline_handler(). This also changes to use correct symbol dereference function to get the function address from the kretprobe_trampoline.

Fixes e792ff804f49 ("ia64: kprobes: Use generic kretprobe trampoline handler")

Signed-off-by Masami Hiramatsu <[email protected]>

A better changelog:

The following commit:

Commit e792ff804f49 ("ia64: kprobes: Use generic kretprobe trampoline handler")

Passed the wrong trampoline address to __kretprobe_trampoline_handler(): it passes the descriptor address instead of function entry address.

Pass the right parameter.

Also use correct symbol dereference function to get the function address from 'kretprobe_trampoline' - an IA64 special.

(Although I realize that much of this goes away just a couple of patches later.)

Replace arch_deref_entry_point() with dereference_symbol_descriptor() because those are doing same thing.

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Andrii Nakryik <[email protected]>

Quoted 5 lines · Masami Hiramatsu

Replace arch_deref_entry_point() with dereference_symbol_descriptor() because those are doing same thing.

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Andrii Nakryik <[email protected]>

A better changelog:

~15 years ago kprobes grew the 'arch_deref_entry_point()' __weak function:

3d7e33825d87: ("jprobes: make jprobes a little safer for users")

But this is just open-coded dereference_symbol_descriptor() in essence, and its obscure nature was causing bugs.

Just use the real thing.

Thanks,

Ingo

Replace arch_deref_entry_point() with dereference_symbol_descriptor() because those are doing same thing.

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Andrii Nakryik <[email protected]>

---
Changes in v6:
- Use dereference_symbol_descriptor() so that it can handle address in modules correctly.

---
 arch/ia64/kernel/kprobes.c    |    5 -----
 arch/powerpc/kernel/kprobes.c |   11 -----------
 include/linux/kprobes.h       |    1 -
 kernel/kprobes.c              |    7 +------
 lib/error-inject.c            |    3 ++-
 5 files changed, 3 insertions(+), 24 deletions(-)

Quoted 4 lines · Masami Hiramatsu

Replace arch_deref_entry_point() with dereference_symbol_descriptor() because those are doing same thing.

Quoted 2 lines

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Andrii Nakryik <[email protected]>

Hi Masami,

If you are going to post v9 anyway, can you please fix up my name, it should be "Andrii Nakryiko", thanks!

Quoted 13 lines

---
Changes in v6:
- Use dereference_symbol_descriptor() so that it can handle address in modules correctly.

---
 arch/ia64/kernel/kprobes.c    |    5 -----
 arch/powerpc/kernel/kprobes.c |   11 -----------
 include/linux/kprobes.h       |    1 -
 kernel/kprobes.c              |    7 +------
 lib/error-inject.c            |    3 ++-
 5 files changed, 3 insertions(+), 24 deletions(-)

[...]

include/linux/kprobes.h +0 -0

@@ -197,15 +197,23 @@ extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				   struct pt_regs *regs);
 extern int arch_trampoline_kprobe(struct kprobe *p);

Quoted 5 lines · Masami Hiramatsu

include/linux/kprobes.h +0 -0

@@ -197,15 +197,23 @@ extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				   struct pt_regs *regs);
 extern int arch_trampoline_kprobe(struct kprobe *p);

include/linux/kprobes.h extern void arch_prepare_kretprobe(struct kretprobe_instance *ri, · line 208

Quoted 10 lines

include/linux/kprobes.h +9 -0

 
+void kretprobe_trampoline(void);
+/*
+ * Since some architecture uses structured function pointer,
+ * use dereference_function_descriptor() to get real function address.
+ */
+static nokprobe_inline void *kretprobe_trampoline_addr(void)
+{
+	return dereference_kernel_function_descriptor(kretprobe_trampoline);
+}

Could we please also make 'kretprobe_trampoline' harder to use accidentally, such by naming it appropriately?

__kretprobe_trampoline would be a good first step I guess.

Remove trampoline_address from kretprobe_trampoline_handler(). Instead of passing the address, kretprobe_trampoline_handler() can use new kretprobe_trampoline_addr().

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Andrii Nakryik <[email protected]>

Add kretprobe_find_ret_addr() for searching correct return address from kretprobe instance list.

 include/linux/kprobes.h |   22 +++++++++++
 kernel/kprobes.c        |   91 ++++++++++++++++++++++++++++++++++-------------
 2 files changed, 87 insertions(+), 26 deletions(-)

include/linux/kprobes.h +0 -0

index 5ce677819a25..08d3415e4418 100644

include/linux/kprobes.h +4 -0

+unsigned long kretprobe_find_ret_addr(struct task_struct *tsk, void *fp,
+				      struct llist_node **cur)
+{
+	struct kretprobe_instance *ri = NULL;

Quoted 2 lines · Masami Hiramatsu

Add kretprobe_find_ret_addr() for searching correct return address from kretprobe instance list.

A better changelog:

Add kretprobe_find_ret_addr() for searching the correct return address from the kretprobe instances list.

But an explanation of *why* we want to add this function would be even better. Is it a cleanup? Is it in preparation for future changes?

Plus:

Quoted 9 lines

 include/linux/kprobes.h |   22 +++++++++++
 kernel/kprobes.c        |   91 ++++++++++++++++++++++++++++++++++-------------
 2 files changed, 87 insertions(+), 26 deletions(-)

include/linux/kprobes.h +0 -0

index 5ce677819a25..08d3415e4418 100644

include/linux/kprobes.h static nokprobe_inline void *kretprobe_trampoline_addr(void) · line 216

Quoted 10 lines

include/linux/kprobes.h +7 -0

@@ -207,6 +207,14 @@ static nokprobe_inline void *kretprobe_trampoline_addr(void)
 	return dereference_kernel_function_descriptor(kretprobe_trampoline);
 }
 
+static nokprobe_inline bool is_kretprobe_trampoline(unsigned long addr)
+{
+	return (void *)addr = kretprobe_trampoline_addr();
+}
+
+unsigned long kretprobe_find_ret_addr(struct task_struct *tsk, void *fp,
+				      struct llist_node **cur);

These prototypes for helpers are put into a section of:

  #ifdef CONFIG_KRETPROBES

But:

include/linux/kprobes.h static inline bool is_kprobe_optinsn_slot(unsigned long addr) · line 529

Quoted 13 lines

include/linux/kprobes.h +13 -0

+#if !defined(CONFIG_KRETPROBES)
+static nokprobe_inline bool is_kretprobe_trampoline(unsigned long addr)
+{
+	return false;
+}
+
+static nokprobe_inline
+unsigned long kretprobe_find_ret_addr(struct task_struct *tsk, void *fp,
+				      struct llist_node **cur)
+{
+	return 0;
+}
+#endif

Why does this use such a weird pattern? What is wrong with:

   #ifndef CONFIG_KRETPROBES

But more importantly, why isn't this in the regular '#else' block of the CONFIG_KRETPROBES block you added the other functions to ??

Why this intentional obfuscation combined with poor changelogs - is the kprobes code too easy to read, does it have too few bugs?

And this series is on v8 already, and nobody noticed this?

include/linux/kprobes.h

Quoted 3 lines

include/linux/kprobes.h +3 -0

+/* This assumes the tsk is current or the task which is not running. */
+static unsigned long __kretprobe_find_ret_addr(struct task_struct *tsk,
+					       struct llist_node **cur)

A better comment:

    /* This assumes 'tsk' is the current task, or is not running. */

We always escape variable names in English sentences. This is nothing new.

include/linux/kprobes.h

Quoted 3 lines

include/linux/kprobes.h +2 -0

+			*cur = node;
+			return (unsigned long)ri->ret_addr;

Don't just randomly add forced type casts (which are dangerous, bug-inducing patterns of code) without examining whether it's justified.

But a compiler warning is not justification!

In this case the examination would involve:

kepler:~/tip> git grep -w ret_addr kernel/kprobes.c

  kernel/kprobes.c:               if (ri->ret_addr != kretprobe_trampoline_addr()) {
  kernel/kprobes.c:                       return (unsigned long)ri->ret_addr;
  kernel/kprobes.c:                       ri->ret_addr = correct_ret_addr;

kepler:~/tip> git grep -w correct_ret_addr kernel/kprobes.c

  kernel/kprobes.c:       kprobe_opcode_t *correct_ret_addr = NULL;
  kernel/kprobes.c:       correct_ret_addr = (void *)__kretprobe_find_ret_addr(current, &node);
  kernel/kprobes.c:       if (!correct_ret_addr) {

kernel/kprobes.c: ri->ret_addr = correct_ret_addr;
kernel/kprobes.c: return (unsigned long)correct_ret_addr;

what we can see here is unnecessary type confusion & friction of the first degree:

- 'correct_ret_addr' is 'kprobe_opcode_t *' (which is good), but the newly introduced __kretprobe_find_ret_addr() function doesn't return such a type - why?

- struct_kretprobe_instance::ret_address has a 'kprobe_opcode_t *' type as well - which is good.

- kretprobe_find_ret_addr() uses 'unsigned long', but it returns the value to __kretprobe_trampoline_handler(), which does *another* forced type cast:

+ correct_ret_addr = (void *)__kretprobe_find_ret_addr(current, &node);

So we have the following type conversions:

kprobe_opcode_t * => unsigned long => unsigned long => kprobe_opcode_t *

Is there a technical reason why we cannot just use 'kprobe_opcode_t *'.

All other type casts in the kprobes code should be reviewed as well.

include/linux/kprobes.h

Quoted 2 lines

include/linux/kprobes.h +1 -1

-	BUG_ON(1);
+	return 0;

And in the proper, intact type propagation model this would become 'return NULL' - which is *far* more obviously a 'not found' condition than a random zero that might mean anything...

Quoted 11 lines

include/linux/kprobes.h +10 -0

+unsigned long kretprobe_find_ret_addr(struct task_struct *tsk, void *fp,
+				      struct llist_node **cur)
+{
+	struct kretprobe_instance *ri = NULL;
+	unsigned long ret;
+
+	do {
+		ret = __kretprobe_find_ret_addr(tsk, cur);
+		if (!ret)
+			return ret;

include/linux/kprobes.h

Quoted 4 lines

include/linux/kprobes.h +4 -0

+		ri = container_of(*cur, struct kretprobe_instance, llist);
+	} while (ri->fp != fp);
+
+	return ret;

Here I see another type model problem: why is the frame pointer 'void *', which makes it way too easy to mix up with text pointers such as 'kprobe_opcode_t *'?

In the x86 unwinder we use 'unsigned long *' as the frame pointer:

unsigned long *bp

but it might also make sense to introduce a more opaque dedicated type within the kprobes code, such as 'frame_pointer_t'.

include/linux/kprobes.h

Quoted 12 lines

include/linux/kprobes.h +12 -0

+unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
+					     void *frame_pointer)
+{
+	kprobe_opcode_t *correct_ret_addr = NULL;
+	struct kretprobe_instance *ri = NULL;
+	struct llist_node *first, *node = NULL;
+	struct kretprobe *rp;
+
+	/* Find correct address and all nodes for this frame. */
+	correct_ret_addr = (void *)__kretprobe_find_ret_addr(current, &node);
+	if (!correct_ret_addr) {
+		pr_err("Oops! Kretprobe fails to find correct return address.\n");

Could we please make user-facing messages less random? Right now we have:

  kepler:~/tip> git grep -E 'pr_.*\(' kernel/kprobes.c include/linux/kprobes.h include/asm-generic/kprobes.h $(find arch/ -name kprobes.c)

  arch/arm64/kernel/probes/kprobes.c:             pr_warn("Unrecoverable kprobe detected.\n");
  arch/csky/kernel/probes/kprobes.c:              pr_warn("Address not aligned.\n");
  arch/csky/kernel/probes/kprobes.c:              pr_warn("Unrecoverable kprobe detected.\n");
  arch/mips/kernel/kprobes.c:             pr_notice("Kprobes for ll and sc instructions are not"
  arch/mips/kernel/kprobes.c:             pr_notice("Kprobes for branch delayslot are not supported\n");
  arch/mips/kernel/kprobes.c:             pr_notice("Kprobes for compact branches are not supported\n");
  arch/mips/kernel/kprobes.c:     pr_notice("%s: unaligned epc - sending SIGBUS.\n", current->comm);
  arch/mips/kernel/kprobes.c:                     pr_notice("Kprobes: Error in evaluating branch\n");
  arch/riscv/kernel/probes/kprobes.c:             pr_warn("Address not aligned.\n");
  arch/riscv/kernel/probes/kprobes.c:             pr_warn("Unrecoverable kprobe detected.\n");
  arch/s390/kernel/kprobes.c:             pr_err("Invalid kprobe detected.\n");
  kernel/kprobes.c:               pr_debug("Failed to arm kprobe-ftrace at %pS (%d)\n",
  kernel/kprobes.c:                       pr_debug("Failed to init kprobe-ftrace (%d)\n", ret);
  kernel/kprobes.c:               pr_err("Oops! Kretprobe fails to find correct return address.\n");
  kernel/kprobes.c:       pr_err("Dumping kprobe:\n");
  kernel/kprobes.c:       pr_err("Name: %s\nOffset: %x\nAddress: %pS\n",
  kernel/kprobes.c:               pr_err("kprobes: failed to populate blacklist: %d\n", err);
  kernel/kprobes.c:               pr_err("Please take care of using kprobes.\n");
  kernel/kprobes.c:               pr_warn("Kprobes globally enabled, but failed to arm %d out of %d probes\n",
  kernel/kprobes.c:               pr_info("Kprobes globally enabled\n");
  kernel/kprobes.c:               pr_warn("Kprobes globally disabled, but failed to disarm %d out of %d probes\n",
  kernel/kprobes.c:               pr_info("Kprobes globally disabled\n");

In particular, what users may see in their syslog, when the kprobes code runs into trouble, is, roughly:

  kepler:~/tip> git grep -E 'pr_.*\(' kernel/kprobes.c include/linux/kprobes.h include/asm-generic/kprobes.h $(find arch/ -name kprobes.c) | cut -d\" -f2

Unrecoverable kprobe detected.\n
Address not aligned.\n
Unrecoverable kprobe detected.\n
Kprobes for ll and sc instructions are not
Kprobes for branch delayslot are not supported\n
Kprobes for compact branches are not supported\n
%s: unaligned epc - sending SIGBUS.\n
Kprobes: Error in evaluating branch\n
Address not aligned.\n
Unrecoverable kprobe detected.\n
Invalid kprobe detected.\n
Failed to arm kprobe-ftrace at %pS (%d)\n
Failed to init kprobe-ftrace (%d)\n
Oops! Kretprobe fails to find correct return address.\n Dumping kprobe:\n
Name: %s\nOffset: %x\nAddress: %pS\n
kprobes: failed to populate blacklist: %d\n
Please take care of using kprobes.\n
Kprobes globally enabled, but failed to arm %d out of %d probes\n Kprobes globally enabled\n
Kprobes globally disabled, but failed to disarm %d out of %d probes\n Kprobes globally disabled\n

Ugh. Some of the messages don't even have 'kprobes' in them...

So my suggestion would be:

- Introduce a subsystem syslog message prefix, via the standard pattern of:

     #define pr_fmt(fmt) "kprobes: " fmt

- Standardize the messages:

- Start each message with a key noun that stresses the nature of the failure.

- *Make each message self-explanatory*, don't leave users hanging in the air about what is going to happen next. Messages like:

Address not aligned.\n

- Check spelling. This:

pr_err("kprobes: failed to populate blacklist: %d\n", err); pr_err("Please take care of using kprobes.\n");

should be on a single line and should probably say something like:

pr_err("kprobes: Failed to populate blacklist (error: %d), kprobes not restricted, be careful using them!.\n", err);

and if checkpatch complains that the line is 'too long', ignore checkpatch and keep the message self-contained.

- and most importantly: provide a suggested *resolution*. Passive-aggressive messages like:

Oops! Kretprobe fails to find correct return address.\n

are next to useless. Instead, always describe:

- what happened,
- what is the kernel going to do or not do,
- is the kernel fine,
- what can the user do about it.

In this case, a better message would be:

kretprobes: Return address not found, not executing handler. Kernel is probably fine, but check the system tool that did this.

Each and every message should be reviewed & fixed to meet these standards - or should be removed and replaced with a WARN_ON() if it's indicating an internal bug that cannot be caused by kprobes using tools, such as this one:

include/linux/kprobes.h

Quoted 2 lines

include/linux/kprobes.h +2 -0

+		if (WARN_ON_ONCE(ri->fp != frame_pointer))
+			break;

I can help double checking the fixed messages, if you are unsure about any of them.

include/linux/kprobes.h

Quoted 8 lines

include/linux/kprobes.h +4 -0

+	/* Recycle them.  */
+	while (first) {
+		ri = container_of(first, struct kretprobe_instance, llist);
+		first = first->next;
 
 		recycle_rp_inst(ri);
 	}

It would be helpful to explain, a bit more verbose comment, what 'recycling' is in this context. The code is not very helpful:

  NOKPROBE_SYMBOL(free_rp_inst_rcu);

  static void recycle_rp_inst(struct kretprobe_instance *ri)
  {
          struct kretprobe *rp = get_kretprobe(ri);

          if (likely(rp)) {
                  freelist_add(&ri->freelist, &rp->freelist);
          } else
                  call_rcu(&ri->rcu, free_rp_inst_rcu);

}
NOKPROBE_SYMBOL(recycle_rp_inst);

BTW., why are unnecessary curly braces used here?

The kprobes code urgently needs a quality boost.

Thanks,

Ingo

From: Josh Poimboeuf <[email protected]>

Add UNWIND_HINT_FUNC on kretporbe_trampoline code so that ORC information is generated on the kretprobe_trampoline correctly.

Note that when the CONFIG_FRAME_POINTER=y, since the kretprobe_trampoline skips updating frame pointer, the stack frame of the kretprobe_trampoline seems non-standard. So this marks it is STACK_FRAME_NON_STANDARD() and undefine UNWIND_HINT_FUNC.

Anyway, with the frame pointer, FP unwinder can unwind the stack frame correctly without that hint.

Signed-off-by Josh Poimboeuf <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Andrii Nakryik <[email protected]>

arch/x86/kernel/kprobes/core.c +5 -0

+STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
+#undef UNWIND_HINT_FUNC
+#define UNWIND_HINT_FUNC
+#endif
 /*
  * When a retprobed function returns, this code saves registers and
  * calls trampoline_handler() runs, which calls the kretprobe's handler.

@@ -1031,6 +1044,7 @@ asm(
 	/* We don't bother saving the ss register */
 #ifdef CONFIG_X86_64
 	"	pushq %rsp\n"
+	UNWIND_HINT_FUNC
 	"	pushfq\n"
 	SAVE_REGS_STRING
 	"	movq %rsp, %rdi\n"

Quoted 4 lines · Masami Hiramatsu

From: Josh Poimboeuf <[email protected]>

Add UNWIND_HINT_FUNC on kretporbe_trampoline code so that ORC information is generated on the kretprobe_trampoline correctly.

What is a 'kretporbe'?

Quoted 5 lines

Note that when the CONFIG_FRAME_POINTER=y, since the kretprobe_trampoline skips updating frame pointer, the stack frame of the kretprobe_trampoline seems non-standard. So this marks it is STACK_FRAME_NON_STANDARD() and undefine UNWIND_HINT_FUNC.

What does 'marks it is' mean?

'undefine' UNWIND_HINT_FUNC?

Doesn't the patch do the exact opposite:

arch/x86/include/asm/unwind_hints.h line 58

Quoted 2 lines

arch/x86/include/asm/unwind_hints.h +2 -0

+#define UNWIND_HINT_FUNC \
+	UNWIND_HINT(ORC_REG_SP, 8, UNWIND_HINT_TYPE_FUNC, 0)

But it does undefine it in a specific spot:

Quoted 6 lines

Anyway, with the frame pointer, FP unwinder can unwind the stack frame correctly without that hint.

Signed-off-by Josh Poimboeuf <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Andrii Nakryik <[email protected]>

I have to say these changelogs are very careless.

Review

Quoted 3 lines

(quoted diff) +2 -0

+#else
+

In headers, in longer CPP blocks, please always mark the '#else' branch with what it is the else branch of.

See the output of:

kepler:~/tip> git grep '#else' arch/x86/include/asm/ | head

arch/x86/kernel/kprobes/core.c int kprobe_int3_handler(struct pt_regs *regs) · line 1029

Quoted 9 lines

arch/x86/kernel/kprobes/core.c +8 -0

+#ifdef CONFIG_FRAME_POINTER
+/*
+ * kretprobe_trampoline skips updating frame pointer. The frame pointer
+ * saved in trampoline_handler points to the real caller function's
+ * frame pointer. Thus the kretprobe_trampoline doesn't seems to have a
+ * standard stack frame with CONFIG_FRAME_POINTER=y.
+ * Let's mark it non-standard function. Anyway, FP unwinder can correctly
+ * unwind without the hint.

s/doesn't seems to have a standard stack frame
/doesn't have a standard stack frame

There's nothing 'seems' about the situation - it's a non-standard function entry and stack frame situation, and the unwinder needs to know about it.

Quoted 19 lines

arch/x86/kernel/kprobes/core.c +5 -0

+STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
+#undef UNWIND_HINT_FUNC
+#define UNWIND_HINT_FUNC
+#endif
 /*
  * When a retprobed function returns, this code saves registers and
  * calls trampoline_handler() runs, which calls the kretprobe's handler.

@@ -1031,6 +1044,7 @@ asm(
 	/* We don't bother saving the ss register */
 #ifdef CONFIG_X86_64
 	"	pushq %rsp\n"
+	UNWIND_HINT_FUNC
 	"	pushfq\n"
 	SAVE_REGS_STRING
 	"	movq %rsp, %rdi\n"

@@ -1041,6 +1055,7 @@ asm(
 	"	popfq\n"
 #else

arch/x86/kernel/kprobes/core.c asm( · line 1061

Quoted 5 lines

arch/x86/kernel/kprobes/core.c +1 -0

 	"	pushl %esp\n"
+	UNWIND_HINT_FUNC
 	"	pushfl\n"
 	SAVE_REGS_STRING
 	"	movl %esp, %eax\n"

Why not provide an appropriate annotation method in <asm/unwind_hints.h>, so that other future code can use it too instead of reinventing the wheel?

Quoted 18 lines

Quoted 18 lines

arch/x86/kernel/kprobes/core.c +5 -0

+STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
+#undef UNWIND_HINT_FUNC
+#define UNWIND_HINT_FUNC
+#endif
 /*
  * When a retprobed function returns, this code saves registers and
  * calls trampoline_handler() runs, which calls the kretprobe's handler.

@@ -1031,6 +1044,7 @@ asm(
 	/* We don't bother saving the ss register */
 #ifdef CONFIG_X86_64
 	"	pushq %rsp\n"
+	UNWIND_HINT_FUNC
 	"	pushfq\n"
 	SAVE_REGS_STRING
 	"	movq %rsp, %rdi\n"

@@ -1041,6 +1055,7 @@ asm(
 	"	popfq\n"
 #else

Quoted 8 lines

arch/x86/kernel/kprobes/core.c asm( · line 1061

Quoted 5 lines

arch/x86/kernel/kprobes/core.c +1 -0

 	"	pushl %esp\n"
+	UNWIND_HINT_FUNC
 	"	pushfl\n"
 	SAVE_REGS_STRING
 	"	movl %esp, %eax\n"

Why not provide an appropriate annotation method in <asm/unwind_hints.h>, so that other future code can use it too instead of reinventing the wheel?

Hi Ingo and Josh,

Quoted 18 lines · On Sat, 10 Jul 2021 00:31:40 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 18 lines

Quoted 18 lines

arch/x86/kernel/kprobes/core.c +5 -0

+STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
+#undef UNWIND_HINT_FUNC
+#define UNWIND_HINT_FUNC
+#endif
 /*
  * When a retprobed function returns, this code saves registers and
  * calls trampoline_handler() runs, which calls the kretprobe's handler.

@@ -1031,6 +1044,7 @@ asm(
 	/* We don't bother saving the ss register */
 #ifdef CONFIG_X86_64
 	"	pushq %rsp\n"
+	UNWIND_HINT_FUNC
 	"	pushfq\n"
 	SAVE_REGS_STRING
 	"	movq %rsp, %rdi\n"

@@ -1041,6 +1055,7 @@ asm(
 	"	popfq\n"
 #else

Quoted 8 lines

Quoted 8 lines

arch/x86/kernel/kprobes/core.c asm( · line 1061

Quoted 5 lines

arch/x86/kernel/kprobes/core.c +1 -0

 	"	pushl %esp\n"
+	UNWIND_HINT_FUNC
 	"	pushfl\n"
 	SAVE_REGS_STRING
 	"	movl %esp, %eax\n"

Why not provide an appropriate annotation method in <asm/unwind_hints.h>, so that other future code can use it too instead of reinventing the wheel?

I think I got what you meant. Let me summarize the issue.

In case of CONFIG_FRAME_POINTER=n, it is OK just adding UNWIND_HINT_FUNC.

In case of CONFIG_FRAME_POINTER=y, without STACK_FRAME_NON_STANDARD(),
the objtool complains that a CALL instruction without the frame pointer.
---
arch/x86/kernel/kprobes/core.o: warning: objtool: __kretprobe_trampoline()+0x25: call without frame pointer save/setup
---

If we just add STACK_FRAME_NON_STANDARD() with UNWIND_HINT_FUNC macro,
the objtool complains that non-standard function has unwind hint.
---
arch/x86/kernel/kprobes/core.o: warning: objtool: __kretprobe_trampoline()+0x1: BUG: why am I validating an ignored function?

Add a CONFIG_FRAME_POINTER-specific version of STACK_FRAME_NON_STANDARD() for the case where a function is intentionally missing frame pointer setup, but otherwise needs objtool/ORC coverage when frame pointers are disabled.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 include/linux/objtool.h       | 11 +++++++++++
 tools/include/linux/objtool.h | 11 +++++++++++
 2 files changed, 22 insertions(+)

include/linux/objtool.h +11 -0

index 7e72d975cb76..c9575ed91052 100644
@@ -66,6 +66,17 @@ struct unwind_hint {
 	static void __used __section(".discard.func_stack_frame_non_standard") \
 		*__func_stack_frame_non_standard_##func = func
 
+/*
+ * STACK_FRAME_NON_STANDARD_FP() is a frame-pointer-specific function ignore
+ * for the case where a function is intentionally missing frame pointer setup,
+ * but otherwise needs objtool/ORC coverage when frame pointers are disabled.
+ */
+#ifdef CONFIG_FRAME_POINTER
+#define STACK_FRAME_NON_STANDARD_FP(func) STACK_FRAME_NON_STANDARD(func)
+#else
+#define STACK_FRAME_NON_STANDARD_FP(func)
+#endif
+
 #else /* __ASSEMBLY__ */
 
 /*

tools/include/linux/objtool.h +11 -0

index 7e72d975cb76..c9575ed91052 100644
@@ -66,6 +66,17 @@ struct unwind_hint {
 	static void __used __section(".discard.func_stack_frame_non_standard") \
 		*__func_stack_frame_non_standard_##func = func
 
+/*
+ * STACK_FRAME_NON_STANDARD_FP() is a frame-pointer-specific function ignore
+ * for the case where a function is intentionally missing frame pointer setup,
+ * but otherwise needs objtool/ORC coverage when frame pointers are disabled.
+ */
+#ifdef CONFIG_FRAME_POINTER
+#define STACK_FRAME_NON_STANDARD_FP(func) STACK_FRAME_NON_STANDARD(func)
+#else
+#define STACK_FRAME_NON_STANDARD_FP(func)
+#endif
+
 #else /* __ASSEMBLY__ */
 
 /*

Signature · 1 line

2.31.1

Add a CONFIG_FRAME_POINTER-specific version of STACK_FRAME_NON_STANDARD() for the case where a function is intentionally missing frame pointer setup, but otherwise needs objtool/ORC coverage when frame pointers are disabled.

Signed-off-by Josh Poimboeuf <[email protected]>

---
 include/linux/objtool.h       | 11 +++++++++++
 tools/include/linux/objtool.h | 11 +++++++++++
 2 files changed, 22 insertions(+)

include/linux/objtool.h +11 -0

index 7e72d975cb76..c9575ed91052 100644
@@ -66,6 +66,17 @@ struct unwind_hint {
 	static void __used __section(".discard.func_stack_frame_non_standard") \
 		*__func_stack_frame_non_standard_##func = func
 
+/*
+ * STACK_FRAME_NON_STANDARD_FP() is a frame-pointer-specific function ignore
+ * for the case where a function is intentionally missing frame pointer setup,
+ * but otherwise needs objtool/ORC coverage when frame pointers are disabled.
+ */
+#ifdef CONFIG_FRAME_POINTER
+#define STACK_FRAME_NON_STANDARD_FP(func) STACK_FRAME_NON_STANDARD(func)
+#else
+#define STACK_FRAME_NON_STANDARD_FP(func)
+#endif
+
 #else /* __ASSEMBLY__ */
 
 /*

tools/include/linux/objtool.h +11 -0

index 7e72d975cb76..c9575ed91052 100644
@@ -66,6 +66,17 @@ struct unwind_hint {
 	static void __used __section(".discard.func_stack_frame_non_standard") \
 		*__func_stack_frame_non_standard_##func = func
 
+/*
+ * STACK_FRAME_NON_STANDARD_FP() is a frame-pointer-specific function ignore
+ * for the case where a function is intentionally missing frame pointer setup,
+ * but otherwise needs objtool/ORC coverage when frame pointers are disabled.
+ */
+#ifdef CONFIG_FRAME_POINTER
+#define STACK_FRAME_NON_STANDARD_FP(func) STACK_FRAME_NON_STANDARD(func)
+#else
+#define STACK_FRAME_NON_STANDARD_FP(func)
+#endif
+
 #else /* __ASSEMBLY__ */
 
 /*

Signature · 1 line

2.31.1

If a function is ignored, also ignore its hints. This is useful for the case where the function ignore is conditional on frame pointers, e.g. STACK_FRAME_NON_STANDARD_FP().

Signed-off-by Josh Poimboeuf <[email protected]>

---
 tools/objtool/check.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/objtool/check.c +1 -1

index e5947fbb9e7a..67cbdcfcabae 100644
@@ -2909,7 +2909,7 @@ static int validate_unwind_hints(struct objtool_file *file, struct section *sec)
 	}
 
 	while (&insn->list != &file->insn_list && (!sec || insn->sec = sec)) {
-		if (insn->hint && !insn->visited) {
+		if (insn->hint && !insn->visited && !insn->ignore) {
 			ret = validate_branch(file, insn->func, insn, state);
 			if (ret && backtrace)
 				BT_FUNC("<== (hint)", insn);

Signature · 1 line

2.31.1

Change kretprobe_trampoline to make a space for regs->ARM_pc so that kretprobe_trampoline_handler can call instruction_pointer_set() safely.

Quoted 3 lines · Masami Hiramatsu

Change kretprobe_trampoline to make a space for regs->ARM_pc so that kretprobe_trampoline_handler can call instruction_pointer_set() safely.

The idiom is "make space", but in any case, what does this mean?

Was the stack frame set up in kretprobe_trampoline() and calling trampoline_handler() buggy?

If yes, then explain the bad effects of the bug, and make all of this clear in the title & changelog.

Thanks,

Ingo

Since the ORC unwinder from pt_regs requires setting up regs->ip correctly, set the correct return address to the regs->ip before calling user kretprobe handler.

This allows the kretrprobe handler to trace stack from the kretprobe's pt_regs by stack_trace_save_regs() (eBPF will do this), instead of stack tracing from the handler context by stack_trace_save() (ftrace will do this).

Suggested-by Josh Poimboeuf <[email protected]>

Signed-off-by Masami Hiramatsu <[email protected]>

Tested-by Andrii Nakryik <[email protected]>

---
Changes in v8:
- Update comment to clarify why this is needed.
Changes in v3:
- Cast the correct_ret_addr to unsigned long.

---
 kernel/kprobes.c |    3 +++
 1 file changed, 3 insertions(+)

kernel/kprobes.c +3 -0

index 650cbe738666..ba729ed05cb3 100644
@@ -1896,6 +1896,9 @@ unsigned long __kretprobe_trampoline_handler(struct pt_regs *regs,
 		BUG_ON(1);
 	}
 
+	/* Set the instruction pointer to the correct address */
+	instruction_pointer_set(regs, (unsigned long)correct_ret_addr);
+
 	/* Run them. */
 	first = current->kretprobe_instances.first;
 	while (first) {

arch/x86/kernel/kprobes/core.c __used __visible void *trampoline_handler(struct pt_regs *regs) · line 1097

Quoted 5 lines · Masami Hiramatsu

arch/x86/kernel/kprobes/core.c +5 -0

+	/* Replace fake return address with real one. */
+	*frame_pointer = kretprobe_trampoline_handler(regs, frame_pointer);
+	/*
+	 * Move flags to sp so that kretprobe_trapmoline can return
+	 * right after popf.

What is a trapmoline?

Also, in the x86 code we capitalize register and instruction names so that they are more distinctive and easier to read in the flow of English text.

Thanks,

Ingo

arch/x86/kernel/unwind_orc.c +9 -1

@@ -549,7 +548,15 @@ bool unwind_next_frame(struct unwind_state *state)
 					 (void *)orig_ip);
 			goto err;
 		}
-
+		/*
+		 * There is a small chance to interrupt at the entry of
+		 * kretprobe_trampoline where the ORC info doesn't exist.
+		 * That point is right after the RET to kretprobe_trampoline
+		 * which was modified return address. So the @addr_p must
+		 * be right before the regs->sp.
+		 */
+		state->ip = unwind_recover_kretprobe(state, state->ip,
+				(unsigned long *)(state->sp - sizeof(long)));
 		state->regs = (struct pt_regs *)sp;
 		state->prev_regs = NULL;
 		state->full_regs = true;

Quoted 25 lines · Masami Hiramatsu

arch/x86/kernel/unwind_orc.c +12 -1

@@ -549,7 +548,15 @@ bool unwind_next_frame(struct unwind_state *state)
 					 (void *)orig_ip);
 			goto err;
 		}
-
+		/*
+		 * There is a small chance to interrupt at the entry of
+		 * kretprobe_trampoline where the ORC info doesn't exist.
+		 * That point is right after the RET to kretprobe_trampoline
+		 * which was modified return address. So the @addr_p must
+		 * be right before the regs->sp.
+		 */
+		state->ip = unwind_recover_kretprobe(state, state->ip,
+				(unsigned long *)(state->sp - sizeof(long)));
 		state->regs = (struct pt_regs *)sp;
 		state->prev_regs = NULL;
 		state->full_regs = true;

@@ -562,6 +569,9 @@ bool unwind_next_frame(struct unwind_state *state)
 					 (void *)orig_ip);
 			goto err;
 		}
+		/* See UNWIND_HINT_TYPE_REGS case comment. */
+		state->ip = unwind_recover_kretprobe(state, state->ip,
+				(unsigned long *)(state->sp - sizeof(long)));
 

arch/x86/kernel/unwind_orc.c bool unwind_next_frame(struct unwind_state *state) · line 577

Quoted 2 lines

arch/x86/kernel/unwind_orc.c +0 -0

 		if (state->full_regs)
 			state->prev_regs = state->regs;

Why doesn't the ftrace case have this? That is, why aren't both return trampolines having the same general shape?

Quoted 26 lines · On Mon, 5 Jul 2021 13:36:14 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 25 lines · Masami Hiramatsu

arch/x86/kernel/unwind_orc.c +12 -1

@@ -549,7 +548,15 @@ bool unwind_next_frame(struct unwind_state *state)
 					 (void *)orig_ip);
 			goto err;
 		}
-
+		/*
+		 * There is a small chance to interrupt at the entry of
+		 * kretprobe_trampoline where the ORC info doesn't exist.
+		 * That point is right after the RET to kretprobe_trampoline
+		 * which was modified return address. So the @addr_p must
+		 * be right before the regs->sp.
+		 */
+		state->ip = unwind_recover_kretprobe(state, state->ip,
+				(unsigned long *)(state->sp - sizeof(long)));
 		state->regs = (struct pt_regs *)sp;
 		state->prev_regs = NULL;
 		state->full_regs = true;

@@ -562,6 +569,9 @@ bool unwind_next_frame(struct unwind_state *state)
 					 (void *)orig_ip);
 			goto err;
 		}
+		/* See UNWIND_HINT_TYPE_REGS case comment. */
+		state->ip = unwind_recover_kretprobe(state, state->ip,
+				(unsigned long *)(state->sp - sizeof(long)));
 

Quoted 5 lines

arch/x86/kernel/unwind_orc.c bool unwind_next_frame(struct unwind_state *state) · line 577

Quoted 2 lines

arch/x86/kernel/unwind_orc.c +0 -0

 		if (state->full_regs)
 			state->prev_regs = state->regs;

Why doesn't the ftrace case have this? That is, why aren't both return trampolines having the same general shape?

Ah, this strongly depends what the trampoline code does.
For the kretprobe case, the PUSHQ at the entry of the kretprobe_trampoline() does not covered by UNWIND_HINT_FUNC. Thus it needs to find 'correct_ret_addr' by the frame pointer (which is next to the sp).

"kretprobe_trampoline:\n"
#ifdef CONFIG_X86_64
/* Push fake return address to tell the unwinder it's a kretprobe */
" pushq $kretprobe_trampoline\n"
UNWIND_HINT_FUNC

But I'm not so sure how ftrace treat it. It seems that the return_to_handler() doesn't care such case. (anyway, return_to_handler() does not return but jump to the original call-site, in that case, the information will be lost.)

Quoted 3 lines

But I'm not so sure how ftrace treat it. It seems that the return_to_handler() doesn't care such case. (anyway, return_to_handler() does not return but jump to the original call-site, in that case, the information will be lost.)

I find it bothersome (OCD, sorry :-) that both return trampolines behave differently. Doubly so because I know people (Steve in particular) have been talking about unifying them.

Steve, can you clarify the ftrace side here? Afaict return_to_handler() is similarly affected.

Quoted 7 lines · On Tue, 6 Jul 2021 09:55:03 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 3 lines

But I'm not so sure how ftrace treat it. It seems that the return_to_handler() doesn't care such case. (anyway, return_to_handler() does not return but jump to the original call-site, in that case, the information will be lost.)

I find it bothersome (OCD, sorry :-) that both return trampolines behave differently. Doubly so because I know people (Steve in particular) have been talking about unifying them.

They were developed separately, and designed differently with different goals in mind. Yes, I want to unify them, but trying to get the different goals together, compounded by the fact that almost every arch also implemented them differently (in which case, we need to find a way to do it one arch at a time), makes the process extremely frustrating.

Quoted 2 lines

Steve, can you clarify the ftrace side here? Afaict return_to_handler() is similarly affected.

I'm not exactly sure what the issue is. As Masami stated, kretprobe uses a ret to return to the calling function, but ftrace uses a jmp.

kretprobe return tracing is more complex than the function graph return tracing is (which is one of the issues I need to overcome to unify them),

and when the function graph return trampoline was created, it did things as simple as possible (and before ORC existed).

Is this something to worry about now, or should we look to fix his in the unifying process?

Quoted 5 lines

Quoted 2 lines

Steve, can you clarify the ftrace side here? Afaict return_to_handler() is similarly affected.

I'm not exactly sure what the issue is. As Masami stated, kretprobe uses a ret to return to the calling function, but ftrace uses a jmp.

I'll have to re-read the ftrace bits, but from the top of my head you cannot do an indirect jump and preserve all registers at the same time, so a return stub must use jump from stack aka. ret.

Quoted 5 lines

Quoted 2 lines

Steve, can you clarify the ftrace side here? Afaict return_to_handler() is similarly affected.

I'm not exactly sure what the issue is. As Masami stated, kretprobe uses a ret to return to the calling function, but ftrace uses a jmp.

I'll have to re-read the ftrace bits, but from the top of my head you cannot do an indirect jump and preserve all registers at the same time, so a return stub must use jump from stack aka. ret.

Quoted 3 lines

kretprobe return tracing is more complex than the function graph return tracing is (which is one of the issues I need to overcome to unify them),

I'm not sure it is. IIRC the biggest pain point with kretprobe is that 'silly' property that the kretprobe_instance are not the same between kretprobes. Luckily, that's not actually used anywhere, so we can simply rip that out.

I actually don't want to keep this feature because no one use it. (only systemtap needs it?)

Anyway, if we keep the idea-level compatibility (not code level), what we need is 'void *data' in the struct kretprobe_instance. User who needs it can allocate their own instance data for their kretprobes when initialising it and sets in their entry handler.

Then we can have a simple kretprobe_instance.

Quoted 2 lines · Masami Hiramatsu

I actually don't want to keep this feature because no one use it. (only systemtap needs it?)

Yeah, you mentioned systemtap, but since that's out-of-tree I don't care. Their problem.

Quoted 5 lines

Anyway, if we keep the idea-level compatibility (not code level), what we need is 'void *data' in the struct kretprobe_instance. User who needs it can allocate their own instance data for their kretprobes when initialising it and sets in their entry handler.

Quoted 1 line

Then we can have a simple kretprobe_instance.

When would you do the alloc? When installing the retprobe, but that might be inside the allocator, which means you can't call the allocator etc.. :-)

If we look at struct ftrace_ret_stack, it has a few fixed function fields. The calltime one is all that is needed for the kretprobe example code.

Quoted 7 lines · On Wed, 7 Jul 2021 12:20:57 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 2 lines · Masami Hiramatsu

I actually don't want to keep this feature because no one use it. (only systemtap needs it?)

Yeah, you mentioned systemtap, but since that's out-of-tree I don't care. Their problem.

Quoted 11 lines

Quoted 7 lines

Anyway, if we keep the idea-level compatibility (not code level), what we need is 'void *data' in the struct kretprobe_instance. User who needs it can allocate their own instance data for their kretprobes when initialising it and sets in their entry handler.

Then we can have a simple kretprobe_instance.

When would you do the alloc? When installing the retprobe, but that might be inside the allocator, which means you can't call the allocator etc.. :-)

Yes, so the user may need to allocate a pool right before register_kretprobe(). (whether per-kretprobe or per-task or global pool, that is user's choice.)

Quoted 1 line

Quoted 3 lines

If we look at struct ftrace_ret_stack, it has a few fixed function fields. The calltime one is all that is needed for the kretprobe example code.

kretprobe consumes 3 fields, a pointer to 'struct kretprobe' (which stores callee function address in 'kretprobe::kp.addr'), a return address and a frame pointer (*).

Quoted 5 lines · On Wed, 7 Jul 2021 19:45:30 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 2 lines · On Wed, 7 Jul 2021 12:20:57 +0200 Peter Zijlstra <[email protected]> wrote:

On Wed, Jul 07, 2021 at 07:15:10PM +0900, Masami Hiramatsu wrote:

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

I actually don't want to keep this feature because no one use it. (only systemtap needs it?)

Yeah, you mentioned systemtap, but since that's out-of-tree I don't care. Their problem.

Yeah, maybe it is not hard to update.

Quoted 24 lines

Quoted 11 lines

Quoted 7 lines

Anyway, if we keep the idea-level compatibility (not code level), what we need is 'void *data' in the struct kretprobe_instance. User who needs it can allocate their own instance data for their kretprobes when initialising it and sets in their entry handler.

Then we can have a simple kretprobe_instance.

When would you do the alloc? When installing the retprobe, but that might be inside the allocator, which means you can't call the allocator etc.. :-)

Yes, so the user may need to allocate a pool right before register_kretprobe(). (whether per-kretprobe or per-task or global pool, that is user's choice.)

Quoted 4 lines

If we look at struct ftrace_ret_stack, it has a few fixed function fields. The calltime one is all that is needed for the kretprobe example code.

kretprobe consumes 3 fields, a pointer to 'struct kretprobe' (which stores callee function address in 'kretprobe::kp.addr'), a return address and a frame pointer (*).
Oops, I forgot to add "void *data" for storing user data.

Quoted 36 lines · Masami Hiramatsu

Quoted 10 lines · On Wed, 7 Jul 2021 19:45:30 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 7 lines · On Wed, 7 Jul 2021 12:20:57 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 2 lines · Masami Hiramatsu

I actually don't want to keep this feature because no one use it. (only systemtap needs it?)

Yeah, you mentioned systemtap, but since that's out-of-tree I don't care. Their problem.

Yeah, maybe it is not hard to update.

Quoted 20 lines

Quoted 11 lines

Quoted 7 lines

Anyway, if we keep the idea-level compatibility (not code level), what we need is 'void *data' in the struct kretprobe_instance. User who needs it can allocate their own instance data for their kretprobes when initialising it and sets in their entry handler.

Then we can have a simple kretprobe_instance.

When would you do the alloc? When installing the retprobe, but that might be inside the allocator, which means you can't call the allocator etc.. :-)

Yes, so the user may need to allocate a pool right before register_kretprobe(). (whether per-kretprobe or per-task or global pool, that is user's choice.)

Quoted 4 lines

If we look at struct ftrace_ret_stack, it has a few fixed function fields. The calltime one is all that is needed for the kretprobe example code.

Quoted 5 lines

Quoted 4 lines

kretprobe consumes 3 fields, a pointer to 'struct kretprobe' (which stores callee function address in 'kretprobe::kp.addr'), a return address and a frame pointer (*).
Oops, I forgot to add "void *data" for storing user data.

Should use "struct kretprobe_holder *rph", since "struct kretprobe" belongs to 3rd-party module (which might be unloaded any time).

User's own pool might not work if the module can be unloaded. Better manage the pool in kretprobe_holder, which needs no changes from user side.

Quoted 37 lines · On Wed, 7 Jul 2021 22:42:47 +0800 Matt Wu <[email protected]> wrote:

Quoted 36 lines · Masami Hiramatsu

Quoted 10 lines · On Wed, 7 Jul 2021 19:45:30 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 7 lines · On Wed, 7 Jul 2021 12:20:57 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 2 lines · Masami Hiramatsu

I actually don't want to keep this feature because no one use it. (only systemtap needs it?)

Yeah, you mentioned systemtap, but since that's out-of-tree I don't care. Their problem.

Yeah, maybe it is not hard to update.

Quoted 20 lines

Quoted 11 lines

Quoted 7 lines

Anyway, if we keep the idea-level compatibility (not code level), what we need is 'void *data' in the struct kretprobe_instance. User who needs it can allocate their own instance data for their kretprobes when initialising it and sets in their entry handler.

Then we can have a simple kretprobe_instance.

When would you do the alloc? When installing the retprobe, but that might be inside the allocator, which means you can't call the allocator etc.. :-)

Yes, so the user may need to allocate a pool right before register_kretprobe(). (whether per-kretprobe or per-task or global pool, that is user's choice.)

Quoted 4 lines

If we look at struct ftrace_ret_stack, it has a few fixed function fields. The calltime one is all that is needed for the kretprobe example code.

Quoted 8 lines

Quoted 5 lines

Quoted 4 lines

kretprobe consumes 3 fields, a pointer to 'struct kretprobe' (which stores callee function address in 'kretprobe::kp.addr'), a return address and a frame pointer (*).
Oops, I forgot to add "void *data" for storing user data.

Should use "struct kretprobe_holder *rph", since "struct kretprobe" belongs to 3rd-party module (which might be unloaded any time).

Good catch. Yes, instead of 'struct kretprobe', we need to use the holder.

Quoted 2 lines

User's own pool might not work if the module can be unloaded. Better manage the pool in kretprobe_holder, which needs no changes from user side.

No, since the 'data' will be only refered from user handler. If the kretprobe is released, then the kretprobe_holder will clear the refernce to the 'struct kretprobe'. Then, the user handler is never called. No one access the 'data'.

Quoted 49 lines · Masami Hiramatsu

Quoted 37 lines · On Wed, 7 Jul 2021 22:42:47 +0800 Matt Wu <[email protected]> wrote:

Quoted 36 lines · Masami Hiramatsu

Quoted 10 lines · On Wed, 7 Jul 2021 19:45:30 +0900 Masami Hiramatsu <[email protected]> wrote:

Quoted 7 lines · On Wed, 7 Jul 2021 12:20:57 +0200 Peter Zijlstra <[email protected]> wrote:

Quoted 2 lines · Masami Hiramatsu

I actually don't want to keep this feature because no one use it. (only systemtap needs it?)

Yeah, you mentioned systemtap, but since that's out-of-tree I don't care. Their problem.

Yeah, maybe it is not hard to update.

Quoted 20 lines

Quoted 11 lines

Quoted 7 lines

Anyway, if we keep the idea-level compatibility (not code level), what we need is 'void *data' in the struct kretprobe_instance. User who needs it can allocate their own instance data for their kretprobes when initialising it and sets in their entry handler.

Then we can have a simple kretprobe_instance.

When would you do the alloc? When installing the retprobe, but that might be inside the allocator, which means you can't call the allocator etc.. :-)

Yes, so the user may need to allocate a pool right before register_kretprobe(). (whether per-kretprobe or per-task or global pool, that is user's choice.)

Quoted 4 lines

If we look at struct ftrace_ret_stack, it has a few fixed function fields. The calltime one is all that is needed for the kretprobe example code.

Quoted 8 lines

Quoted 5 lines

Quoted 4 lines

kretprobe consumes 3 fields, a pointer to 'struct kretprobe' (which stores callee function address in 'kretprobe::kp.addr'), a return address and a frame pointer (*).
Oops, I forgot to add "void *data" for storing user data.

Should use "struct kretprobe_holder *rph", since "struct kretprobe" belongs to 3rd-party module (which might be unloaded any time).

Quoted 8 lines

Good catch. Yes, instead of 'struct kretprobe', we need to use the holder.

Quoted 2 lines

User's own pool might not work if the module can be unloaded. Better manage the pool in kretprobe_holder, which needs no changes from user side.

No, since the 'data' will be only refered from user handler. If the kretprobe is released, then the kretprobe_holder will clear the refernce to the 'struct kretprobe'. Then, the user handler is never called. No one access the 'data'.

Indeed, there is no race of "data" accessing, since unregister_kretprobes() is taking care of it.

This implementation just increases the complexity of caller to keep track of all allocated instances and release them after unregistration.

But guys are likely to use kmalloc in pre-handler and kfree in post-handler, which will lead to memory leaks.

In x86, kretprobe trampoline address on the stack frame will be replaced with the real return address after returning from trampoline_handler. Before fixing the return address, the real return address can be found in the current->kretprobe_instances.

However, since there is a window between updating the current->kretprobe_instances and fixing the address on the stack, if an interrupt caused at that timing and the interrupt handler does stacktrace, it may fail to unwind because it can not get the correct return address from current->kretprobe_instances.

This will minimize that window by fixing the return address right before updating current->kretprobe_instances.

Quoted 11 lines · Masami Hiramatsu

In x86, kretprobe trampoline address on the stack frame will be replaced with the real return address after returning from trampoline_handler. Before fixing the return address, the real return address can be found in the current->kretprobe_instances.

However, since there is a window between updating the current->kretprobe_instances and fixing the address on the stack, if an interrupt caused at that timing and the interrupt handler does stacktrace, it may fail to unwind because it can not get the correct return address from current->kretprobe_instances.

Quoted 2 lines

This will minimize that window by fixing the return address right before updating current->kretprobe_instances.

Is there still a window? I.e. is it "minimized" (to how big of a window?), or eliminated?

arch/x86/kernel/kprobes/core.c asm( · line 1085

Quoted 10 lines

arch/x86/kernel/kprobes/core.c +9 -0

+void arch_kretprobe_fixup_return(struct pt_regs *regs,
+				 unsigned long correct_ret_addr)
+{
+	unsigned long *frame_pointer;
+
+	frame_pointer = ((unsigned long *)&regs->sp) + 1;
+
+	/* Replace fake return address with real one. */
+	*frame_pointer = correct_ret_addr;

Firstly, why does &regs->sp have to be forced to 'unsigned long *'?

pt_regs::sp is 'unsigned long' on both 32-bit and 64-bit kernels AFAICS.

Secondly, the new code modified by your patch now looks like this:

        frame_pointer = ((unsigned long *)&regs->sp) + 1;

+ kretprobe_trampoline_handler(regs, frame_pointer);

where:

arch/x86/kernel/kprobes/core.c +10 -0

+void arch_kretprobe_fixup_return(struct pt_regs *regs,
+                                unsigned long correct_ret_addr)
+{
+       unsigned long *frame_pointer;
+
+       frame_pointer = ((unsigned long *)&regs->sp) + 1;
+
+       /* Replace fake return address with real one. */
+       *frame_pointer = correct_ret_addr;
+}

So we first do:

        frame_pointer = ((unsigned long *)&regs->sp) + 1;

... and pass that in to arch_kretprobe_fixup_return() as 'correct_ret_addr', which does:

arch/x86/kernel/kprobes/core.c +2 -0

+       frame_pointer = ((unsigned long *)&regs->sp) + 1;
+	*frame_pointer = correct_ret_addr;

... which looks like the exact same thing as:

	*frame_pointer = frame_pointer;

... obfuscated through a thick layer of type casts?

Thanks,

Ingo

Hello,

Here is the 8th version of the series to fix the stacktrace with kretprobe on x86.

The previous version is;

https://lore.kernel.org/bpf/162209754288.436794.3904335049560916855.stgit@devnote2/

This version fixes to call appropriate function and drop some unneeded patches.

Changes from v7:
[03/13]: Call dereference_kernel_function_descriptor() for getting the
address of kretprobe_trampoline.
[09/13]: Update the title and description to explain why it is needed. [10/13][11/13]: Add Josh's Acked-by.

With this series, unwinder can unwind stack correctly from ftrace as below;

  # cd /sys/kernel/debug/tracing
  # echo > trace
  # echo 1 > options/sym-offset
  # echo r vfs_read >> kprobe_events
  # echo r full_proxy_read >> kprobe_events
  # echo traceoff:1 > events/kprobes/r_vfs_read_0/trigger
  # echo stacktrace:1 > events/kprobes/r_full_proxy_read_0/trigger
  # echo 1 > events/kprobes/enable
  # cat /sys/kernel/debug/kprobes/list
ffffffff8133b740  r  full_proxy_read+0x0    [FTRACE]
ffffffff812560b0  r  vfs_read+0x0    [FTRACE]
  # echo 0 > events/kprobes/enable
  # cat trace
# tracer: nop
#
# entries-in-buffer/entries-written: 3/3   #P:8
#
#                                _-----=> irqs-off
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| /     delay
#           TASK-PID     CPU#  ||||   TIMESTAMP  FUNCTION
#              | |         |   ||||      |         |
           <...>-134     [007] ...1    16.185877: r_full_proxy_read_0: (vfs_read+0x98/0x180 <- full_proxy_read)
           <...>-134     [007] ...1    16.185901: <stack trace>
 => kretprobe_trace_func+0x209/0x300
 => kretprobe_dispatcher+0x4a/0x70
 => __kretprobe_trampoline_handler+0xd4/0x170
 => trampoline_handler+0x43/0x60
 => kretprobe_trampoline+0x2a/0x50
 => vfs_read+0x98/0x180
 => ksys_read+0x5f/0xe0
 => do_syscall_64+0x37/0x90
 => entry_SYSCALL_64_after_hwframe+0x44/0xae
           <...>-134     [007] ...1    16.185902: r_vfs_read_0: (ksys_read+0x5f/0xe0 <- vfs_read)

This shows the double return probes (vfs_read and full_proxy_read) on the stack correctly unwinded. (vfs_read will return to ksys_read+0x5f and full_proxy_read will return to vfs_read+0x98)

This actually changes the kretprobe behavisor a bit, now the instraction pointer in the pt_regs passed to kretprobe user handler is correctly set the real return address. So user handlers can get it via instruction_pointer() API, and can use stack_trace_save_regs().

You can also get this series from
git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git kprobes/kretprobe-stackfix-v8

Thank you,

---

Josh Poimboeuf (1):
x86/kprobes: Add UNWIND_HINT_FUNC on kretprobe_trampoline code

Masami Hiramatsu (12):
ia64: kprobes: Fix to pass correct trampoline address to the handler
kprobes: treewide: Replace arch_deref_entry_point() with dereference_symbol_descriptor()
kprobes: treewide: Remove trampoline_address from kretprobe_trampoline_handler()
kprobes: Add kretprobe_find_ret_addr() for searching return address
ARC: Add instruction_pointer_set() API
ia64: Add instruction_pointer_set() API
arm: kprobes: Make a space for regs->ARM_pc at kretprobe_trampoline
kprobes: Enable stacktrace from pt_regs in kretprobe handler
x86/kprobes: Push a fake return address at kretprobe_trampoline x86/unwind: Recover kretprobe trampoline entry
tracing: Show kretprobe unknown indicator only for kretprobe_trampoline x86/kprobes: Fixup return address in generic trampoline handler

 arch/arc/include/asm/ptrace.h       |    5 ++
 arch/arc/kernel/kprobes.c           |    2 -
 arch/arm/probes/kprobes/core.c      |    5 +-
 arch/arm64/kernel/probes/kprobes.c  |    3 -
 arch/csky/kernel/probes/kprobes.c   |    2 -
 arch/ia64/include/asm/ptrace.h      |    5 ++
 arch/ia64/kernel/kprobes.c          |   15 ++---
 arch/mips/kernel/kprobes.c          |    3 -
 arch/parisc/kernel/kprobes.c        |    4 +
 arch/powerpc/kernel/kprobes.c       |   13 ----
 arch/riscv/kernel/probes/kprobes.c  |    2 -
 arch/s390/kernel/kprobes.c          |    2 -
 arch/sh/kernel/kprobes.c            |    2 -
 arch/sparc/kernel/kprobes.c         |    2 -
 arch/x86/include/asm/kprobes.h      |    1
 arch/x86/include/asm/unwind.h       |   23 +++++++
 arch/x86/include/asm/unwind_hints.h |    5 ++
 arch/x86/kernel/kprobes/core.c      |   53 +++++++++++++++--
 arch/x86/kernel/unwind_frame.c      |    3 -
 arch/x86/kernel/unwind_guess.c      |    3 -
 arch/x86/kernel/unwind_orc.c        |   18 +++++-
 include/linux/kprobes.h             |   44 ++++++++++++--
 kernel/kprobes.c                    |  108 +++++++++++++++++++++++++----------
 kernel/trace/trace_output.c         |   17 +-----
 lib/error-inject.c                  |    3 +
 25 files changed, 238 insertions(+), 105 deletions(-)

--
Masami Hiramatsu (Linaro) <[email protected]>

Hello,

Here is the 8th version of the series to fix the stacktrace with kretprobe on x86.

The previous version is;

https://lore.kernel.org/bpf/162209754288.436794.3904335049560916855.stgit@devnote2/

This version fixes to call appropriate function and drop some unneeded patches.

Changes from v7:
[03/13]: Call dereference_kernel_function_descriptor() for getting the
address of kretprobe_trampoline.
[09/13]: Update the title and description to explain why it is needed. [10/13][11/13]: Add Josh's Acked-by.

With this series, unwinder can unwind stack correctly from ftrace as below;

  # cd /sys/kernel/debug/tracing
  # echo > trace
  # echo 1 > options/sym-offset
  # echo r vfs_read >> kprobe_events
  # echo r full_proxy_read >> kprobe_events
  # echo traceoff:1 > events/kprobes/r_vfs_read_0/trigger
  # echo stacktrace:1 > events/kprobes/r_full_proxy_read_0/trigger
  # echo 1 > events/kprobes/enable
  # cat /sys/kernel/debug/kprobes/list
ffffffff8133b740  r  full_proxy_read+0x0    [FTRACE]
ffffffff812560b0  r  vfs_read+0x0    [FTRACE]
  # echo 0 > events/kprobes/enable
  # cat trace
# tracer: nop
#
# entries-in-buffer/entries-written: 3/3   #P:8
#
#                                _-----=> irqs-off
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| /     delay
#           TASK-PID     CPU#  ||||   TIMESTAMP  FUNCTION
#              | |         |   ||||      |         |
           <...>-134     [007] ...1    16.185877: r_full_proxy_read_0: (vfs_read+0x98/0x180 <- full_proxy_read)
           <...>-134     [007] ...1    16.185901: <stack trace>
 => kretprobe_trace_func+0x209/0x300
 => kretprobe_dispatcher+0x4a/0x70
 => __kretprobe_trampoline_handler+0xd4/0x170
 => trampoline_handler+0x43/0x60
 => kretprobe_trampoline+0x2a/0x50
 => vfs_read+0x98/0x180
 => ksys_read+0x5f/0xe0
 => do_syscall_64+0x37/0x90
 => entry_SYSCALL_64_after_hwframe+0x44/0xae
           <...>-134     [007] ...1    16.185902: r_vfs_read_0: (ksys_read+0x5f/0xe0 <- vfs_read)

This shows the double return probes (vfs_read and full_proxy_read) on the stack correctly unwinded. (vfs_read will return to ksys_read+0x5f and full_proxy_read will return to vfs_read+0x98)

This actually changes the kretprobe behavisor a bit, now the instraction pointer in the pt_regs passed to kretprobe user handler is correctly set the real return address. So user handlers can get it via instruction_pointer() API, and can use stack_trace_save_regs().

You can also get this series from
git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git kprobes/kretprobe-stackfix-v8

Thank you,

---

Josh Poimboeuf (1):
x86/kprobes: Add UNWIND_HINT_FUNC on kretprobe_trampoline code

Masami Hiramatsu (12):
ia64: kprobes: Fix to pass correct trampoline address to the handler
kprobes: treewide: Replace arch_deref_entry_point() with dereference_symbol_descriptor()
kprobes: treewide: Remove trampoline_address from kretprobe_trampoline_handler()
kprobes: Add kretprobe_find_ret_addr() for searching return address
ARC: Add instruction_pointer_set() API
ia64: Add instruction_pointer_set() API
arm: kprobes: Make a space for regs->ARM_pc at kretprobe_trampoline
kprobes: Enable stacktrace from pt_regs in kretprobe handler
x86/kprobes: Push a fake return address at kretprobe_trampoline x86/unwind: Recover kretprobe trampoline entry
tracing: Show kretprobe unknown indicator only for kretprobe_trampoline x86/kprobes: Fixup return address in generic trampoline handler

 arch/arc/include/asm/ptrace.h       |    5 ++
 arch/arc/kernel/kprobes.c           |    2 -
 arch/arm/probes/kprobes/core.c      |    5 +-
 arch/arm64/kernel/probes/kprobes.c  |    3 -
 arch/csky/kernel/probes/kprobes.c   |    2 -
 arch/ia64/include/asm/ptrace.h      |    5 ++
 arch/ia64/kernel/kprobes.c          |   15 ++---
 arch/mips/kernel/kprobes.c          |    3 -
 arch/parisc/kernel/kprobes.c        |    4 +
 arch/powerpc/kernel/kprobes.c       |   13 ----
 arch/riscv/kernel/probes/kprobes.c  |    2 -
 arch/s390/kernel/kprobes.c          |    2 -
 arch/sh/kernel/kprobes.c            |    2 -
 arch/sparc/kernel/kprobes.c         |    2 -
 arch/x86/include/asm/kprobes.h      |    1 
 arch/x86/include/asm/unwind.h       |   23 +++++++
 arch/x86/include/asm/unwind_hints.h |    5 ++
 arch/x86/kernel/kprobes/core.c      |   53 +++++++++++++++--
 arch/x86/kernel/unwind_frame.c      |    3 -
 arch/x86/kernel/unwind_guess.c      |    3 -
 arch/x86/kernel/unwind_orc.c        |   18 +++++-
 include/linux/kprobes.h             |   44 ++++++++++++--
 kernel/kprobes.c                    |  108 +++++++++++++++++++++++++----------
 kernel/trace/trace_output.c         |   17 +-----
 lib/error-inject.c                  |    3 +
 25 files changed, 238 insertions(+), 105 deletions(-)

--
Masami Hiramatsu (Linaro) <[email protected]>