commit 1069c1dd20a3 ("ARM: 9231/1: Recover kretprobes return address for
EABI stack unwinder")
tickled a bug in clang's integrated assembler where the .save and .pad
directives must have corresponding .fnstart directives. The integrated
assembler is unaware that the compiler will be generating the .fnstart
directive.

  arch/arm/probes/kprobes/core.c:409:30: error: .fnstart must precede
  .save or .vsave directives
  <inline asm>:3:2: note: instantiated into assembly here
  .save   {sp, lr, pc}
  ^
  arch/arm/probes/kprobes/core.c:412:29: error: .fnstart must precede
  .pad directive
  <inline asm>:6:2: note: instantiated into assembly here
  .pad    #52
  ^

__kretprobe_trampoline's definition is already entirely inline asm. Move
it to out-of-line asm to avoid breaking the build. Forward declare
trampoline_handler() to avoid -Wmissing-prototypes since it's only
called from assembler. Fixes another instance of -Wmissing-prototypes on
kprobe_handler() so that arch/arm/probes/kprobes/core.c builds cleanly
with W=1.

Link: https://github.com/llvm/llvm-project/issues/57993
Link: https://github.com/ClangBuiltLinux/linux/issues/1718
Reported-by: Nathan Chancellor <nathan@kernel.org>
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Suggested-by: Logan Chien <loganchien@google.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes v2 -> v3:
* Fix -Wmissing-prototypes on trampoline_handler() as reported by kernel
  test robot.
* Update comment above trampoline_handler().
* Fix another pre-existing case of -Wmissing-prototypes on
  kprobe_handler() so that arch/arm/probes/kprobes/core.c builds cleanly
  with W=1.
* Make note of the above in the commit message.

Changes v1 -> v2:
* rebase on linux-next again.
* drop commented out declaration of __kretprobe_trampoline from v1.

 arch/arm/probes/kprobes/Makefile              |  1 +
 arch/arm/probes/kprobes/core.c                | 54 +++---------------
 .../arm/probes/kprobes/kretprobe-trampoline.S | 55 +++++++++++++++++++
 include/asm-generic/kprobes.h                 | 13 +++--
 4 files changed, 72 insertions(+), 51 deletions(-)
 create mode 100644 arch/arm/probes/kprobes/kretprobe-trampoline.S

diff --git a/arch/arm/probes/kprobes/Makefile b/arch/arm/probes/kprobes/Makefile
index 6159010dac4a..cdbe9dd99e28 100644
--- a/arch/arm/probes/kprobes/Makefile
+++ b/arch/arm/probes/kprobes/Makefile
@@ -3,6 +3,7 @@ KASAN_SANITIZE_actions-common.o := n
 KASAN_SANITIZE_actions-arm.o := n
 KASAN_SANITIZE_actions-thumb.o := n
 obj-$(CONFIG_KPROBES)		+= core.o actions-common.o checkers-common.o
+obj-$(CONFIG_KPROBES)		+= kretprobe-trampoline.o
 obj-$(CONFIG_ARM_KPROBES_TEST)	+= test-kprobes.o
 test-kprobes-objs		:= test-core.o
 
diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 9090c3a74dcc..11159fcf6ba6 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -233,7 +233,7 @@ singlestep(struct kprobe *p, struct pt_regs *regs, struct kprobe_ctlblk *kcb)
  * kprobe, and that level is reserved for user kprobe handlers, so we can't
  * risk encountering a new kprobe in an interrupt handler.
  */
-void __kprobes kprobe_handler(struct pt_regs *regs)
+static void __kprobes kprobe_handler(struct pt_regs *regs)
 {
 	struct kprobe *p, *cur;
 	struct kprobe_ctlblk *kcb;
@@ -366,53 +366,11 @@ int __kprobes kprobe_exceptions_notify(struct notifier_block *self,
 }
 
 /*
- * When a retprobed function returns, trampoline_handler() is called,
- * calling the kretprobe's handler. We construct a struct pt_regs to
- * give a view of registers r0-r11, sp, lr, and pc to the user
- * return-handler. This is not a complete pt_regs structure, but that
- * should be enough for stacktrace from the return handler with or
- * without pt_regs.
+ * Called from __kretprobe_trampoline in assembler. Forward declare to avoid
+ * -Wmissing-prototypes.
  */
-void __naked __kprobes __kretprobe_trampoline(void)
-{
-	__asm__ __volatile__ (
-#ifdef CONFIG_FRAME_POINTER
-		"ldr	lr, =__kretprobe_trampoline	\n\t"
-	/* __kretprobe_trampoline makes a framepointer on pt_regs. */
-#ifdef CONFIG_CC_IS_CLANG
-		"stmdb	sp, {sp, lr, pc}	\n\t"
-		"sub	sp, sp, #12		\n\t"
-		/* In clang case, pt_regs->ip = lr. */
-		"stmdb	sp!, {r0 - r11, lr}	\n\t"
-		/* fp points regs->r11 (fp) */
-		"add	fp, sp,	#44		\n\t"
-#else /* !CONFIG_CC_IS_CLANG */
-		/* In gcc case, pt_regs->ip = fp. */
-		"stmdb	sp, {fp, sp, lr, pc}	\n\t"
-		"sub	sp, sp, #16		\n\t"
-		"stmdb	sp!, {r0 - r11}		\n\t"
-		/* fp points regs->r15 (pc) */
-		"add	fp, sp, #60		\n\t"
-#endif /* CONFIG_CC_IS_CLANG */
-#else /* !CONFIG_FRAME_POINTER */
-		"sub	sp, sp, #16		\n\t"
-		"stmdb	sp!, {r0 - r11}		\n\t"
-#endif /* CONFIG_FRAME_POINTER */
-		"mov	r0, sp			\n\t"
-		"bl	trampoline_handler	\n\t"
-		"mov	lr, r0			\n\t"
-		"ldmia	sp!, {r0 - r11}		\n\t"
-		"add	sp, sp, #16		\n\t"
-#ifdef CONFIG_THUMB2_KERNEL
-		"bx	lr			\n\t"
-#else
-		"mov	pc, lr			\n\t"
-#endif
-		: : : "memory");
-}
-
-/* Called from __kretprobe_trampoline */
-static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
+void *trampoline_handler(struct pt_regs *regs);
+__kprobes void *trampoline_handler(struct pt_regs *regs)
 {
 	return (void *)kretprobe_trampoline_handler(regs, (void *)regs->ARM_fp);
 }
@@ -420,6 +378,8 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
 void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				      struct pt_regs *regs)
 {
+	extern void __kretprobe_trampoline(void);
+
 	ri->ret_addr = (kprobe_opcode_t *)regs->ARM_lr;
 	ri->fp = (void *)regs->ARM_fp;
 
diff --git a/arch/arm/probes/kprobes/kretprobe-trampoline.S b/arch/arm/probes/kprobes/kretprobe-trampoline.S
new file mode 100644
index 000000000000..261c99b8c17f
--- /dev/null
+++ b/arch/arm/probes/kprobes/kretprobe-trampoline.S
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <linux/linkage.h>
+#include <asm/unwind.h>
+#include <asm-generic/kprobes.h>
+
+/*
+ * When a retprobed function returns, trampoline_handler() is called,
+ * calling the kretprobe's handler. We construct a struct pt_regs to
+ * give a view of registers r0-r11, sp, lr, and pc to the user
+ * return-handler. This is not a complete pt_regs structure, but that
+ * should be enough for stacktrace from the return handler with or
+ * without pt_regs.
+ */
+__KPROBE
+SYM_FUNC_START(__kretprobe_trampoline)
+UNWIND(.fnstart)
+	ldr	lr, =__kretprobe_trampoline
+#ifdef CONFIG_FRAME_POINTER
+	/* __kretprobe_trampoline makes a framepointer on pt_regs. */
+#ifdef CONFIG_CC_IS_CLANG
+	stmdb	sp, {sp, lr, pc}
+	sub	sp, sp, #12
+	/* In clang case, pt_regs->ip = lr. */
+	stmdb	sp!, {r0 - r11, lr}
+	/* fp points regs->r11 (fp) */
+	add	fp, sp, #44
+#else /* !CONFIG_CC_IS_CLANG */
+	/* In gcc case, pt_regs->ip = fp. */
+	stmdb	sp, {fp, sp, lr, pc}
+	sub	sp, sp, #16
+	stmdb	sp!, {r0 - r11}
+	/* fp points regs->r15 (pc) */
+	add	fp, sp, #60
+#endif /* CONFIG_CC_IS_CLANG */
+#else /* !CONFIG_FRAME_POINTER */
+	/* store SP, LR on stack and add EABI unwind hint */
+	stmdb	sp, {sp, lr, pc}
+UNWIND(.save	{sp, lr, pc})
+	sub	sp, sp, #16
+	stmdb	sp!, {r0 - r11}
+UNWIND(.pad	#52)
+#endif /* CONFIG_FRAME_POINTER */
+	mov	r0, sp
+	bl	trampoline_handler
+	mov	lr, r0
+	ldmia	sp!, {r0 - r11}
+	add	sp, sp, #16
+#ifdef CONFIG_THUMB2_KERNEL
+	bx	lr
+#else
+	mov	pc, lr
+#endif
+UNWIND(.fnend)
+SYM_FUNC_END(__kretprobe_trampoline)
diff --git a/include/asm-generic/kprobes.h b/include/asm-generic/kprobes.h
index 060eab094e5a..1509daa281b8 100644
--- a/include/asm-generic/kprobes.h
+++ b/include/asm-generic/kprobes.h
@@ -2,7 +2,11 @@
 #ifndef _ASM_GENERIC_KPROBES_H
 #define _ASM_GENERIC_KPROBES_H
 
-#if defined(__KERNEL__) && !defined(__ASSEMBLY__)
+#ifdef __KERNEL__
+
+#ifdef __ASSEMBLY__
+# define __KPROBE .section ".kprobes.text", "ax"
+#else
 #ifdef CONFIG_KPROBES
 /*
  * Blacklist ganerating macro. Specify functions which is not probed
@@ -16,11 +20,12 @@ static unsigned long __used					\
 /* Use this to forbid a kprobes attach on very low level functions */
 # define __kprobes	__section(".kprobes.text")
 # define nokprobe_inline	__always_inline
-#else
+#else /* !defined(CONFIG_KPROBES) */
 # define NOKPROBE_SYMBOL(fname)
 # define __kprobes
 # define nokprobe_inline	inline
-#endif
-#endif /* defined(__KERNEL__) && !defined(__ASSEMBLY__) */
+#endif /* defined(CONFIG_KPROBES) */
+#endif /* defined(__ASSEMBLY__) */
+#endif /* defined(__KERNEL__) */
 
 #endif /* _ASM_GENERIC_KPROBES_H */
-- 
2.38.0.rc1.362.ged0d419d3c-goog

On Fri, Sep 30, 2022 at 2:15 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> commit 1069c1dd20a3 ("ARM: 9231/1: Recover kretprobes return address for
> EABI stack unwinder")
> tickled a bug in clang's integrated assembler where the .save and .pad
> directives must have corresponding .fnstart directives. The integrated
> assembler is unaware that the compiler will be generating the .fnstart
> directive.
>
>   arch/arm/probes/kprobes/core.c:409:30: error: .fnstart must precede
>   .save or .vsave directives
>   <inline asm>:3:2: note: instantiated into assembly here
>   .save   {sp, lr, pc}
>   ^
>   arch/arm/probes/kprobes/core.c:412:29: error: .fnstart must precede
>   .pad directive
>   <inline asm>:6:2: note: instantiated into assembly here
>   .pad    #52
>   ^
>

Chen, I noticed that your patch was discarded; it's not in linux-next today.
https://lore.kernel.org/linux-arm-kernel/YzHPGvhLkdQcDYzx@shell.armlinux.org.uk/
https://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=9231/1
How would you like to proceed here?

I think moving this out of line, incorporating Ard's feedback, then
putting the UNWIND directives on top might be the way to go. What do
you think?
-- 
Thanks,
~Nick Desaulniers

Hi,

Sorry for late reply because I just found this thread before the long 
vacation so I didn't have much time to deal with it.

On 2022/10/7 4:35, Nick Desaulniers wrote:
> On Fri, Sep 30, 2022 at 2:15 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
>> commit 1069c1dd20a3 ("ARM: 9231/1: Recover kretprobes return address for
>> EABI stack unwinder")
>> tickled a bug in clang's integrated assembler where the .save and .pad
>> directives must have corresponding .fnstart directives. The integrated
>> assembler is unaware that the compiler will be generating the .fnstart
>> directive.
>>
>>    arch/arm/probes/kprobes/core.c:409:30: error: .fnstart must precede
>>    .save or .vsave directives
>>    <inline asm>:3:2: note: instantiated into assembly here
>>    .save   {sp, lr, pc}
>>    ^
>>    arch/arm/probes/kprobes/core.c:412:29: error: .fnstart must precede
>>    .pad directive
>>    <inline asm>:6:2: note: instantiated into assembly here
>>    .pad    #52
>>    ^
>>
> Chen, I noticed that your patch was discarded; it's not in linux-next today.
> https://lore.kernel.org/linux-arm-kernel/YzHPGvhLkdQcDYzx@shell.armlinux.org.uk/
> https://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=9231/1
> How would you like to proceed here?

Since 6.1 is closing now. Let's reorganize everything and queue it up 
for -next for 6.2

> I think moving this out of line, incorporating Ard's feedback, then
> putting the UNWIND directives on top might be the way to go. What do
> you think?

This way looks good to me.

How about making a set for this,  to make everything more clear:

1. Move this out of line

2. Apply the feature, test with gcc & clang

3. Other cleaning, or merge with 2 if the cleaning is tiny.

I'll send another version for this, rebased to 6.1-rc1

Thanks for your time!


Best,

Chen