Re: [PATCH] x86: KPROBE_ENTRY should be paired wth KPROBE_END

[Cyrill Gorcunov <gorcunov@gmail.com>]

[Alexander van Heukelum - Sun, Nov 23, 2008 at 04:04:18PM +0100]
| On Sun, Nov 23, 2008 at 05:12:37PM +0300, Cyrill Gorcunov wrote:
| > [Cyrill Gorcunov - Sun, Nov 23, 2008 at 04:51:34PM +0300]
| > | [Ingo Molnar - Sun, Nov 23, 2008 at 02:27:52PM +0100]
| > | | 
| > | | * Alexander van Heukelum <heukelum@mailshack.com> wrote:
| > | | 
| > | | > Impact: moves some code out of .kprobes.text
| > | | > 
| > | | > KPROBE_ENTRY switches code generation to .kprobes.text, and KPROBE_END
| > | | > uses .popsection to get back to the previous section (.text, normally).
| > | | > Also replace ENDPROC by END, for consistency.
| > | | > 
| > | | > Signed-off-by: Alexander van Heukelum <heukelum@fastmail.fm>
| > | | 
| > | | applied to tip/x86/irq, thanks Alexander!
| > | | 
| > | | > One more small change for today. The xen-related functions 
| > | | > xen_do_hypervisor_callback and xen_failsafe_callback are put
| > | | > in the .kprobes.text even in the current kernel: ignore_sysret
| > | | > is enclosed in KPROBE_ENTRY / ENDPROC, instead of KPROBE_ENTRY /
| > | | > KPROBE_END, but I guess the situation is harmless.
| > | | 
| > | | yeah. It narrows no-kprobes protection for that code, but it should 
| > | | indeed be fine (and that's the intention as well).
| > | | 
| > | | Note that this is a reoccuring bug type, and rather long-lived. Can 
| > | | you think of any way to get automated nesting protection of both the 
| > | | .cfi_startproc/endproc macros and kprobes start/end? A poor man's 
| > | | solution would be to grep the number of start and end methods and 
| > | | enforce that they are equal.
| > | | 
| > | | 	Ingo
| > | | 
| > | 
| > | I think we could play with preprocessor and check if ENTRY/END matches.
| > | Looking now.
| > | 
| > | 		- Cyrill -
| > 
| > Here is what I've done
| > 
| > 1) Add some macros like:
| > 
| > 	.macro __set_entry
| > 	.set _ENTRY_IN, 1
| > 	.endm
| > 
| > 	.macro __unset_entry
| > 	.set _ENTRY_IN, 0
| > 	.endm
| > 
| > 	.macro __check_entry
| > 	.ifeq _ENTRY_IN
| > 	.error "END should be used"
| > 	.abort
| > 	.endif
| > 	.endm
| > 
| > So the code
| > 
| > ENTRY(mcount)
| > 	__unset_entry
| > 	retq
| > 	__check_entry
| > END(mcount)
| 
| Looks like a good approach to me. But I assume the ENTRY cppmacro
| will include magic?
| 
| Greetings,
| 	Alexander
| 
| > will fail like
| > 
| > cyrill@lenovo linux-2.6.git $ make arch/x86/kernel/entry_64.o
| >   CHK     include/linux/version.h
| >   CHK     include/linux/utsrelease.h
| >   SYMLINK include/asm -> include/asm-x86
| >   CALL    scripts/checksyscalls.sh
| >   AS      arch/x86/kernel/entry_64.o
| > arch/x86/kernel/entry_64.S: Assembler messages:
| > arch/x86/kernel/entry_64.S:84: Error: END should be used
| > arch/x86/kernel/entry_64.S:84: Fatal error: .abort detected.  Abandoning ship.
| > make[1]: *** [arch/x86/kernel/entry_64.o] Error 1
| > make: *** [arch/x86/kernel/entry_64.o] Error 2
| > cyrill@lenovo linux-2.6.git $ 
| > 
| > So if such an approach is acceptable (in general) -- I could take a more
| > deeper look. So every ENTRY would check if other ENTRY/KPROBE is active
| > and report that.
| > 
| > 		- Cyrill -
| 

OK, the patch (below) found the first problem. The patch is still quite
rough and not good for usage in general but it found that we have an
unbalanced ENTRY for ENTRY(native_usergs_sysret64).

And the message is not fully correct -- it's not mess btw ENTRY and KPROBE
but just unbalanced ENRTY -- ie not closed by END.

---
cyrill@lenovo linux-2.6.git $ make arch/x86/kernel/entry_64.o
  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  SYMLINK include/asm -> include/asm-x86
  CALL    scripts/checksyscalls.sh
  AS      arch/x86/kernel/entry_64.o
arch/x86/kernel/entry_64.S: Assembler messages:
arch/x86/kernel/entry_64.S:284: Error: Do not mess regular ENTRY and KPROBE!
arch/x86/kernel/entry_64.S:284: Fatal error: .abort detected.  Abandoning
ship.
make[1]: *** [arch/x86/kernel/entry_64.o] Error 1
make: *** [arch/x86/kernel/entry_64.o] Error 2
cyrill@lenovo linux-2.6.git $ 
---

Not sure if general linkage.h is good place for such a macros.
Anyway, the patch applied to get a glace view.

		- Cyrill -
---

---
 include/linux/linkage.h |   43 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 4 deletions(-)

Index: linux-2.6.git/include/linux/linkage.h
===================================================================
--- linux-2.6.git.orig/include/linux/linkage.h
+++ linux-2.6.git/include/linux/linkage.h
@@ -51,8 +51,35 @@
 #define ALIGN __ALIGN
 #define ALIGN_STR __ALIGN_STR
 
+#define __set_entry   .set _ENTRY_IN, 0
+#define __unset_entry .set _ENTRY_IN, 1
+#define __set_kprobe  .set _KPROBE_IN, 0
+#define __unset_kprobe .set _KPROBE_IN, 1
+
+#define __check_entry		\
+	.ifdef _ENTRY_IN;	\
+	.ifeq _ENTRY_IN;	\
+	.error "Do not mess regular ENTRY and KPROBE!"; \
+	.abort;			\
+	.endif;			\
+	.endif
+
+#define __check_kprobe		\
+	.ifdef _KPROBE_IN;	\
+	.ifeq _KPROBE_IN;	\
+	.error "Do not mess regular ENTRY and KPROBE!"; \
+	.abort;			\
+	.endif;			\
+	.endif
+
+#define __check_entry_kprobe	\
+	__check_entry;		\
+	__check_kprobe
+
 #ifndef ENTRY
 #define ENTRY(name) \
+  __check_entry_kprobe; \
+  __set_entry; \
   .globl name; \
   ALIGN; \
   name:
@@ -65,16 +92,24 @@
 #endif
 
 #define KPROBE_ENTRY(name) \
+  __check_entry_kprobe; \
+  __set_kprobe; \
   .pushsection .kprobes.text, "ax"; \
-  ENTRY(name)
+  .globl name; \
+  ALIGN; \
+  name:
 
 #define KPROBE_END(name) \
-  END(name);		 \
-  .popsection
+  __unset_kprobe; \
+  __check_entry_kprobe; \
+    .size name, .-name; \
+ .popsection
 
 #ifndef END
 #define END(name) \
-  .size name, .-name
+  __unset_entry; \
+  __check_entry_kprobe; \
+   .size name, .-name
 #endif
 
 /* If symbol 'name' is treated as a subroutine (gets called, and returns)