linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text
@ 2013-11-08 12:52 Masami Hiramatsu
  2013-11-08 12:52 ` [PATCH -tip RFC 1/2] kprobes: Prohibit probing on .entry.text code Masami Hiramatsu
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Masami Hiramatsu @ 2013-11-08 12:52 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: x86, Ananth N Mavinakayanahalli, Peter Zijlstra, Rusty Russell,
	lkml, Steven Rostedt (Red Hat),
	virtualization, David S. Miller

Currently the blacklist is maintained by hand in kprobes.c 
which is separated from the function definition and is hard
to catch up the kernel update.
To solve this issue, I've tried to implement new
NOKPROBE_SYMBOL() macro for making kprobe blacklist at 
build time. Since the NOKPROBE_SYMBOL() macros can be placed
right after the function is defined, it is easy to maintain.
At this moment, I applied the macro only for the symbols
which is listed in kprobes.c. As we discussed in previous
thread, if the gcc accepts to introduce new annotation to
store the function address (and size) at somewhere, we can
easily move onto that by replacing NOKPROBE_SYMBOL() with
nokprobe annotation (and just modifying the
populate_kprobe_blacklist() a bit).

This series also includes a change which prohibits probing
on the address in .entry.text because the code is used for
very low-level sensitive interrupt/syscall entries. Probing
such code may cause unexpected result (actually most of
that area is already in the kprobe blacklist).
So I've decide to prohibit probing all of them.

Since Ingo wasn't convinced about the idea in the previous
discussion, I just make this series as RFC series.
I'd like to ask again with actual implementation and plan.

Thank you,

---

Masami Hiramatsu (2):
      kprobes: Prohibit probing on .entry.text code
      kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist


 arch/x86/kernel/entry_32.S        |   33 ------------
 arch/x86/kernel/entry_64.S        |   20 --------
 arch/x86/kernel/paravirt.c        |    4 ++
 include/asm-generic/vmlinux.lds.h |    9 +++
 include/linux/kprobes.h           |   19 +++++++
 kernel/kprobes.c                  |   98 ++++++++++++++++++-------------------
 kernel/sched/core.c               |    1 
 7 files changed, 80 insertions(+), 104 deletions(-)

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH -tip RFC 1/2] kprobes: Prohibit probing on .entry.text code
  2013-11-08 12:52 [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text Masami Hiramatsu
@ 2013-11-08 12:52 ` Masami Hiramatsu
  2013-11-08 12:52 ` [PATCH -tip RFC 2/2] kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist Masami Hiramatsu
  2013-11-11 11:16 ` [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text Ingo Molnar
  2 siblings, 0 replies; 7+ messages in thread
From: Masami Hiramatsu @ 2013-11-08 12:52 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: x86, Ananth N Mavinakayanahalli, Peter Zijlstra, Rusty Russell,
	lkml, Steven Rostedt (Red Hat),
	virtualization, David S. Miller

.entry.text is a code area which is used for interrupt/syscall
entries, and there are many sensitive codes.
Thus, it is better to prohibit probing on all of such codes
instead of a part of that.
Since some symbols are already registered on kprobe blacklist,
this also removes them from the blacklist.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 arch/x86/kernel/entry_32.S |   33 ---------------------------------
 arch/x86/kernel/entry_64.S |   20 --------------------
 kernel/kprobes.c           |   10 +++++-----
 3 files changed, 5 insertions(+), 58 deletions(-)

diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index fd1bc1b..6d19cfb 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -315,10 +315,6 @@ ENTRY(ret_from_kernel_thread)
 ENDPROC(ret_from_kernel_thread)
 
 /*
- * Interrupt exit functions should be protected against kprobes
- */
-	.pushsection .kprobes.text, "ax"
-/*
  * Return to user mode is not as complex as all this looks,
  * but we want the default path for a system call return to
  * go as quickly as possible which is why some of this is
@@ -372,10 +368,6 @@ need_resched:
 END(resume_kernel)
 #endif
 	CFI_ENDPROC
-/*
- * End of kprobes section
- */
-	.popsection
 
 /* SYSENTER_RETURN points to after the "sysenter" instruction in
    the vsyscall page.  See vsyscall-sysentry.S, which defines the symbol.  */
@@ -495,10 +487,6 @@ sysexit_audit:
 	PTGS_TO_GS_EX
 ENDPROC(ia32_sysenter_target)
 
-/*
- * syscall stub including irq exit should be protected against kprobes
- */
-	.pushsection .kprobes.text, "ax"
 	# system call handler stub
 ENTRY(system_call)
 	RING0_INT_FRAME			# can't unwind into user space anyway
@@ -691,10 +679,6 @@ syscall_badsys:
 	jmp resume_userspace
 END(syscall_badsys)
 	CFI_ENDPROC
-/*
- * End of kprobes section
- */
-	.popsection
 
 .macro FIXUP_ESPFIX_STACK
 /*
@@ -781,10 +765,6 @@ common_interrupt:
 ENDPROC(common_interrupt)
 	CFI_ENDPROC
 
-/*
- *  Irq entries should be protected against kprobes
- */
-	.pushsection .kprobes.text, "ax"
 #define BUILD_INTERRUPT3(name, nr, fn)	\
 ENTRY(name)				\
 	RING0_INT_FRAME;		\
@@ -961,10 +941,6 @@ ENTRY(spurious_interrupt_bug)
 	jmp error_code
 	CFI_ENDPROC
 END(spurious_interrupt_bug)
-/*
- * End of kprobes section
- */
-	.popsection
 
 #ifdef CONFIG_XEN
 /* Xen doesn't set %esp to be precisely what the normal sysenter
@@ -1239,11 +1215,6 @@ return_to_handler:
 	jmp *%ecx
 #endif
 
-/*
- * Some functions should be protected against kprobes
- */
-	.pushsection .kprobes.text, "ax"
-
 ENTRY(page_fault)
 	RING0_EC_FRAME
 	ASM_CLAC
@@ -1443,7 +1414,3 @@ ENTRY(async_page_fault)
 END(async_page_fault)
 #endif
 
-/*
- * End of kprobes section
- */
-	.popsection
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 603be7c..263c6cf 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -487,8 +487,6 @@ ENDPROC(native_usergs_sysret64)
 	TRACE_IRQS_OFF
 	.endm
 
-/* save complete stack frame */
-	.pushsection .kprobes.text, "ax"
 ENTRY(save_paranoid)
 	XCPT_FRAME 1 RDI+8
 	cld
@@ -517,7 +515,6 @@ ENTRY(save_paranoid)
 1:	ret
 	CFI_ENDPROC
 END(save_paranoid)
-	.popsection
 
 /*
  * A newly forked process directly context switches into this address.
@@ -975,10 +972,6 @@ END(interrupt)
 	call \func
 	.endm
 
-/*
- * Interrupt entry/exit should be protected against kprobes
- */
-	.pushsection .kprobes.text, "ax"
 	/*
 	 * The interrupt stubs push (~vector+0x80) onto the stack and
 	 * then jump to common_interrupt.
@@ -1113,10 +1106,6 @@ ENTRY(retint_kernel)
 
 	CFI_ENDPROC
 END(common_interrupt)
-/*
- * End of kprobes section
- */
-       .popsection
 
 /*
  * APIC interrupts.
@@ -1466,11 +1455,6 @@ apicinterrupt3 HYPERVISOR_CALLBACK_VECTOR \
 	hyperv_callback_vector hyperv_vector_handler
 #endif /* CONFIG_HYPERV */
 
-/*
- * Some functions should be protected against kprobes
- */
-	.pushsection .kprobes.text, "ax"
-
 paranoidzeroentry_ist debug do_debug DEBUG_STACK
 paranoidzeroentry_ist int3 do_int3 DEBUG_STACK
 paranoiderrorentry stack_segment do_stack_segment
@@ -1887,7 +1871,3 @@ ENTRY(ignore_sysret)
 	CFI_ENDPROC
 END(ignore_sysret)
 
-/*
- * End of kprobes section
- */
-	.popsection
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index a0d367a..ec0dbc7 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -96,9 +96,6 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
 static struct kprobe_blackpoint kprobe_blacklist[] = {
 	{"preempt_schedule",},
 	{"native_get_debugreg",},
-	{"irq_entries_start",},
-	{"common_interrupt",},
-	{"mcount",},	/* mcount can be called from everywhere */
 	{NULL}    /* Terminator */
 };
 
@@ -1328,8 +1325,11 @@ static int __kprobes in_kprobes_functions(unsigned long addr)
 {
 	struct kprobe_blackpoint *kb;
 
-	if (addr >= (unsigned long)__kprobes_text_start &&
-	    addr < (unsigned long)__kprobes_text_end)
+	/* The __kprobes marked functions and entry code must not be probed */
+	if ((addr >= (unsigned long)__kprobes_text_start &&
+	     addr < (unsigned long)__kprobes_text_end) ||
+	    (addr >= (unsigned long)__entry_text_start &&
+	     addr < (unsigned long)__entry_text_end))
 		return -EINVAL;
 	/*
 	 * If there exists a kprobe_blacklist, verify and


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH -tip RFC 2/2] kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist
  2013-11-08 12:52 [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text Masami Hiramatsu
  2013-11-08 12:52 ` [PATCH -tip RFC 1/2] kprobes: Prohibit probing on .entry.text code Masami Hiramatsu
@ 2013-11-08 12:52 ` Masami Hiramatsu
  2013-11-11 11:16 ` [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text Ingo Molnar
  2 siblings, 0 replies; 7+ messages in thread
From: Masami Hiramatsu @ 2013-11-08 12:52 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: x86, Ananth N Mavinakayanahalli, Peter Zijlstra, Rusty Russell,
	lkml, Steven Rostedt (Red Hat),
	virtualization, David S. Miller

Introduce NOKPROBE_SYMBOL() macro which builds a kprobe
blacklist in build time. The usage of this macro is similar
to the EXPORT_SYMBOL, put the NOKPROBE_SYMBOL(function); just
after the function definition.

If CONFIG_KPROBES=y, the macro is expanded to the definition
of a static data structure of kprobe_blackpoint which is
initialized for the function and put the address of the data
structure in the "_kprobe_blacklist" section.

Since the data structures are not fully initialized by the
macro (because there is no "size" information),  those
are re-initialized at boot time by using kallsyms.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
 arch/x86/kernel/paravirt.c        |    4 ++
 include/asm-generic/vmlinux.lds.h |    9 ++++
 include/linux/kprobes.h           |   19 ++++++++
 kernel/kprobes.c                  |   88 ++++++++++++++++++-------------------
 kernel/sched/core.c               |    1 
 5 files changed, 75 insertions(+), 46 deletions(-)

diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 1b10af8..4c785fd 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -23,6 +23,7 @@
 #include <linux/efi.h>
 #include <linux/bcd.h>
 #include <linux/highmem.h>
+#include <linux/kprobes.h>
 
 #include <asm/bug.h>
 #include <asm/paravirt.h>
@@ -389,6 +390,9 @@ __visible struct pv_cpu_ops pv_cpu_ops = {
 	.end_context_switch = paravirt_nop,
 };
 
+/* At this point, native_get_debugreg has real function entry */
+NOKPROBE_SYMBOL(native_get_debugreg);
+
 struct pv_apic_ops pv_apic_ops = {
 #ifdef CONFIG_X86_LOCAL_APIC
 	.startup_ipi_hook = paravirt_nop,
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 83e2c31..294ea96 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -109,6 +109,14 @@
 #define BRANCH_PROFILE()
 #endif
 
+#ifdef CONFIG_KPROBES
+#define KPROBE_BLACKLIST()	VMLINUX_SYMBOL(__start_kprobe_blacklist) = .; \
+				*(_kprobe_blacklist)			      \
+				VMLINUX_SYMBOL(__stop_kprobe_blacklist) = .;
+#else
+#define KPROBE_BLACKLIST()
+#endif
+
 #ifdef CONFIG_EVENT_TRACING
 #define FTRACE_EVENTS()	. = ALIGN(8);					\
 			VMLINUX_SYMBOL(__start_ftrace_events) = .;	\
@@ -487,6 +495,7 @@
 	*(.init.rodata)							\
 	FTRACE_EVENTS()							\
 	TRACE_SYSCALLS()						\
+	KPROBE_BLACKLIST()						\
 	MEM_DISCARD(init.rodata)					\
 	CLK_OF_TABLES()							\
 	CLKSRC_OF_TABLES()						\
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 925eaf2..a403038 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -206,6 +206,7 @@ struct kretprobe_blackpoint {
 };
 
 struct kprobe_blackpoint {
+	struct list_head list;
 	const char *name;
 	unsigned long start_addr;
 	unsigned long range;
@@ -476,4 +477,22 @@ static inline int enable_jprobe(struct jprobe *jp)
 	return enable_kprobe(&jp->kp);
 }
 
+#ifdef CONFIG_KPROBES
+/*
+ * Blacklist ganerating macro. Specify functions which is not probed
+ * by using this macro.
+ */
+#define NOKPROBE_SYMBOL(fname)			\
+static struct kprobe_blackpoint __used		\
+  _kprobe_bp_##fname = {			\
+	.name = #fname,				\
+	.start_addr = (unsigned long)fname,	\
+};						\
+static struct kprobe_blackpoint __used		\
+  __attribute__((section("_kprobe_blacklist"))) \
+ *_p_kprobe_bp_##fname = &_kprobe_bp_##fname;
+#else
+#define NOKPROBE_SYMBOL(fname)
+#endif
+
 #endif /* _LINUX_KPROBES_H */
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ec0dbc7..1fab712 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -86,18 +86,8 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
 	return &(kretprobe_table_locks[hash].lock);
 }
 
-/*
- * Normally, functions that we'd want to prohibit kprobes in, are marked
- * __kprobes. But, there are cases where such functions already belong to
- * a different section (__sched for preempt_schedule)
- *
- * For such cases, we now have a blacklist
- */
-static struct kprobe_blackpoint kprobe_blacklist[] = {
-	{"preempt_schedule",},
-	{"native_get_debugreg",},
-	{NULL}    /* Terminator */
-};
+/* Blacklist -- list of struct kprobe_blackpoint */
+static LIST_HEAD(kprobe_blacklist);
 
 #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
 /*
@@ -1321,9 +1311,9 @@ out:
 	return ret;
 }
 
-static int __kprobes in_kprobes_functions(unsigned long addr)
+static int __kprobes in_nokprobe_functions(unsigned long addr)
 {
-	struct kprobe_blackpoint *kb;
+	struct kprobe_blackpoint *bp;
 
 	/* The __kprobes marked functions and entry code must not be probed */
 	if ((addr >= (unsigned long)__kprobes_text_start &&
@@ -1335,12 +1325,10 @@ static int __kprobes in_kprobes_functions(unsigned long addr)
 	 * If there exists a kprobe_blacklist, verify and
 	 * fail any probe registration in the prohibited area
 	 */
-	for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
-		if (kb->start_addr) {
-			if (addr >= kb->start_addr &&
-			    addr < (kb->start_addr + kb->range))
-				return -EINVAL;
-		}
+	list_for_each_entry(bp, &kprobe_blacklist, list) {
+		if (addr >= bp->start_addr &&
+		    addr < (bp->start_addr + bp->range))
+			return -EINVAL;
 	}
 	return 0;
 }
@@ -1433,7 +1421,7 @@ static __kprobes int check_kprobe_address_safe(struct kprobe *p,
 
 	/* Ensure it is not in reserved area nor out of text */
 	if (!kernel_text_address((unsigned long) p->addr) ||
-	    in_kprobes_functions((unsigned long) p->addr) ||
+	    in_nokprobe_functions((unsigned long) p->addr) ||
 	    jump_label_text_reserved(p->addr, p->addr)) {
 		ret = -EINVAL;
 		goto out;
@@ -2062,14 +2050,41 @@ static struct notifier_block kprobe_module_nb = {
 	.priority = 0
 };
 
-static int __init init_kprobes(void)
+/*
+ * Lookup and populate the kprobe_blacklist.
+ *
+ * Unlike the kretprobe blacklist, we'll need to determine
+ * the range of addresses that belong to the said functions,
+ * since a kprobe need not necessarily be at the beginning
+ * of a function.
+ */
+static void populate_kprobe_blacklist(struct kprobe_blackpoint **start,
+				      struct kprobe_blackpoint **end)
 {
-	int i, err = 0;
+	struct kprobe_blackpoint **iter, *bp;
 	unsigned long offset = 0, size = 0;
 	char *modname, namebuf[128];
 	const char *symbol_name;
-	void *addr;
-	struct kprobe_blackpoint *kb;
+
+	for (iter = start; (unsigned long)iter < (unsigned long)end; iter++) {
+		bp = *iter;
+		symbol_name = kallsyms_lookup(bp->start_addr,
+				&size, &offset, &modname, namebuf);
+		if (!symbol_name)
+			continue;
+
+		bp->range = size;
+		INIT_LIST_HEAD(&bp->list);
+		list_add_tail(&bp->list, &kprobe_blacklist);
+	}
+}
+
+extern struct kprobe_blackpoint *__start_kprobe_blacklist[];
+extern struct kprobe_blackpoint *__stop_kprobe_blacklist[];
+
+static int __init init_kprobes(void)
+{
+	int i, err = 0;
 
 	/* FIXME allocate the probe table, currently defined statically */
 	/* initialize all list heads */
@@ -2079,27 +2094,8 @@ static int __init init_kprobes(void)
 		raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
 	}
 
-	/*
-	 * Lookup and populate the kprobe_blacklist.
-	 *
-	 * Unlike the kretprobe blacklist, we'll need to determine
-	 * the range of addresses that belong to the said functions,
-	 * since a kprobe need not necessarily be at the beginning
-	 * of a function.
-	 */
-	for (kb = kprobe_blacklist; kb->name != NULL; kb++) {
-		kprobe_lookup_name(kb->name, addr);
-		if (!addr)
-			continue;
-
-		kb->start_addr = (unsigned long)addr;
-		symbol_name = kallsyms_lookup(kb->start_addr,
-				&size, &offset, &modname, namebuf);
-		if (!symbol_name)
-			kb->range = 0;
-		else
-			kb->range = size;
-	}
+	populate_kprobe_blacklist(__start_kprobe_blacklist,
+				  __stop_kprobe_blacklist);
 
 	if (kretprobe_blacklist_size) {
 		/* lookup the function address from its name */
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 1deccd7..527fd78 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2645,6 +2645,7 @@ asmlinkage void __sched notrace preempt_schedule(void)
 		barrier();
 	} while (need_resched());
 }
+NOKPROBE_SYMBOL(preempt_schedule);
 EXPORT_SYMBOL(preempt_schedule);
 
 /*


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text
  2013-11-08 12:52 [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text Masami Hiramatsu
  2013-11-08 12:52 ` [PATCH -tip RFC 1/2] kprobes: Prohibit probing on .entry.text code Masami Hiramatsu
  2013-11-08 12:52 ` [PATCH -tip RFC 2/2] kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist Masami Hiramatsu
@ 2013-11-11 11:16 ` Ingo Molnar
  2013-11-11 17:18   ` Masami Hiramatsu
  2 siblings, 1 reply; 7+ messages in thread
From: Ingo Molnar @ 2013-11-11 11:16 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: x86, Ananth N Mavinakayanahalli, Peter Zijlstra, Rusty Russell,
	lkml, Steven Rostedt (Red Hat),
	virtualization, David S. Miller


* Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:

> Currently the blacklist is maintained by hand in kprobes.c 
> which is separated from the function definition and is hard
> to catch up the kernel update.
> To solve this issue, I've tried to implement new
> NOKPROBE_SYMBOL() macro for making kprobe blacklist at 
> build time. Since the NOKPROBE_SYMBOL() macros can be placed
> right after the function is defined, it is easy to maintain.
> At this moment, I applied the macro only for the symbols
> which is listed in kprobes.c. As we discussed in previous
> thread, if the gcc accepts to introduce new annotation to
> store the function address (and size) at somewhere, we can
> easily move onto that by replacing NOKPROBE_SYMBOL() with
> nokprobe annotation (and just modifying the
> populate_kprobe_blacklist() a bit).
> 
> This series also includes a change which prohibits probing
> on the address in .entry.text because the code is used for
> very low-level sensitive interrupt/syscall entries. Probing
> such code may cause unexpected result (actually most of
> that area is already in the kprobe blacklist).
> So I've decide to prohibit probing all of them.
> 
> Since Ingo wasn't convinced about the idea in the previous
> discussion, I just make this series as RFC series.
> I'd like to ask again with actual implementation and plan.
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (2):
>       kprobes: Prohibit probing on .entry.text code
>       kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist
> 
> 
>  arch/x86/kernel/entry_32.S        |   33 ------------
>  arch/x86/kernel/entry_64.S        |   20 --------
>  arch/x86/kernel/paravirt.c        |    4 ++
>  include/asm-generic/vmlinux.lds.h |    9 +++
>  include/linux/kprobes.h           |   19 +++++++
>  kernel/kprobes.c                  |   98 ++++++++++++++++++-------------------
>  kernel/sched/core.c               |    1 
>  7 files changed, 80 insertions(+), 104 deletions(-)

Ok, I like it after all.

Mind changing over arch/x86/kprobes/* to use this new facility? There's no 
sense in kprobes internals using two types

After that we can convert all the rest, probably as part of this series.

There's a good reason now to do it: it's not just about cleanliness, it 
will also impact generated code less.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Re: [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text
  2013-11-11 11:16 ` [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text Ingo Molnar
@ 2013-11-11 17:18   ` Masami Hiramatsu
  2013-11-11 17:25     ` Steven Rostedt
  0 siblings, 1 reply; 7+ messages in thread
From: Masami Hiramatsu @ 2013-11-11 17:18 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: x86, Ananth N Mavinakayanahalli, Peter Zijlstra, Rusty Russell,
	lkml, Steven Rostedt (Red Hat),
	virtualization, David S. Miller

(2013/11/11 20:16), Ingo Molnar wrote:
> 
> * Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
>> Currently the blacklist is maintained by hand in kprobes.c 
>> which is separated from the function definition and is hard
>> to catch up the kernel update.
>> To solve this issue, I've tried to implement new
>> NOKPROBE_SYMBOL() macro for making kprobe blacklist at 
>> build time. Since the NOKPROBE_SYMBOL() macros can be placed
>> right after the function is defined, it is easy to maintain.
>> At this moment, I applied the macro only for the symbols
>> which is listed in kprobes.c. As we discussed in previous
>> thread, if the gcc accepts to introduce new annotation to
>> store the function address (and size) at somewhere, we can
>> easily move onto that by replacing NOKPROBE_SYMBOL() with
>> nokprobe annotation (and just modifying the
>> populate_kprobe_blacklist() a bit).
>>
>> This series also includes a change which prohibits probing
>> on the address in .entry.text because the code is used for
>> very low-level sensitive interrupt/syscall entries. Probing
>> such code may cause unexpected result (actually most of
>> that area is already in the kprobe blacklist).
>> So I've decide to prohibit probing all of them.
>>
>> Since Ingo wasn't convinced about the idea in the previous
>> discussion, I just make this series as RFC series.
>> I'd like to ask again with actual implementation and plan.
>>
>> Thank you,
>>
>> ---
>>
>> Masami Hiramatsu (2):
>>       kprobes: Prohibit probing on .entry.text code
>>       kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist
>>
>>
>>  arch/x86/kernel/entry_32.S        |   33 ------------
>>  arch/x86/kernel/entry_64.S        |   20 --------
>>  arch/x86/kernel/paravirt.c        |    4 ++
>>  include/asm-generic/vmlinux.lds.h |    9 +++
>>  include/linux/kprobes.h           |   19 +++++++
>>  kernel/kprobes.c                  |   98 ++++++++++++++++++-------------------
>>  kernel/sched/core.c               |    1 
>>  7 files changed, 80 insertions(+), 104 deletions(-)
> 
> Ok, I like it after all.
> 
> Mind changing over arch/x86/kprobes/* to use this new facility? There's no 
> sense in kprobes internals using two types

Sure, that's why I introduced this :)

> After that we can convert all the rest, probably as part of this series.

OK, I'll do. :)
BTW, converting all the __kprobes involves many archs, which
kprobes ported. In that case, which mailing-list would better me
to post the series, linux-arch?

> 
> There's a good reason now to do it: it's not just about cleanliness, it 
> will also impact generated code less.

Thank you!


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text
  2013-11-11 17:18   ` Masami Hiramatsu
@ 2013-11-11 17:25     ` Steven Rostedt
  2013-11-11 21:15       ` Ingo Molnar
  0 siblings, 1 reply; 7+ messages in thread
From: Steven Rostedt @ 2013-11-11 17:25 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Ingo Molnar, x86, Ananth N Mavinakayanahalli, Peter Zijlstra,
	Rusty Russell, lkml, virtualization, David S. Miller

On Tue, 12 Nov 2013 02:18:53 +0900
Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
> > After that we can convert all the rest, probably as part of this series.
> 
> OK, I'll do. :)
> BTW, converting all the __kprobes involves many archs, which
> kprobes ported. In that case, which mailing-list would better me
> to post the series, linux-arch?

I would add linux-arch.

Note, you may need to support both ways for the current time being, as
new __kprobes are being added (I've seen several in patches flying by
in LKML).

But perhaps at a later -rc we convert the rest and discontinue them.
That way, linux-next will break all new __kprobes, and that should get
them fixed before we enter 3.14-rc.

-- Steve



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text
  2013-11-11 17:25     ` Steven Rostedt
@ 2013-11-11 21:15       ` Ingo Molnar
  0 siblings, 0 replies; 7+ messages in thread
From: Ingo Molnar @ 2013-11-11 21:15 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Masami Hiramatsu, x86, Ananth N Mavinakayanahalli,
	Peter Zijlstra, Rusty Russell, lkml, virtualization,
	David S. Miller


* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 12 Nov 2013 02:18:53 +0900
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> > 
> > > After that we can convert all the rest, probably as part of this series.
> > 
> > OK, I'll do. :)
> > BTW, converting all the __kprobes involves many archs, which
> > kprobes ported. In that case, which mailing-list would better me
> > to post the series, linux-arch?
> 
> I would add linux-arch.
> 
> Note, you may need to support both ways for the current time being, as 
> new __kprobes are being added (I've seen several in patches flying by in 
> LKML).

We'd rather like to know about all cases where new code is added to 
kprobes (or where we missed to consider some existing code).

It's really just a handful of annotations, the brunt of which is internal 
to kprobes.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2013-11-11 21:15 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-08 12:52 [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text Masami Hiramatsu
2013-11-08 12:52 ` [PATCH -tip RFC 1/2] kprobes: Prohibit probing on .entry.text code Masami Hiramatsu
2013-11-08 12:52 ` [PATCH -tip RFC 2/2] kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist Masami Hiramatsu
2013-11-11 11:16 ` [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text Ingo Molnar
2013-11-11 17:18   ` Masami Hiramatsu
2013-11-11 17:25     ` Steven Rostedt
2013-11-11 21:15       ` Ingo Molnar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).