linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
@ 2017-10-05 23:13 Masami Hiramatsu
  2017-10-05 23:13 ` [RFC PATCH -tip 1/5] kprobes: Use ENOTSUPP instead of ENOSYS Masami Hiramatsu
                   ` (6 more replies)
  0 siblings, 7 replies; 38+ messages in thread
From: Masami Hiramatsu @ 2017-10-05 23:13 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Peter Zijlstra, Alexei Starovoitov,
	Ananth N Mavinakayanahalli, Paul E . McKenney, Steven Rostedt,
	Thomas Gleixner, LKML, H . Peter Anvin, Anil S Keshavamurthy,
	David S . Miller, Kees Cook, Ian McDonald, Vlad Yasevich,
	Stephen Hemminger

Hi,

This series abolishes jprobe APIs and remove or disable related
code. This is a preparation of removing all jprobe code (including
kprobe's break_handler.)
I'm not so sure how many jprobe users still exists, but
please migrate your tool to trace-event or perf-probe.

As we discussed this thread ( https://lkml.org/lkml/2017/10/2/386 ),
we decided to remove jprobe.

Nowadays ftrace and other tracing features are enough matured
to replace jprobe use-cases. Users can safely use ftrace and
perf probe etc. for their use cases. So we have better way.
IOW, jprobe finished its task.

People who still use jprobe, must migrate to other tracing features.
Please consider to migrate your tool to following options.

- Use trace-event to trace target function with arguments
  trace-event is a low-overhead (and almost no visible overhead if it
  is off) statically defined event interface. You can define new events
  and trace it via ftrace or any other tracing tools.
  See following urls,
  - https://lwn.net/Articles/379903/
  - https://lwn.net/Articles/381064/
  - https://lwn.net/Articles/383362/

- Use ftrace dynamic events (kprobe event) with perf-probe
  If you build your kernel with debug info (CONFIG_DEBUG_INFO), you can
  find which register/stack is assigned to which local variable or arguments
  by using perf-probe and set up new event to trace it.
  See following documents,
  - Documentation/trace/kprobetrace.txt
  - Documentation/trace/events.txt
  - tools/perf/Documentation/perf-probe.txt

As far as I can see, tcp probe, dccp probe, sctp probe and lkdtm
are using jprobe to probe function. Please consider to migrate.

Thank you,

---

Masami Hiramatsu (5):
      kprobes: Use ENOTSUPP instead of ENOSYS
      kprobes: Abolish jprobe APIs
      kprobes: Disable jprobe test code
      kprobes: Remove jprobe sample code
      kprobes: docs: Remove jprobe related document


 Documentation/kprobes.txt        |  153 +++++++++++++-------------------------
 include/linux/kprobes.h          |   52 ++++++-------
 kernel/kprobes.c                 |    6 +
 kernel/test_kprobes.c            |    9 ++
 samples/kprobes/Makefile         |    2 
 samples/kprobes/jprobe_example.c |   67 -----------------
 6 files changed, 88 insertions(+), 201 deletions(-)
 delete mode 100644 samples/kprobes/jprobe_example.c

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

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

* [RFC PATCH -tip 1/5] kprobes: Use ENOTSUPP instead of ENOSYS
  2017-10-05 23:13 [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
@ 2017-10-05 23:13 ` Masami Hiramatsu
  2017-10-20  8:57   ` Ingo Molnar
  2017-10-05 23:14 ` [RFC PATCH -tip 2/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 38+ messages in thread
From: Masami Hiramatsu @ 2017-10-05 23:13 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Peter Zijlstra, Alexei Starovoitov,
	Ananth N Mavinakayanahalli, Paul E . McKenney, Steven Rostedt,
	Thomas Gleixner, LKML, H . Peter Anvin, Anil S Keshavamurthy,
	David S . Miller, Kees Cook, Ian McDonald, Vlad Yasevich,
	Stephen Hemminger

Use ENOTSUPP instead of ENOSYS because ENOSYS is reserved
only for invalid syscall number.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 include/linux/kprobes.h |   16 ++++++++--------
 kernel/kprobes.c        |    4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index bd2684700b74..39d558ec71c6 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -431,11 +431,11 @@ static inline struct kprobe *kprobe_running(void)
 }
 static inline int register_kprobe(struct kprobe *p)
 {
-	return -ENOSYS;
+	return -ENOTSUPP;
 }
 static inline int register_kprobes(struct kprobe **kps, int num)
 {
-	return -ENOSYS;
+	return -ENOTSUPP;
 }
 static inline void unregister_kprobe(struct kprobe *p)
 {
@@ -445,11 +445,11 @@ static inline void unregister_kprobes(struct kprobe **kps, int num)
 }
 static inline int register_jprobe(struct jprobe *p)
 {
-	return -ENOSYS;
+	return -ENOTSUPP;
 }
 static inline int register_jprobes(struct jprobe **jps, int num)
 {
-	return -ENOSYS;
+	return -ENOTSUPP;
 }
 static inline void unregister_jprobe(struct jprobe *p)
 {
@@ -462,11 +462,11 @@ static inline void jprobe_return(void)
 }
 static inline int register_kretprobe(struct kretprobe *rp)
 {
-	return -ENOSYS;
+	return -ENOTSUPP;
 }
 static inline int register_kretprobes(struct kretprobe **rps, int num)
 {
-	return -ENOSYS;
+	return -ENOTSUPP;
 }
 static inline void unregister_kretprobe(struct kretprobe *rp)
 {
@@ -479,11 +479,11 @@ static inline void kprobe_flush_task(struct task_struct *tk)
 }
 static inline int disable_kprobe(struct kprobe *kp)
 {
-	return -ENOSYS;
+	return -ENOTSUPP;
 }
 static inline int enable_kprobe(struct kprobe *kp)
 {
-	return -ENOSYS;
+	return -ENOTSUPP;
 }
 #endif /* CONFIG_KPROBES */
 static inline int disable_kretprobe(struct kretprobe *rp)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2d28377a0e32..5e977d3b712a 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2021,13 +2021,13 @@ EXPORT_SYMBOL_GPL(unregister_kretprobes);
 #else /* CONFIG_KRETPROBES */
 int register_kretprobe(struct kretprobe *rp)
 {
-	return -ENOSYS;
+	return -ENOTSUPP;
 }
 EXPORT_SYMBOL_GPL(register_kretprobe);
 
 int register_kretprobes(struct kretprobe **rps, int num)
 {
-	return -ENOSYS;
+	return -ENOTSUPP;
 }
 EXPORT_SYMBOL_GPL(register_kretprobes);
 

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

* [RFC PATCH -tip 2/5] kprobes: Abolish jprobe APIs
  2017-10-05 23:13 [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
  2017-10-05 23:13 ` [RFC PATCH -tip 1/5] kprobes: Use ENOTSUPP instead of ENOSYS Masami Hiramatsu
@ 2017-10-05 23:14 ` Masami Hiramatsu
  2017-10-20 12:26   ` [tip:perf/core] kprobes: Disable the jprobes APIs tip-bot for Masami Hiramatsu
  2017-10-05 23:15 ` [RFC PATCH -tip 3/5] kprobes: Disable jprobe test code Masami Hiramatsu
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 38+ messages in thread
From: Masami Hiramatsu @ 2017-10-05 23:14 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Peter Zijlstra, Alexei Starovoitov,
	Ananth N Mavinakayanahalli, Paul E . McKenney, Steven Rostedt,
	Thomas Gleixner, LKML, H . Peter Anvin, Anil S Keshavamurthy,
	David S . Miller, Kees Cook, Ian McDonald, Vlad Yasevich,
	Stephen Hemminger

Abolish jprobe APIs and comment out the jprobe API function
code. This is a preparation of removing all jprobe related
code (including kprobe's break_handler)

Nowadays ftrace and other tracing features are enough matured
to replace jprobe use-cases. Users can safely use ftrace and
perf probe etc. for their use cases.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 include/linux/kprobes.h |   40 ++++++++++++++++++----------------------
 kernel/kprobes.c        |    2 ++
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 39d558ec71c6..47aaecd0f702 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -391,10 +391,6 @@ int register_kprobes(struct kprobe **kps, int num);
 void unregister_kprobes(struct kprobe **kps, int num);
 int setjmp_pre_handler(struct kprobe *, struct pt_regs *);
 int longjmp_break_handler(struct kprobe *, struct pt_regs *);
-int register_jprobe(struct jprobe *p);
-void unregister_jprobe(struct jprobe *p);
-int register_jprobes(struct jprobe **jps, int num);
-void unregister_jprobes(struct jprobe **jps, int num);
 void jprobe_return(void);
 unsigned long arch_deref_entry_point(void *);
 
@@ -443,20 +439,6 @@ static inline void unregister_kprobe(struct kprobe *p)
 static inline void unregister_kprobes(struct kprobe **kps, int num)
 {
 }
-static inline int register_jprobe(struct jprobe *p)
-{
-	return -ENOTSUPP;
-}
-static inline int register_jprobes(struct jprobe **jps, int num)
-{
-	return -ENOTSUPP;
-}
-static inline void unregister_jprobe(struct jprobe *p)
-{
-}
-static inline void unregister_jprobes(struct jprobe **jps, int num)
-{
-}
 static inline void jprobe_return(void)
 {
 }
@@ -486,6 +468,20 @@ static inline int enable_kprobe(struct kprobe *kp)
 	return -ENOTSUPP;
 }
 #endif /* CONFIG_KPROBES */
+static inline int __deprecated register_jprobe(struct jprobe *p)
+{
+	return -ENOTSUPP;
+}
+static inline int __deprecated register_jprobes(struct jprobe **jps, int num)
+{
+	return -ENOTSUPP;
+}
+static inline void __deprecated unregister_jprobe(struct jprobe *p)
+{
+}
+static inline void __deprecated unregister_jprobes(struct jprobe **jps, int num)
+{
+}
 static inline int disable_kretprobe(struct kretprobe *rp)
 {
 	return disable_kprobe(&rp->kp);
@@ -494,13 +490,13 @@ static inline int enable_kretprobe(struct kretprobe *rp)
 {
 	return enable_kprobe(&rp->kp);
 }
-static inline int disable_jprobe(struct jprobe *jp)
+static inline int __deprecated disable_jprobe(struct jprobe *jp)
 {
-	return disable_kprobe(&jp->kp);
+	return -ENOTSUPP;
 }
-static inline int enable_jprobe(struct jprobe *jp)
+static inline int __deprecated enable_jprobe(struct jprobe *jp)
 {
-	return enable_kprobe(&jp->kp);
+	return -ENOTSUPP;
 }
 
 #ifndef CONFIG_KPROBES
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 5e977d3b712a..4ab1fb52194f 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1772,6 +1772,7 @@ unsigned long __weak arch_deref_entry_point(void *entry)
 	return (unsigned long)entry;
 }
 
+#if 0
 int register_jprobes(struct jprobe **jps, int num)
 {
 	int ret = 0, i;
@@ -1840,6 +1841,7 @@ void unregister_jprobes(struct jprobe **jps, int num)
 	}
 }
 EXPORT_SYMBOL_GPL(unregister_jprobes);
+#endif
 
 #ifdef CONFIG_KRETPROBES
 /*

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

* [RFC PATCH -tip 3/5] kprobes: Disable jprobe test code
  2017-10-05 23:13 [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
  2017-10-05 23:13 ` [RFC PATCH -tip 1/5] kprobes: Use ENOTSUPP instead of ENOSYS Masami Hiramatsu
  2017-10-05 23:14 ` [RFC PATCH -tip 2/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
@ 2017-10-05 23:15 ` Masami Hiramatsu
  2017-10-20 12:26   ` [tip:perf/core] kprobes: Disable the jprobes " tip-bot for Masami Hiramatsu
  2017-10-05 23:15 ` [RFC PATCH -tip 4/5] kprobes: Remove jprobe sample code Masami Hiramatsu
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 38+ messages in thread
From: Masami Hiramatsu @ 2017-10-05 23:15 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Peter Zijlstra, Alexei Starovoitov,
	Ananth N Mavinakayanahalli, Paul E . McKenney, Steven Rostedt,
	Thomas Gleixner, LKML, H . Peter Anvin, Anil S Keshavamurthy,
	David S . Miller, Kees Cook, Ian McDonald, Vlad Yasevich,
	Stephen Hemminger

Disable jprobe test code because jprobe is deprecated.
This code will be completely removed when jprobe code
is removed.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/test_kprobes.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/test_kprobes.c b/kernel/test_kprobes.c
index 47106a1e645a..dd53e354f630 100644
--- a/kernel/test_kprobes.c
+++ b/kernel/test_kprobes.c
@@ -22,7 +22,7 @@
 
 #define div_factor 3
 
-static u32 rand1, preh_val, posth_val, jph_val;
+static u32 rand1, preh_val, posth_val;
 static int errors, handler_errors, num_tests;
 static u32 (*target)(u32 value);
 static u32 (*target2)(u32 value);
@@ -162,6 +162,9 @@ static int test_kprobes(void)
 
 }
 
+#if 0
+static u32 jph_val;
+
 static u32 j_kprobe_target(u32 value)
 {
 	if (preemptible()) {
@@ -239,6 +242,10 @@ static int test_jprobes(void)
 
 	return 0;
 }
+#else
+#define test_jprobe() (0)
+#define test_jprobes() (0)
+#endif
 #ifdef CONFIG_KRETPROBES
 static u32 krph_val;
 

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

* [RFC PATCH -tip 4/5] kprobes: Remove jprobe sample code
  2017-10-05 23:13 [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
                   ` (2 preceding siblings ...)
  2017-10-05 23:15 ` [RFC PATCH -tip 3/5] kprobes: Disable jprobe test code Masami Hiramatsu
@ 2017-10-05 23:15 ` Masami Hiramatsu
  2017-10-20 12:27   ` [tip:perf/core] kprobes: Remove the jprobes " tip-bot for Masami Hiramatsu
  2017-10-05 23:16 ` [RFC PATCH -tip 5/5] kprobes: docs: Remove jprobe related document Masami Hiramatsu
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 38+ messages in thread
From: Masami Hiramatsu @ 2017-10-05 23:15 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Peter Zijlstra, Alexei Starovoitov,
	Ananth N Mavinakayanahalli, Paul E . McKenney, Steven Rostedt,
	Thomas Gleixner, LKML, H . Peter Anvin, Anil S Keshavamurthy,
	David S . Miller, Kees Cook, Ian McDonald, Vlad Yasevich,
	Stephen Hemminger

Remove jprobe sample module because jprobe is deprecated.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 samples/kprobes/Makefile         |    2 +
 samples/kprobes/jprobe_example.c |   67 --------------------------------------
 2 files changed, 1 insertion(+), 68 deletions(-)
 delete mode 100644 samples/kprobes/jprobe_example.c

diff --git a/samples/kprobes/Makefile b/samples/kprobes/Makefile
index 68739bc4fc6a..880e54d2c082 100644
--- a/samples/kprobes/Makefile
+++ b/samples/kprobes/Makefile
@@ -1,5 +1,5 @@
 # builds the kprobes example kernel modules;
 # then to use one (as root):  insmod <module_name.ko>
 
-obj-$(CONFIG_SAMPLE_KPROBES) += kprobe_example.o jprobe_example.o
+obj-$(CONFIG_SAMPLE_KPROBES) += kprobe_example.o
 obj-$(CONFIG_SAMPLE_KRETPROBES) += kretprobe_example.o
diff --git a/samples/kprobes/jprobe_example.c b/samples/kprobes/jprobe_example.c
deleted file mode 100644
index e3c0a40909f7..000000000000
--- a/samples/kprobes/jprobe_example.c
+++ /dev/null
@@ -1,67 +0,0 @@
-/*
- * Here's a sample kernel module showing the use of jprobes to dump
- * the arguments of _do_fork().
- *
- * For more information on theory of operation of jprobes, see
- * Documentation/kprobes.txt
- *
- * Build and insert the kernel module as done in the kprobe example.
- * You will see the trace data in /var/log/messages and on the
- * console whenever _do_fork() is invoked to create a new process.
- * (Some messages may be suppressed if syslogd is configured to
- * eliminate duplicate messages.)
- */
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/kprobes.h>
-
-/*
- * Jumper probe for _do_fork.
- * Mirror principle enables access to arguments of the probed routine
- * from the probe handler.
- */
-
-/* Proxy routine having the same arguments as actual _do_fork() routine */
-static long j_do_fork(unsigned long clone_flags, unsigned long stack_start,
-	      unsigned long stack_size, int __user *parent_tidptr,
-	      int __user *child_tidptr, unsigned long tls)
-{
-	pr_info("jprobe: clone_flags = 0x%lx, stack_start = 0x%lx "
-		"stack_size = 0x%lx\n", clone_flags, stack_start, stack_size);
-
-	/* Always end with a call to jprobe_return(). */
-	jprobe_return();
-	return 0;
-}
-
-static struct jprobe my_jprobe = {
-	.entry			= j_do_fork,
-	.kp = {
-		.symbol_name	= "_do_fork",
-	},
-};
-
-static int __init jprobe_init(void)
-{
-	int ret;
-
-	ret = register_jprobe(&my_jprobe);
-	if (ret < 0) {
-		pr_err("register_jprobe failed, returned %d\n", ret);
-		return -1;
-	}
-	pr_info("Planted jprobe at %p, handler addr %p\n",
-	       my_jprobe.kp.addr, my_jprobe.entry);
-	return 0;
-}
-
-static void __exit jprobe_exit(void)
-{
-	unregister_jprobe(&my_jprobe);
-	pr_info("jprobe at %p unregistered\n", my_jprobe.kp.addr);
-}
-
-module_init(jprobe_init)
-module_exit(jprobe_exit)
-MODULE_LICENSE("GPL");

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

* [RFC PATCH -tip 5/5] kprobes: docs: Remove jprobe related document
  2017-10-05 23:13 [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
                   ` (3 preceding siblings ...)
  2017-10-05 23:15 ` [RFC PATCH -tip 4/5] kprobes: Remove jprobe sample code Masami Hiramatsu
@ 2017-10-05 23:16 ` Masami Hiramatsu
  2017-10-20 12:27   ` [tip:perf/core] kprobes/docs: Remove jprobes related documents tip-bot for Masami Hiramatsu
  2017-10-05 23:35 ` [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Kees Cook
  2017-10-20 12:22 ` Ingo Molnar
  6 siblings, 1 reply; 38+ messages in thread
From: Masami Hiramatsu @ 2017-10-05 23:16 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Peter Zijlstra, Alexei Starovoitov,
	Ananth N Mavinakayanahalli, Paul E . McKenney, Steven Rostedt,
	Thomas Gleixner, LKML, H . Peter Anvin, Anil S Keshavamurthy,
	David S . Miller, Kees Cook, Ian McDonald, Vlad Yasevich,
	Stephen Hemminger

Remove jprobe related documentations from kprobes.txt.
It also add some migration advice for the people who still
using jprobe.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Documentation/kprobes.txt |  153 +++++++++++++++------------------------------
 1 file changed, 51 insertions(+), 102 deletions(-)

diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
index 2335715bf471..e8dceb1435e0 100644
--- a/Documentation/kprobes.txt
+++ b/Documentation/kprobes.txt
@@ -8,7 +8,7 @@ Kernel Probes (Kprobes)
 
 .. CONTENTS
 
-  1. Concepts: Kprobes, Jprobes, Return Probes
+  1. Concepts: Kprobes, and Return Probes
   2. Architectures Supported
   3. Configuring Kprobes
   4. API Reference
@@ -16,12 +16,12 @@ Kernel Probes (Kprobes)
   6. Probe Overhead
   7. TODO
   8. Kprobes Example
-  9. Jprobes Example
-  10. Kretprobes Example
+  9. Kretprobes Example
+  10. Deprecated feature
   Appendix A: The kprobes debugfs interface
   Appendix B: The kprobes sysctl interface
 
-Concepts: Kprobes, Jprobes, Return Probes
+Concepts: Kprobes and Return Probes
 =========================================
 
 Kprobes enables you to dynamically break into any kernel routine and
@@ -32,12 +32,10 @@ routine to be invoked when the breakpoint is hit.
 .. [1] some parts of the kernel code can not be trapped, see
        :ref:`kprobes_blacklist`)
 
-There are currently three types of probes: kprobes, jprobes, and
-kretprobes (also called return probes).  A kprobe can be inserted
-on virtually any instruction in the kernel.  A jprobe is inserted at
-the entry to a kernel function, and provides convenient access to the
-function's arguments.  A return probe fires when a specified function
-returns.
+There are currently two types of probes: kprobes, and kretprobes
+(also called return probes).  A kprobe can be inserted on virtually
+any instruction in the kernel.  A return probe fires when a specified
+function returns.
 
 In the typical case, Kprobes-based instrumentation is packaged as
 a kernel module.  The module's init function installs ("registers")
@@ -82,45 +80,6 @@ After the instruction is single-stepped, Kprobes executes the
 "post_handler," if any, that is associated with the kprobe.
 Execution then continues with the instruction following the probepoint.
 
-How Does a Jprobe Work?
------------------------
-
-A jprobe is implemented using a kprobe that is placed on a function's
-entry point.  It employs a simple mirroring principle to allow
-seamless access to the probed function's arguments.  The jprobe
-handler routine should have the same signature (arg list and return
-type) as the function being probed, and must always end by calling
-the Kprobes function jprobe_return().
-
-Here's how it works.  When the probe is hit, Kprobes makes a copy of
-the saved registers and a generous portion of the stack (see below).
-Kprobes then points the saved instruction pointer at the jprobe's
-handler routine, and returns from the trap.  As a result, control
-passes to the handler, which is presented with the same register and
-stack contents as the probed function.  When it is done, the handler
-calls jprobe_return(), which traps again to restore the original stack
-contents and processor state and switch to the probed function.
-
-By convention, the callee owns its arguments, so gcc may produce code
-that unexpectedly modifies that portion of the stack.  This is why
-Kprobes saves a copy of the stack and restores it after the jprobe
-handler has run.  Up to MAX_STACK_SIZE bytes are copied -- e.g.,
-64 bytes on i386.
-
-Note that the probed function's args may be passed on the stack
-or in registers.  The jprobe will work in either case, so long as the
-handler's prototype matches that of the probed function.
-
-Note that in some architectures (e.g.: arm64 and sparc64) the stack
-copy is not done, as the actual location of stacked parameters may be
-outside of a reasonable MAX_STACK_SIZE value and because that location
-cannot be determined by the jprobes code. In this case the jprobes
-user must be careful to make certain the calling signature of the
-function does not cause parameters to be passed on the stack (e.g.:
-more than eight function arguments, an argument of more than sixteen
-bytes, or more than 64 bytes of argument data, depending on
-architecture).
-
 Return Probes
 -------------
 
@@ -245,8 +204,7 @@ Pre-optimization
 After preparing the detour buffer, Kprobes verifies that none of the
 following situations exist:
 
-- The probe has either a break_handler (i.e., it's a jprobe) or a
-  post_handler.
+- The probe has a post_handler.
 - Other instructions in the optimized region are probed.
 - The probe is disabled.
 
@@ -331,7 +289,7 @@ rejects registering it, if the given address is in the blacklist.
 Architectures Supported
 =======================
 
-Kprobes, jprobes, and return probes are implemented on the following
+Kprobes and return probes are implemented on the following
 architectures:
 
 - i386 (Supports jump optimization)
@@ -446,27 +404,6 @@ architecture-specific trap number associated with the fault (e.g.,
 on i386, 13 for a general protection fault or 14 for a page fault).
 Returns 1 if it successfully handled the exception.
 
-register_jprobe
----------------
-
-::
-
-	#include <linux/kprobes.h>
-	int register_jprobe(struct jprobe *jp)
-
-Sets a breakpoint at the address jp->kp.addr, which must be the address
-of the first instruction of a function.  When the breakpoint is hit,
-Kprobes runs the handler whose address is jp->entry.
-
-The handler should have the same arg list and return type as the probed
-function; and just before it returns, it must call jprobe_return().
-(The handler never actually returns, since jprobe_return() returns
-control to Kprobes.)  If the probed function is declared asmlinkage
-or anything else that affects how args are passed, the handler's
-declaration must match.
-
-register_jprobe() returns 0 on success, or a negative errno otherwise.
-
 register_kretprobe
 ------------------
 
@@ -513,7 +450,6 @@ unregister_*probe
 
 	#include <linux/kprobes.h>
 	void unregister_kprobe(struct kprobe *kp);
-	void unregister_jprobe(struct jprobe *jp);
 	void unregister_kretprobe(struct kretprobe *rp);
 
 Removes the specified probe.  The unregister function can be called
@@ -532,7 +468,6 @@ register_*probes
 	#include <linux/kprobes.h>
 	int register_kprobes(struct kprobe **kps, int num);
 	int register_kretprobes(struct kretprobe **rps, int num);
-	int register_jprobes(struct jprobe **jps, int num);
 
 Registers each of the num probes in the specified array.  If any
 error occurs during registration, all probes in the array, up to
@@ -555,7 +490,6 @@ unregister_*probes
 	#include <linux/kprobes.h>
 	void unregister_kprobes(struct kprobe **kps, int num);
 	void unregister_kretprobes(struct kretprobe **rps, int num);
-	void unregister_jprobes(struct jprobe **jps, int num);
 
 Removes each of the num probes in the specified array at once.
 
@@ -574,7 +508,6 @@ disable_*probe
 	#include <linux/kprobes.h>
 	int disable_kprobe(struct kprobe *kp);
 	int disable_kretprobe(struct kretprobe *rp);
-	int disable_jprobe(struct jprobe *jp);
 
 Temporarily disables the specified ``*probe``. You can enable it again by using
 enable_*probe(). You must specify the probe which has been registered.
@@ -587,7 +520,6 @@ enable_*probe
 	#include <linux/kprobes.h>
 	int enable_kprobe(struct kprobe *kp);
 	int enable_kretprobe(struct kretprobe *rp);
-	int enable_jprobe(struct jprobe *jp);
 
 Enables ``*probe`` which has been disabled by disable_*probe(). You must specify
 the probe which has been registered.
@@ -595,12 +527,10 @@ the probe which has been registered.
 Kprobes Features and Limitations
 ================================
 
-Kprobes allows multiple probes at the same address.  Currently,
-however, there cannot be multiple jprobes on the same function at
-the same time.  Also, a probepoint for which there is a jprobe or
-a post_handler cannot be optimized.  So if you install a jprobe,
-or a kprobe with a post_handler, at an optimized probepoint, the
-probepoint will be unoptimized automatically.
+Kprobes allows multiple probes at the same address. Also,
+a probepoint for which there is a post_handler cannot be optimized.
+So if you install a kprobe with a post_handler, at an optimized
+probepoint, the probepoint will be unoptimized automatically.
 
 In general, you can install a probe anywhere in the kernel.
 In particular, you can probe interrupt handlers.  Known exceptions
@@ -662,7 +592,7 @@ We're unaware of other specific cases where this could be a problem.
 If, upon entry to or exit from a function, the CPU is running on
 a stack other than that of the current task, registering a return
 probe on that function may produce undesirable results.  For this
-reason, Kprobes doesn't support return probes (or kprobes or jprobes)
+reason, Kprobes doesn't support return probes (or kprobes)
 on the x86_64 version of __switch_to(); the registration functions
 return -EINVAL.
 
@@ -706,24 +636,24 @@ Probe Overhead
 On a typical CPU in use in 2005, a kprobe hit takes 0.5 to 1.0
 microseconds to process.  Specifically, a benchmark that hits the same
 probepoint repeatedly, firing a simple handler each time, reports 1-2
-million hits per second, depending on the architecture.  A jprobe or
-return-probe hit typically takes 50-75% longer than a kprobe hit.
+million hits per second, depending on the architecture.  A return-probe
+hit typically takes 50-75% longer than a kprobe hit.
 When you have a return probe set on a function, adding a kprobe at
 the entry to that function adds essentially no overhead.
 
 Here are sample overhead figures (in usec) for different architectures::
 
-  k = kprobe; j = jprobe; r = return probe; kr = kprobe + return probe
-  on same function; jr = jprobe + return probe on same function::
+  k = kprobe; r = return probe; kr = kprobe + return probe
+  on same function
 
   i386: Intel Pentium M, 1495 MHz, 2957.31 bogomips
-  k = 0.57 usec; j = 1.00; r = 0.92; kr = 0.99; jr = 1.40
+  k = 0.57 usec; r = 0.92; kr = 0.99
 
   x86_64: AMD Opteron 246, 1994 MHz, 3971.48 bogomips
-  k = 0.49 usec; j = 0.76; r = 0.80; kr = 0.82; jr = 1.07
+  k = 0.49 usec; r = 0.80; kr = 0.82
 
   ppc64: POWER5 (gr), 1656 MHz (SMT disabled, 1 virtual CPU per physical CPU)
-  k = 0.77 usec; j = 1.31; r = 1.26; kr = 1.45; jr = 1.99
+  k = 0.77 usec; r = 1.26; kr = 1.45
 
 Optimized Probe Overhead
 ------------------------
@@ -755,11 +685,6 @@ Kprobes Example
 
 See samples/kprobes/kprobe_example.c
 
-Jprobes Example
-===============
-
-See samples/kprobes/jprobe_example.c
-
 Kretprobes Example
 ==================
 
@@ -772,6 +697,31 @@ For additional information on Kprobes, refer to the following URLs:
 - http://www-users.cs.umn.edu/~boutcher/kprobes/
 - http://www.linuxsymposium.org/2006/linuxsymposium_procv2.pdf (pages 101-115)
 
+Deprecated Features
+===================
+
+Jprobe is now deprecated feature. People who are depending on it, must
+migrate to other tracing features. Please consider to migrate your tool
+to following options.
+
+- Use trace-event to trace target function with arguments
+  trace-event is a low-overhead (and almost no visible overhead if it
+  is off) statically defined event interface. You can define new events
+  and trace it via ftrace or any other tracing tools.
+  See following urls,
+  - https://lwn.net/Articles/379903/
+  - https://lwn.net/Articles/381064/
+  - https://lwn.net/Articles/383362/
+
+- Use ftrace dynamic events (kprobe event) with perf-probe
+  If you build your kernel with debug info (CONFIG_DEBUG_INFO), you can
+  find which register/stack is assigned to which local variable or arguments
+  by using perf-probe and set up new event to trace it.
+  See following documents,
+  - Documentation/trace/kprobetrace.txt
+  - Documentation/trace/events.txt
+  - tools/perf/Documentation/perf-probe.txt
+
 
 The kprobes debugfs interface
 =============================
@@ -783,14 +733,13 @@ under the /sys/kernel/debug/kprobes/ directory (assuming debugfs is mounted at /
 /sys/kernel/debug/kprobes/list: Lists all registered probes on the system::
 
 	c015d71a  k  vfs_read+0x0
-	c011a316  j  do_fork+0x0
 	c03dedc5  r  tcp_v4_rcv+0x0
 
 The first column provides the kernel address where the probe is inserted.
-The second column identifies the type of probe (k - kprobe, r - kretprobe
-and j - jprobe), while the third column specifies the symbol+offset of
-the probe. If the probed function belongs to a module, the module name
-is also specified. Following columns show probe status. If the probe is on
+The second column identifies the type of probe (k - kprobe and r - kretprobe)
+while the third column specifies the symbol+offset of the probe.
+If the probed function belongs to a module, the module name is also
+specified. Following columns show probe status. If the probe is on
 a virtual address that is no longer valid (module init sections, module
 virtual addresses that correspond to modules that've been unloaded),
 such probes are marked with [GONE]. If the probe is temporarily disabled,

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-05 23:13 [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
                   ` (4 preceding siblings ...)
  2017-10-05 23:16 ` [RFC PATCH -tip 5/5] kprobes: docs: Remove jprobe related document Masami Hiramatsu
@ 2017-10-05 23:35 ` Kees Cook
  2017-10-05 23:58   ` Steven Rostedt
  2017-10-06  0:32   ` Masami Hiramatsu
  2017-10-20 12:22 ` Ingo Molnar
  6 siblings, 2 replies; 38+ messages in thread
From: Kees Cook @ 2017-10-05 23:35 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Ingo Molnar, Linus Torvalds, Peter Zijlstra, Alexei Starovoitov,
	Ananth N Mavinakayanahalli, Paul E . McKenney, Steven Rostedt,
	Thomas Gleixner, LKML, H . Peter Anvin, Anil S Keshavamurthy,
	David S . Miller, Ian McDonald, Vlad Yasevich, Stephen Hemminger

On Thu, Oct 5, 2017 at 4:13 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
> Hi,
>
> This series abolishes jprobe APIs and remove or disable related
> code. This is a preparation of removing all jprobe code (including
> kprobe's break_handler.)
> I'm not so sure how many jprobe users still exists, but
> please migrate your tool to trace-event or perf-probe.
>
> As we discussed this thread ( https://lkml.org/lkml/2017/10/2/386 ),
> we decided to remove jprobe.
>
> Nowadays ftrace and other tracing features are enough matured
> to replace jprobe use-cases. Users can safely use ftrace and
> perf probe etc. for their use cases. So we have better way.
> IOW, jprobe finished its task.
>
> People who still use jprobe, must migrate to other tracing features.
> Please consider to migrate your tool to following options.
>
> - Use trace-event to trace target function with arguments
>   trace-event is a low-overhead (and almost no visible overhead if it
>   is off) statically defined event interface. You can define new events
>   and trace it via ftrace or any other tracing tools.
>   See following urls,
>   - https://lwn.net/Articles/379903/
>   - https://lwn.net/Articles/381064/
>   - https://lwn.net/Articles/383362/

It seems this method requires setting up the target trace ahead of time?

> - Use ftrace dynamic events (kprobe event) with perf-probe
>   If you build your kernel with debug info (CONFIG_DEBUG_INFO), you can
>   find which register/stack is assigned to which local variable or arguments
>   by using perf-probe and set up new event to trace it.
>   See following documents,
>   - Documentation/trace/kprobetrace.txt
>   - Documentation/trace/events.txt
>   - tools/perf/Documentation/perf-probe.txt

These seem to be more about setting up probes from userspace.

> As far as I can see, tcp probe, dccp probe, sctp probe and lkdtm
> are using jprobe to probe function. Please consider to migrate.

I'm happy to do so, but I'm quite unfamiliar with how to do this (I
didn't write lkdtm's jprobe code originally). lkdtm just wants to hook
function entry and call it's own function before.

It uses struct jprobe like this:

                .jprobe = {                                     \
                        .kp.symbol_name = _symbol,              \
                        .entry = (kprobe_opcode_t *)_entry,     \
                },                                              \

and defines a bunch of handlers like this for the _symbol and _entry pairs:

                   "do_IRQ",                    jp_do_irq),
...
                   "tasklet_action",            jp_tasklet_action),

where all the handlers look exactly the same (and don't care about arguments):

static unsigned int jp_do_irq(unsigned int irq)
{
        lkdtm_handler();
        jprobe_return();
        return 0;
}

What's the right way to migrate away from jprobe for lkdtm?

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-05 23:35 ` [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Kees Cook
@ 2017-10-05 23:58   ` Steven Rostedt
  2017-10-06  0:06     ` Kees Cook
  2017-10-06  4:49     ` Masami Hiramatsu
  2017-10-06  0:32   ` Masami Hiramatsu
  1 sibling, 2 replies; 38+ messages in thread
From: Steven Rostedt @ 2017-10-05 23:58 UTC (permalink / raw)
  To: Kees Cook
  Cc: Masami Hiramatsu, Ingo Molnar, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Thu, 5 Oct 2017 16:35:22 -0700
Kees Cook <keescook@chromium.org> wrote:

> > As far as I can see, tcp probe, dccp probe, sctp probe and lkdtm
> > are using jprobe to probe function. Please consider to migrate.  
> 
> I'm happy to do so, but I'm quite unfamiliar with how to do this (I
> didn't write lkdtm's jprobe code originally). lkdtm just wants to hook
> function entry and call it's own function before.

That can be done with ftrace. That's how live kernel patching works. It
registers a callback via register_ftrace_function(), and with fentry
(gcc 4.6 and later on x86), you can "hijack" the function. If you don't
modify the regs->ip, then the function you hooked to will be called.

> 
> It uses struct jprobe like this:
> 
>                 .jprobe = {                                     \
>                         .kp.symbol_name = _symbol,              \
>                         .entry = (kprobe_opcode_t *)_entry,     \
>                 },                                              \
> 
> and defines a bunch of handlers like this for the _symbol and _entry pairs:
> 
>                    "do_IRQ",                    jp_do_irq),
> ...
>                    "tasklet_action",            jp_tasklet_action),
> 
> where all the handlers look exactly the same (and don't care about arguments):

Hell, this is really easy then!

> 
> static unsigned int jp_do_irq(unsigned int irq)
> {
>         lkdtm_handler();
>         jprobe_return();
>         return 0;
> }
> 
> What's the right way to migrate away from jprobe for lkdtm?

Perhaps something like:

#include <linux/ftrace.h>

static void lkdtm_callback(unsigned long ip, unsigned long parent_ip,
			struct ftrace_ops *ops, struct pt_regs *regs)
{
	lkdt_handler();
}


static struct ftrace_ops ops = {
	.func		= lkdtm_callback,
};

[..]
	ftrace_set_filter(&ops, "do_IRQ", strlen("do_IRQ"), 0);
	ftrace_set_filter(&ops, "tasklet_action", strlen("tasklet_action"), 0);
	[..]

	/* to add the hook */

	register_ftrace_function(&ops);

Now all functions you set the filter for will be traced.

Oh you may want to check the return status of ftrace_set_filter()
otherwise, if they all fail, you will be tracing all functions.

-- Steve

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-05 23:58   ` Steven Rostedt
@ 2017-10-06  0:06     ` Kees Cook
  2017-10-06  4:49     ` Masami Hiramatsu
  1 sibling, 0 replies; 38+ messages in thread
From: Kees Cook @ 2017-10-06  0:06 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Masami Hiramatsu, Ingo Molnar, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Thu, Oct 5, 2017 at 4:58 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 5 Oct 2017 16:35:22 -0700
> Kees Cook <keescook@chromium.org> wrote:
>
>> > As far as I can see, tcp probe, dccp probe, sctp probe and lkdtm
>> > are using jprobe to probe function. Please consider to migrate.
>>
>> I'm happy to do so, but I'm quite unfamiliar with how to do this (I
>> didn't write lkdtm's jprobe code originally). lkdtm just wants to hook
>> function entry and call it's own function before.
>
> That can be done with ftrace. That's how live kernel patching works. It
> registers a callback via register_ftrace_function(), and with fentry
> (gcc 4.6 and later on x86), you can "hijack" the function. If you don't
> modify the regs->ip, then the function you hooked to will be called.
>
>>
>> It uses struct jprobe like this:
>>
>>                 .jprobe = {                                     \
>>                         .kp.symbol_name = _symbol,              \
>>                         .entry = (kprobe_opcode_t *)_entry,     \
>>                 },                                              \
>>
>> and defines a bunch of handlers like this for the _symbol and _entry pairs:
>>
>>                    "do_IRQ",                    jp_do_irq),
>> ...
>>                    "tasklet_action",            jp_tasklet_action),
>>
>> where all the handlers look exactly the same (and don't care about arguments):
>
> Hell, this is really easy then!
>
>>
>> static unsigned int jp_do_irq(unsigned int irq)
>> {
>>         lkdtm_handler();
>>         jprobe_return();
>>         return 0;
>> }
>>
>> What's the right way to migrate away from jprobe for lkdtm?
>
> Perhaps something like:
>
> #include <linux/ftrace.h>
>
> static void lkdtm_callback(unsigned long ip, unsigned long parent_ip,
>                         struct ftrace_ops *ops, struct pt_regs *regs)
> {
>         lkdt_handler();
> }
>
>
> static struct ftrace_ops ops = {
>         .func           = lkdtm_callback,
> };
>
> [..]
>         ftrace_set_filter(&ops, "do_IRQ", strlen("do_IRQ"), 0);
>         ftrace_set_filter(&ops, "tasklet_action", strlen("tasklet_action"), 0);
>         [..]
>
>         /* to add the hook */
>
>         register_ftrace_function(&ops);
>
> Now all functions you set the filter for will be traced.
>
> Oh you may want to check the return status of ftrace_set_filter()
> otherwise, if they all fail, you will be tracing all functions.

Ah-ha! Perfect, thank you! I'll give this a shot.

-Kees

-- 
Kees Cook
Pixel Security

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-05 23:35 ` [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Kees Cook
  2017-10-05 23:58   ` Steven Rostedt
@ 2017-10-06  0:32   ` Masami Hiramatsu
  2017-10-06  1:11     ` Steven Rostedt
  1 sibling, 1 reply; 38+ messages in thread
From: Masami Hiramatsu @ 2017-10-06  0:32 UTC (permalink / raw)
  To: Kees Cook
  Cc: Ingo Molnar, Linus Torvalds, Peter Zijlstra, Alexei Starovoitov,
	Ananth N Mavinakayanahalli, Paul E . McKenney, Steven Rostedt,
	Thomas Gleixner, LKML, H . Peter Anvin, Anil S Keshavamurthy,
	David S . Miller, Ian McDonald, Vlad Yasevich, Stephen Hemminger

On Thu, 5 Oct 2017 16:35:22 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Thu, Oct 5, 2017 at 4:13 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > Hi,
> >
> > This series abolishes jprobe APIs and remove or disable related
> > code. This is a preparation of removing all jprobe code (including
> > kprobe's break_handler.)
> > I'm not so sure how many jprobe users still exists, but
> > please migrate your tool to trace-event or perf-probe.
> >
> > As we discussed this thread ( https://lkml.org/lkml/2017/10/2/386 ),
> > we decided to remove jprobe.
> >
> > Nowadays ftrace and other tracing features are enough matured
> > to replace jprobe use-cases. Users can safely use ftrace and
> > perf probe etc. for their use cases. So we have better way.
> > IOW, jprobe finished its task.
> >
> > People who still use jprobe, must migrate to other tracing features.
> > Please consider to migrate your tool to following options.
> >
> > - Use trace-event to trace target function with arguments
> >   trace-event is a low-overhead (and almost no visible overhead if it
> >   is off) statically defined event interface. You can define new events
> >   and trace it via ftrace or any other tracing tools.
> >   See following urls,
> >   - https://lwn.net/Articles/379903/
> >   - https://lwn.net/Articles/381064/
> >   - https://lwn.net/Articles/383362/
> 
> It seems this method requires setting up the target trace ahead of time?
> 
> > - Use ftrace dynamic events (kprobe event) with perf-probe
> >   If you build your kernel with debug info (CONFIG_DEBUG_INFO), you can
> >   find which register/stack is assigned to which local variable or arguments
> >   by using perf-probe and set up new event to trace it.
> >   See following documents,
> >   - Documentation/trace/kprobetrace.txt
> >   - Documentation/trace/events.txt
> >   - tools/perf/Documentation/perf-probe.txt
> 
> These seem to be more about setting up probes from userspace.
> 
> > As far as I can see, tcp probe, dccp probe, sctp probe and lkdtm
> > are using jprobe to probe function. Please consider to migrate.
> 
> I'm happy to do so, but I'm quite unfamiliar with how to do this (I
> didn't write lkdtm's jprobe code originally). lkdtm just wants to hook
> function entry and call it's own function before.
> 
> It uses struct jprobe like this:
> 
>                 .jprobe = {                                     \
>                         .kp.symbol_name = _symbol,              \
>                         .entry = (kprobe_opcode_t *)_entry,     \
>                 },                                              \
> 
> and defines a bunch of handlers like this for the _symbol and _entry pairs:
> 
>                    "do_IRQ",                    jp_do_irq),
> ...
>                    "tasklet_action",            jp_tasklet_action),
> 
> where all the handlers look exactly the same (and don't care about arguments):

If so, you can just change it to kprobes instead of jprobe.
e.g.

	.kprobe = {
		.symbol_name = _symbol,
		.pre_handler = _entry,
	}

and 
	"do_IRQ",	kp_pre_handler),
...
	"tasklet_action",	kp_pre_handler),

both kp_do_irq and kp_tasklet_action has same signature, so you can
use same function like 

static unsigned int kp_pre_handler(struct kprobe *kp, struct pt_regs *regs)
{
        lkdtm_handler();
        return 0;
}

I think using ftrace gives you lower latency, but you need to depend on
CONFIG_FUNCTION_TRACER instead of CONFIG_KPROBES.

Anyway, please choose either one of those :)

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-06  0:32   ` Masami Hiramatsu
@ 2017-10-06  1:11     ` Steven Rostedt
  2017-10-06  4:47       ` Masami Hiramatsu
  0 siblings, 1 reply; 38+ messages in thread
From: Steven Rostedt @ 2017-10-06  1:11 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Kees Cook, Ingo Molnar, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Fri, 6 Oct 2017 09:32:52 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> I think using ftrace gives you lower latency, but you need to depend on
> CONFIG_FUNCTION_TRACER instead of CONFIG_KPROBES.

Which shouldn't be an issue, since all distros now have that enabled.

-- Steve

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-06  1:11     ` Steven Rostedt
@ 2017-10-06  4:47       ` Masami Hiramatsu
  0 siblings, 0 replies; 38+ messages in thread
From: Masami Hiramatsu @ 2017-10-06  4:47 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Kees Cook, Ingo Molnar, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Thu, 5 Oct 2017 21:11:30 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 6 Oct 2017 09:32:52 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > I think using ftrace gives you lower latency, but you need to depend on
> > CONFIG_FUNCTION_TRACER instead of CONFIG_KPROBES.
> 
> Which shouldn't be an issue, since all distros now have that enabled.

Yeah, so both are good option :)

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-05 23:58   ` Steven Rostedt
  2017-10-06  0:06     ` Kees Cook
@ 2017-10-06  4:49     ` Masami Hiramatsu
  2017-10-06 12:58       ` Steven Rostedt
  2017-10-06 15:34       ` Steven Rostedt
  1 sibling, 2 replies; 38+ messages in thread
From: Masami Hiramatsu @ 2017-10-06  4:49 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Kees Cook, Masami Hiramatsu, Ingo Molnar, Linus Torvalds,
	Peter Zijlstra, Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Thu, 5 Oct 2017 19:58:08 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 5 Oct 2017 16:35:22 -0700
> Kees Cook <keescook@chromium.org> wrote:
> 
> > > As far as I can see, tcp probe, dccp probe, sctp probe and lkdtm
> > > are using jprobe to probe function. Please consider to migrate.  
> > 
> > I'm happy to do so, but I'm quite unfamiliar with how to do this (I
> > didn't write lkdtm's jprobe code originally). lkdtm just wants to hook
> > function entry and call it's own function before.
> 
> That can be done with ftrace. That's how live kernel patching works. It
> registers a callback via register_ftrace_function(), and with fentry
> (gcc 4.6 and later on x86), you can "hijack" the function. If you don't
> modify the regs->ip, then the function you hooked to will be called.
> 
> > 
> > It uses struct jprobe like this:
> > 
> >                 .jprobe = {                                     \
> >                         .kp.symbol_name = _symbol,              \
> >                         .entry = (kprobe_opcode_t *)_entry,     \
> >                 },                                              \
> > 
> > and defines a bunch of handlers like this for the _symbol and _entry pairs:
> > 
> >                    "do_IRQ",                    jp_do_irq),
> > ...
> >                    "tasklet_action",            jp_tasklet_action),
> > 
> > where all the handlers look exactly the same (and don't care about arguments):
> 
> Hell, this is really easy then!
> 
> > 
> > static unsigned int jp_do_irq(unsigned int irq)
> > {
> >         lkdtm_handler();
> >         jprobe_return();
> >         return 0;
> > }
> > 
> > What's the right way to migrate away from jprobe for lkdtm?
> 
> Perhaps something like:
> 
> #include <linux/ftrace.h>
> 
> static void lkdtm_callback(unsigned long ip, unsigned long parent_ip,
> 			struct ftrace_ops *ops, struct pt_regs *regs)
> {
> 	lkdt_handler();
> }
> 
> 
> static struct ftrace_ops ops = {
> 	.func		= lkdtm_callback,
> };
> 
> [..]
> 	ftrace_set_filter(&ops, "do_IRQ", strlen("do_IRQ"), 0);
> 	ftrace_set_filter(&ops, "tasklet_action", strlen("tasklet_action"), 0);
> 	[..]
> 
> 	/* to add the hook */
> 
> 	register_ftrace_function(&ops);
> 
> Now all functions you set the filter for will be traced.
> 
> Oh you may want to check the return status of ftrace_set_filter()
> otherwise, if they all fail, you will be tracing all functions.

Steve, could you write a documentation how to use ftrace callback?
I think I should update the Documentation/kprobes.txt so that jprobe
user can easily migrate on that.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-06  4:49     ` Masami Hiramatsu
@ 2017-10-06 12:58       ` Steven Rostedt
  2017-10-06 15:34       ` Steven Rostedt
  1 sibling, 0 replies; 38+ messages in thread
From: Steven Rostedt @ 2017-10-06 12:58 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Kees Cook, Ingo Molnar, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Fri, 6 Oct 2017 13:49:59 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:


> Steve, could you write a documentation how to use ftrace callback?
> I think I should update the Documentation/kprobes.txt so that jprobe
> user can easily migrate on that.

Sure, I'll add that to my todo list.

-- Steve

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-06  4:49     ` Masami Hiramatsu
  2017-10-06 12:58       ` Steven Rostedt
@ 2017-10-06 15:34       ` Steven Rostedt
  2017-10-07  5:24         ` Stafford Horne
                           ` (3 more replies)
  1 sibling, 4 replies; 38+ messages in thread
From: Steven Rostedt @ 2017-10-06 15:34 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Kees Cook, Ingo Molnar, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Fri, 6 Oct 2017 13:49:59 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Steve, could you write a documentation how to use ftrace callback?
> I think I should update the Documentation/kprobes.txt so that jprobe
> user can easily migrate on that.

I decided to do this now. Here's a first draft. What do you think?

-- Steve

		Using ftrace to hook to functions
		=================================

Copyright 2017 VMware Inc.
   Author:   Steven Rostedt <srostedt@goodmis.org>
  License:   The GNU Free Documentation License, Version 1.2
               (dual licensed under the GPL v2)

Written for: 4.14

Introduction
------------

The ftrace infrastructure was originially created to attach hooks to the
beginning of functions in order to record and trace the flow of the kernel.
But hooks to the start of a function can have other use cases. Either
for live kernel patching, or for security monitoring. This document describes
how to use ftrace to implement your own function hooks.


The ftrace context
==================

WARNING: The ability to add a callback to almost any function within the
kernel comes with risks. A callback can be called from any context
(normal, softirq, irq, and NMI). Callbacks can also be called just before
going to idle, during CPU bring up and takedown, or going to user space.
This requires extra care to what can be done inside a callback. A callback
can be called outside the protective scope of RCU.

The ftrace infrastructure has some protections agains recursions and RCU
but one must still be very careful how they use the callbacks.


The ftrace_ops structure
========================

To register a function callback, a ftrace_ops is required. This structure
is used to tell ftrace what function should be called as the callback
as well as what protections the callback will perform and not require
ftrace to handle.

There are only two fields that are needed to be set when registering
an ftrace_ops with ftrace. The rest should be NULL.

struct ftrace_ops ops = {
       .func			= my_callback_func,
       .flags			= MY_FTRACE_FLAGS
       .private			= any_private_data_structure,
};

Both .flags and .private are optional. Only .func is required.

To enable tracing call:

  register_ftrace_function(&ops);

To disable tracing call:

  unregister_ftrace_function(@ops);


The callback function
=====================

The prototype of the callback function is as follows (as of v4.14):

 void callback_func(unsigned long ip, unsigned long parent_ip,
		    struct ftrace_ops *op, struct pt_regs *regs);

@ip - This is the instruction pointer of the function that is being traced.
      (where the fentry or mcount is within the function)

@parent_ip - This is the instruction pointer of the function that called the
      the function being traced (where the call of the function occurred).

@op - This is a pointer to ftrace_ops that was used to register the callback.
      This can be used to pass data to the callback via the private pointer.

@regs - If the FTRACE_OPS_FL_SAVE_REGS or FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED
      flags are set in the ftrace_ops structure, then this will be pointing
      to the pt_regs structure like it would be if an breakpoint was placed
      at the start of the function where ftrace was tracing. Otherwise it
      either contains garbage, or NULL.


The ftrace FLAGS
================

The ftrace_ops flags are all defined and documented in include/linux/ftrace.h.
Some of the flags are used for internal infrastructure of ftrace, but the
ones that users should be aware of are the following:

(All of these are prefixed with FTRACE_OPS_FL_)

PER_CPU - When set, the callback can be enabled or disabled per cpu with the
      following functions:

      void ftrace_function_local_enable(struct ftrace_ops *ops);
      void ftrace_function_local_disable(struct ftrace_ops *ops);

      These two functions must be called with preemption disabled.

SAVE_REGS - If the callback requires reading or modifying the pt_regs
      passed to the callback, then it must set this flag. Registering
      a ftrace_ops with this flag set on an architecture that does not
      support passing of pt_regs to the callback, will fail.

SAVE_REGS_IF_SUPPORTED - Similar to SAVE_REGS but the registering of a
      ftrace_ops on an architecture that does not support passing of regs
      will not fail with this flag set. But the callback must check if
      regs is NULL or not to determine if the architecture supports it.

RECURSION_SAFE - By default, a wrapper is added around the callback to
      make sure that recursion of the function does not occur. That is
      if a function within the callback itself is also traced, ftrace
      will prevent the callback from being called again. But this wrapper
      adds some overhead, and if the callback is safe from recursion,
      it can set this flag to disable the ftrace protection.

IPMODIFY - Requires SAVE_REGS set. If the callback is to "hijack" the
      traced function (have another function called instead of the traced
      function), it requires setting this flag. This is what live kernel
      patches uses. Without this flag the pt_regs->ip can not be modified.
      Note, only one ftrace_ops with IPMODIFY set may be registered to
      any given function at a time.

RCU - If this is set, then the callback will only be called by functions
      where RCU is "watching". This is required if the callback function
      performs any rcu_read_lock() operation.


Filtering what functions to trace
=================================

If a callback is only to be called from specific functions, a filter must be
set up. The filters are added by name, or ip if it is known.

 int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
		       int len, int reset);

 @ops - the ops to set the filter with
 @buf - the string that holds the function filter text.
 @len - the length of the string.
 @reset - non zero to reset all filters before applying this filter.

 Filters denote which functions should be enabled when tracing is enabled.
 If @buf is NULL and reset is set, all functions will be enabled for tracing.


The @buf can also be a glob expression to enable all functions that
match a specific pattern.

To just trace the schedule function:

 ret = ftrace_set_filter(&ops, "schedule", strlen("schedule"), 0);

To add more functions, call the ftrace_set_filter() more than once with the
@reset parameter set to zero. To remove the current filter and replace it
with new functions to trace, have @reset be non zero.

Sometimes more than one function has the same name. To trace just a specific
function in this case, ftrace_set_filter_ip() can be used.

 ret = ftrace_set_filter_ip(&ops, ip, 0, 0);

Although the ip must be the address where the call to fentry or mcount is
located in the function.

If a glob is used to set the filter, to remove unwanted matches the
ftrace_set_notrace() can also be used.

  int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
			 int len, int reset);

This takes the same parameters as ftrace_set_filter() but will add the
functions it finds to not be traced. This doesn't remove them from the
filter itself, but keeps them from being traced. If @reset is set,
the filter is cleaded but the functions that match @buf will still not
be traced (the callback will not be called on those functions).

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-06 15:34       ` Steven Rostedt
@ 2017-10-07  5:24         ` Stafford Horne
  2017-10-09 16:48           ` Steven Rostedt
  2017-10-07  8:55         ` Ingo Molnar
                           ` (2 subsequent siblings)
  3 siblings, 1 reply; 38+ messages in thread
From: Stafford Horne @ 2017-10-07  5:24 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Masami Hiramatsu, Kees Cook, Ingo Molnar, Linus Torvalds,
	Peter Zijlstra, Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

Hello,

Nice read, see some comments below

On Fri, Oct 06, 2017 at 11:34:30AM -0400, Steven Rostedt wrote:
> On Fri, 6 Oct 2017 13:49:59 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Steve, could you write a documentation how to use ftrace callback?
> > I think I should update the Documentation/kprobes.txt so that jprobe
> > user can easily migrate on that.
> 
> I decided to do this now. Here's a first draft. What do you think?
> 
> -- Steve
> 
> 		Using ftrace to hook to functions
> 		=================================
> 
> Copyright 2017 VMware Inc.
>    Author:   Steven Rostedt <srostedt@goodmis.org>
>   License:   The GNU Free Documentation License, Version 1.2
>                (dual licensed under the GPL v2)
> 
> Written for: 4.14
> 
> Introduction
> ------------
> 
> The ftrace infrastructure was originially created to attach hooks to the
> beginning of functions in order to record and trace the flow of the kernel.
> But hooks to the start of a function can have other use cases. Either
> for live kernel patching, or for security monitoring. This document describes
> how to use ftrace to implement your own function hooks.
> 
> 
> The ftrace context
> ==================
> 
> WARNING: The ability to add a callback to almost any function within the
> kernel comes with risks. A callback can be called from any context
> (normal, softirq, irq, and NMI). Callbacks can also be called just before
> going to idle, during CPU bring up and takedown, or going to user space.
> This requires extra care to what can be done inside a callback. A callback
> can be called outside the protective scope of RCU.
> 
> The ftrace infrastructure has some protections agains recursions and RCU
> but one must still be very careful how they use the callbacks.
> 
> 
> The ftrace_ops structure
> ========================
> 
> To register a function callback, a ftrace_ops is required. This structure
> is used to tell ftrace what function should be called as the callback
> as well as what protections the callback will perform and not require
> ftrace to handle.
> 
> There are only two fields that are needed to be set when registering
> an ftrace_ops with ftrace. The rest should be NULL.
> 
> struct ftrace_ops ops = {
>        .func			= my_callback_func,
>        .flags			= MY_FTRACE_FLAGS
>        .private			= any_private_data_structure,
> };
> 
> Both .flags and .private are optional. Only .func is required.
> 
> To enable tracing call:
> 
>   register_ftrace_function(&ops);

Maybe it would help to have a small section on 'The register function'
below to answer?

Is it possible to make changes to the filter after calling
register_ftrace_function()?  Or do you need to call
register_ftrace_function() again?

> To disable tracing call:
> 
>   unregister_ftrace_function(@ops);
> 
> 
> The callback function
> =====================
> 
> The prototype of the callback function is as follows (as of v4.14):
> 
>  void callback_func(unsigned long ip, unsigned long parent_ip,
> 		    struct ftrace_ops *op, struct pt_regs *regs);
> 
> @ip - This is the instruction pointer of the function that is being traced.
>       (where the fentry or mcount is within the function)
> 
> @parent_ip - This is the instruction pointer of the function that called the
>       the function being traced (where the call of the function occurred).
> 
> @op - This is a pointer to ftrace_ops that was used to register the callback.
>       This can be used to pass data to the callback via the private pointer.
> 
> @regs - If the FTRACE_OPS_FL_SAVE_REGS or FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED
>       flags are set in the ftrace_ops structure, then this will be pointing
>       to the pt_regs structure like it would be if an breakpoint was placed
>       at the start of the function where ftrace was tracing. Otherwise it
>       either contains garbage, or NULL.
> 
> 
> The ftrace FLAGS
> ================
> 
> The ftrace_ops flags are all defined and documented in include/linux/ftrace.h.
> Some of the flags are used for internal infrastructure of ftrace, but the
> ones that users should be aware of are the following:
> 
> (All of these are prefixed with FTRACE_OPS_FL_)
> 
> PER_CPU - When set, the callback can be enabled or disabled per cpu with the
>       following functions:
> 
>       void ftrace_function_local_enable(struct ftrace_ops *ops);
>       void ftrace_function_local_disable(struct ftrace_ops *ops);
> 
>       These two functions must be called with preemption disabled.
> 
> SAVE_REGS - If the callback requires reading or modifying the pt_regs
>       passed to the callback, then it must set this flag. Registering
>       a ftrace_ops with this flag set on an architecture that does not
>       support passing of pt_regs to the callback, will fail.
> 
> SAVE_REGS_IF_SUPPORTED - Similar to SAVE_REGS but the registering of a
>       ftrace_ops on an architecture that does not support passing of regs
>       will not fail with this flag set. But the callback must check if
>       regs is NULL or not to determine if the architecture supports it.
> 
> RECURSION_SAFE - By default, a wrapper is added around the callback to
>       make sure that recursion of the function does not occur. That is
>       if a function within the callback itself is also traced, ftrace
>       will prevent the callback from being called again. But this wrapper
>       adds some overhead, and if the callback is safe from recursion,
>       it can set this flag to disable the ftrace protection.
> 
> IPMODIFY - Requires SAVE_REGS set. If the callback is to "hijack" the
>       traced function (have another function called instead of the traced
>       function), it requires setting this flag. This is what live kernel
>       patches uses. Without this flag the pt_regs->ip can not be modified.
>       Note, only one ftrace_ops with IPMODIFY set may be registered to
>       any given function at a time.
> 
> RCU - If this is set, then the callback will only be called by functions
>       where RCU is "watching". This is required if the callback function
>       performs any rcu_read_lock() operation.
> 
> 
> Filtering what functions to trace
> =================================
> 
> If a callback is only to be called from specific functions, a filter must be
> set up. The filters are added by name, or ip if it is known.
> 
>  int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
> 		       int len, int reset);
> 
>  @ops - the ops to set the filter with
>  @buf - the string that holds the function filter text.
>  @len - the length of the string.
>  @reset - non zero to reset all filters before applying this filter.
> 
>  Filters denote which functions should be enabled when tracing is enabled.
>  If @buf is NULL and reset is set, all functions will be enabled for tracing.
> 
> 
> The @buf can also be a glob expression to enable all functions that
> match a specific pattern.
> 
> To just trace the schedule function:
> 
>  ret = ftrace_set_filter(&ops, "schedule", strlen("schedule"), 0);
> 
> To add more functions, call the ftrace_set_filter() more than once with the
> @reset parameter set to zero. To remove the current filter and replace it
> with new functions to trace, have @reset be non zero.
> 
> Sometimes more than one function has the same name. To trace just a specific
> function in this case, ftrace_set_filter_ip() can be used.
> 
>  ret = ftrace_set_filter_ip(&ops, ip, 0, 0);
> 
> Although the ip must be the address where the call to fentry or mcount is
> located in the function.
> 
> If a glob is used to set the filter, to remove unwanted matches the
> ftrace_set_notrace() can also be used.
> 
>   int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
> 			 int len, int reset);
> 
> This takes the same parameters as ftrace_set_filter() but will add the
> functions it finds to not be traced. This doesn't remove them from the
> filter itself, but keeps them from being traced. If @reset is set,
> the filter is cleaded but the functions that match @buf will still not

'cleared'?

> be traced (the callback will not be called on those functions).

This is a bit confusing, I guess it means 'the existng filter is cleared
and the filter *will match all* functions excluding those that match @buf'.

-Stafford

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-06 15:34       ` Steven Rostedt
  2017-10-07  5:24         ` Stafford Horne
@ 2017-10-07  8:55         ` Ingo Molnar
  2017-10-09 16:45           ` Steven Rostedt
  2017-10-07  9:35         ` Masami Hiramatsu
  2017-10-09 15:33         ` Jonathan Corbet
  3 siblings, 1 reply; 38+ messages in thread
From: Ingo Molnar @ 2017-10-07  8:55 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Masami Hiramatsu, Kees Cook, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger


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

> On Fri, 6 Oct 2017 13:49:59 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Steve, could you write a documentation how to use ftrace callback?
> > I think I should update the Documentation/kprobes.txt so that jprobe
> > user can easily migrate on that.
> 
> I decided to do this now. Here's a first draft. What do you think?
> 
> -- Steve
> 
> 		Using ftrace to hook to functions
> 		=================================
> 
> Copyright 2017 VMware Inc.
>    Author:   Steven Rostedt <srostedt@goodmis.org>
>   License:   The GNU Free Documentation License, Version 1.2
>                (dual licensed under the GPL v2)
> 
> Written for: 4.14
> 
> Introduction
> ------------
> 
> The ftrace infrastructure was originially created to attach hooks to the
> beginning of functions in order to record and trace the flow of the kernel.
> But hooks to the start of a function can have other use cases. Either
> for live kernel patching, or for security monitoring. This document describes
> how to use ftrace to implement your own function hooks.
> 
> 
> The ftrace context
> ==================
> 
> WARNING: The ability to add a callback to almost any function within the
> kernel comes with risks. A callback can be called from any context
> (normal, softirq, irq, and NMI). Callbacks can also be called just before
> going to idle, during CPU bring up and takedown, or going to user space.
> This requires extra care to what can be done inside a callback. A callback
> can be called outside the protective scope of RCU.
> 
> The ftrace infrastructure has some protections agains recursions and RCU
> but one must still be very careful how they use the callbacks.
> 
> 
> The ftrace_ops structure
> ========================
> 
> To register a function callback, a ftrace_ops is required. This structure
> is used to tell ftrace what function should be called as the callback
> as well as what protections the callback will perform and not require
> ftrace to handle.

So the text first starts talking about 'hooks' then uses the 'callback' 
terminology in the rest of th document. Could we please change it all to 
'callback'?

[ This is a pet peeve of mine as 'hook' gives me the cringe! ;-) ]

Thanks,

	Ingo

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-06 15:34       ` Steven Rostedt
  2017-10-07  5:24         ` Stafford Horne
  2017-10-07  8:55         ` Ingo Molnar
@ 2017-10-07  9:35         ` Masami Hiramatsu
  2017-10-09 16:59           ` Steven Rostedt
  2017-10-09 15:33         ` Jonathan Corbet
  3 siblings, 1 reply; 38+ messages in thread
From: Masami Hiramatsu @ 2017-10-07  9:35 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Kees Cook, Ingo Molnar, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Fri, 6 Oct 2017 11:34:30 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 6 Oct 2017 13:49:59 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Steve, could you write a documentation how to use ftrace callback?
> > I think I should update the Documentation/kprobes.txt so that jprobe
> > user can easily migrate on that.
> 
> I decided to do this now. Here's a first draft. What do you think?

Good! Thank you for writing this down. I just found some typo.

> 
> -- Steve
> 
> 		Using ftrace to hook to functions
> 		=================================
> 
> Copyright 2017 VMware Inc.
>    Author:   Steven Rostedt <srostedt@goodmis.org>
>   License:   The GNU Free Documentation License, Version 1.2
>                (dual licensed under the GPL v2)
> 
> Written for: 4.14
> 
> Introduction
> ------------
> 
> The ftrace infrastructure was originially created to attach hooks to the
> beginning of functions in order to record and trace the flow of the kernel.
> But hooks to the start of a function can have other use cases. Either
> for live kernel patching, or for security monitoring. This document describes
> how to use ftrace to implement your own function hooks.
> 
> 
> The ftrace context
> ==================
> 
> WARNING: The ability to add a callback to almost any function within the
> kernel comes with risks. A callback can be called from any context
> (normal, softirq, irq, and NMI). Callbacks can also be called just before
> going to idle, during CPU bring up and takedown, or going to user space.
> This requires extra care to what can be done inside a callback. A callback
> can be called outside the protective scope of RCU.
> 
> The ftrace infrastructure has some protections agains recursions and RCU
> but one must still be very careful how they use the callbacks.

Q: As far as I know the ftrace handler is called under preempt-disabled,
 don't we need to mention that here?

> 
> The ftrace_ops structure
> ========================
> 
> To register a function callback, a ftrace_ops is required. This structure
> is used to tell ftrace what function should be called as the callback
> as well as what protections the callback will perform and not require
> ftrace to handle.
> 
> There are only two fields that are needed to be set when registering
> an ftrace_ops with ftrace. The rest should be NULL.
> 
> struct ftrace_ops ops = {
>        .func			= my_callback_func,
>        .flags			= MY_FTRACE_FLAGS
>        .private			= any_private_data_structure,
> };
> 
> Both .flags and .private are optional. Only .func is required.
> 
> To enable tracing call:
> 
>   register_ftrace_function(&ops);
> 
> To disable tracing call:
> 
>   unregister_ftrace_function(@ops);
                               ^^^^Is this &ops?

> 
> 
> The callback function
> =====================
> 
> The prototype of the callback function is as follows (as of v4.14):
                                                                ^^^^^^^^^^^^^
If we put this document under Documentation/, do we still need it?
(since it should be updated along with the code)

>  void callback_func(unsigned long ip, unsigned long parent_ip,
> 		    struct ftrace_ops *op, struct pt_regs *regs);
> 
> @ip - This is the instruction pointer of the function that is being traced.
>       (where the fentry or mcount is within the function)
> 
> @parent_ip - This is the instruction pointer of the function that called the
>       the function being traced (where the call of the function occurred).
> 
> @op - This is a pointer to ftrace_ops that was used to register the callback.
>       This can be used to pass data to the callback via the private pointer.
> 
> @regs - If the FTRACE_OPS_FL_SAVE_REGS or FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED
>       flags are set in the ftrace_ops structure, then this will be pointing
>       to the pt_regs structure like it would be if an breakpoint was placed
>       at the start of the function where ftrace was tracing. Otherwise it
>       either contains garbage, or NULL.
> 
> 
> The ftrace FLAGS
> ================
> 
> The ftrace_ops flags are all defined and documented in include/linux/ftrace.h.
> Some of the flags are used for internal infrastructure of ftrace, but the
> ones that users should be aware of are the following:
> 
> (All of these are prefixed with FTRACE_OPS_FL_)
> 
> PER_CPU - When set, the callback can be enabled or disabled per cpu with the
>       following functions:
> 
>       void ftrace_function_local_enable(struct ftrace_ops *ops);
>       void ftrace_function_local_disable(struct ftrace_ops *ops);
> 
>       These two functions must be called with preemption disabled.
> 
> SAVE_REGS - If the callback requires reading or modifying the pt_regs
>       passed to the callback, then it must set this flag. Registering
>       a ftrace_ops with this flag set on an architecture that does not
>       support passing of pt_regs to the callback, will fail.
> 
> SAVE_REGS_IF_SUPPORTED - Similar to SAVE_REGS but the registering of a
>       ftrace_ops on an architecture that does not support passing of regs
>       will not fail with this flag set. But the callback must check if
>       regs is NULL or not to determine if the architecture supports it.
> 
> RECURSION_SAFE - By default, a wrapper is added around the callback to
>       make sure that recursion of the function does not occur. That is
>       if a function within the callback itself is also traced, ftrace
>       will prevent the callback from being called again. But this wrapper
>       adds some overhead, and if the callback is safe from recursion,
>       it can set this flag to disable the ftrace protection.

( Ah, I didn't noticed this flag )

> 
> IPMODIFY - Requires SAVE_REGS set. If the callback is to "hijack" the
>       traced function (have another function called instead of the traced
>       function), it requires setting this flag. This is what live kernel
>       patches uses. Without this flag the pt_regs->ip can not be modified.
>       Note, only one ftrace_ops with IPMODIFY set may be registered to
>       any given function at a time.
> 
> RCU - If this is set, then the callback will only be called by functions
>       where RCU is "watching". This is required if the callback function
>       performs any rcu_read_lock() operation.
> 
> 
> Filtering what functions to trace
> =================================
> 
> If a callback is only to be called from specific functions, a filter must be
> set up. The filters are added by name, or ip if it is known.
> 
>  int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
> 		       int len, int reset);
> 
>  @ops - the ops to set the filter with
>  @buf - the string that holds the function filter text.
>  @len - the length of the string.
>  @reset - non zero to reset all filters before applying this filter.
> 
>  Filters denote which functions should be enabled when tracing is enabled.
>  If @buf is NULL and reset is set, all functions will be enabled for tracing.
> 
> 
> The @buf can also be a glob expression to enable all functions that
> match a specific pattern.
> 
> To just trace the schedule function:
> 
>  ret = ftrace_set_filter(&ops, "schedule", strlen("schedule"), 0);
> 
> To add more functions, call the ftrace_set_filter() more than once with the
> @reset parameter set to zero. To remove the current filter and replace it
> with new functions to trace, have @reset be non zero.
> 
> Sometimes more than one function has the same name. To trace just a specific
> function in this case, ftrace_set_filter_ip() can be used.
> 
>  ret = ftrace_set_filter_ip(&ops, ip, 0, 0);
> 
> Although the ip must be the address where the call to fentry or mcount is
> located in the function.
> 
> If a glob is used to set the filter, to remove unwanted matches the
> ftrace_set_notrace() can also be used.
> 
>   int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
> 			 int len, int reset);
> 
> This takes the same parameters as ftrace_set_filter() but will add the
> functions it finds to not be traced. This doesn't remove them from the
> filter itself, but keeps them from being traced. If @reset is set,
> the filter is cleaded but the functions that match @buf will still not
                  ^^^^^^^^
                       cleared?

> be traced (the callback will not be called on those functions).

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-06 15:34       ` Steven Rostedt
                           ` (2 preceding siblings ...)
  2017-10-07  9:35         ` Masami Hiramatsu
@ 2017-10-09 15:33         ` Jonathan Corbet
  2017-10-09 16:20           ` Steven Rostedt
                             ` (2 more replies)
  3 siblings, 3 replies; 38+ messages in thread
From: Jonathan Corbet @ 2017-10-09 15:33 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Masami Hiramatsu, Kees Cook, Ingo Molnar, Linus Torvalds,
	Peter Zijlstra, Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Fri, 6 Oct 2017 11:34:30 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 6 Oct 2017 13:49:59 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Steve, could you write a documentation how to use ftrace callback?
> > I think I should update the Documentation/kprobes.txt so that jprobe
> > user can easily migrate on that.  
> 
> I decided to do this now. Here's a first draft. What do you think?

An overall request: this document is already 99% in the RST format.  It
would be nice to just do it in RST to begin with and save somebody the
effort of finishing the job later.

> -- Steve
> 
> 		Using ftrace to hook to functions
> 		=================================
> 
> Copyright 2017 VMware Inc.
>    Author:   Steven Rostedt <srostedt@goodmis.org>
>   License:   The GNU Free Documentation License, Version 1.2
>                (dual licensed under the GPL v2)
> 
> Written for: 4.14

Experience says that strings like this go obsolete in a hurry, so they
don't really reflect the state of the document.  The git history tends to
be rather more reliable.

> Introduction
> ------------
> 
> The ftrace infrastructure was originially created to attach hooks to the
> beginning of functions in order to record and trace the flow of the kernel.
> But hooks to the start of a function can have other use cases. Either
> for live kernel patching, or for security monitoring. This document describes
> how to use ftrace to implement your own function hooks.
> 
> 
> The ftrace context
> ==================
> 
> WARNING: The ability to add a callback to almost any function within the
> kernel comes with risks. A callback can be called from any context
> (normal, softirq, irq, and NMI). Callbacks can also be called just before
> going to idle, during CPU bring up and takedown, or going to user space.
> This requires extra care to what can be done inside a callback. A callback
> can be called outside the protective scope of RCU.
> 
> The ftrace infrastructure has some protections agains recursions and RCU
> but one must still be very careful how they use the callbacks.
> 
> 
> The ftrace_ops structure
> ========================
> 
> To register a function callback, a ftrace_ops is required.

It would be nice to say which file should be #include'd to get these
declarations. 

> This structure
> is used to tell ftrace what function should be called as the callback
> as well as what protections the callback will perform and not require
> ftrace to handle.
> 
> There are only two fields that are needed to be set when registering
> an ftrace_ops with ftrace. The rest should be NULL.
> 
> struct ftrace_ops ops = {
>        .func			= my_callback_func,
>        .flags			= MY_FTRACE_FLAGS
>        .private			= any_private_data_structure,
> };
> 
> Both .flags and .private are optional. Only .func is required.

So...only two fields are needed, but three are shown, and only one is
required.  Dumb people like me are easily confused by such things...

> To enable tracing call:
> 
>   register_ftrace_function(&ops);
> 
> To disable tracing call:
> 
>   unregister_ftrace_function(@ops);

I would presume that the callback can be called immediately, perhaps even
before register_ftrace_function() returns, right?  It never hurts to warn
readers that they need to be ready from the outset.

> 
> The callback function
> =====================
> 
> The prototype of the callback function is as follows (as of v4.14):
> 
>  void callback_func(unsigned long ip, unsigned long parent_ip,
> 		    struct ftrace_ops *op, struct pt_regs *regs);
> 
> @ip - This is the instruction pointer of the function that is being traced.
>       (where the fentry or mcount is within the function)
> 
> @parent_ip - This is the instruction pointer of the function that called the
>       the function being traced (where the call of the function occurred).
> 
> @op - This is a pointer to ftrace_ops that was used to register the callback.
>       This can be used to pass data to the callback via the private pointer.
> 
> @regs - If the FTRACE_OPS_FL_SAVE_REGS or FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED
>       flags are set in the ftrace_ops structure, then this will be pointing
>       to the pt_regs structure like it would be if an breakpoint was placed
>       at the start of the function where ftrace was tracing. Otherwise it
>       either contains garbage, or NULL.
> 
> 
> The ftrace FLAGS
> ================
> 
> The ftrace_ops flags are all defined and documented in include/linux/ftrace.h.
> Some of the flags are used for internal infrastructure of ftrace, but the
> ones that users should be aware of are the following:
> 
> (All of these are prefixed with FTRACE_OPS_FL_)

I hate to say it, but you'll reduce the cognitive load on the reader if you
just spell the flags out.

> PER_CPU - When set, the callback can be enabled or disabled per cpu with the
>       following functions:
> 
>       void ftrace_function_local_enable(struct ftrace_ops *ops);
>       void ftrace_function_local_disable(struct ftrace_ops *ops);
> 
>       These two functions must be called with preemption disabled.

s/per cpu/per-CPU/

More importantly, though, what does this actually mean?  Presumably that
the callback is only called if the function is hit on an enabled CPU?  It
seems you can only enable/disable the local CPU?  Is the default state
enabled or disabled?  Inquiring minds want to know.

> SAVE_REGS - If the callback requires reading or modifying the pt_regs
>       passed to the callback, then it must set this flag. Registering
>       a ftrace_ops with this flag set on an architecture that does not
>       support passing of pt_regs to the callback, will fail.

Drop that last comma :)

> SAVE_REGS_IF_SUPPORTED - Similar to SAVE_REGS but the registering of a
>       ftrace_ops on an architecture that does not support passing of regs
>       will not fail with this flag set. But the callback must check if
>       regs is NULL or not to determine if the architecture supports it.
> 
> RECURSION_SAFE - By default, a wrapper is added around the callback to
>       make sure that recursion of the function does not occur. That is
>       if a function within the callback itself is also traced, ftrace

s/within the/called by the/

>       will prevent the callback from being called again. But this wrapper
>       adds some overhead, and if the callback is safe from recursion,
>       it can set this flag to disable the ftrace protection.

What happens if you mark a function safe and it recurses anyway?  Say, if
somebody else has hooked a different function unknown to the current
caller? 

> IPMODIFY - Requires SAVE_REGS set. If the callback is to "hijack" the
>       traced function (have another function called instead of the traced
>       function), it requires setting this flag. This is what live kernel
>       patches uses. Without this flag the pt_regs->ip can not be modified.
>       Note, only one ftrace_ops with IPMODIFY set may be registered to
>       any given function at a time.

I assume this requires being able to get the regs too?  

> RCU - If this is set, then the callback will only be called by functions
>       where RCU is "watching". This is required if the callback function
>       performs any rcu_read_lock() operation.

What does that mean, exactly?  When might RCU not be watching?


> Filtering what functions to trace

s/what/which/

> =================================
> 
> If a callback is only to be called from specific functions, a filter must be
> set up. The filters are added by name, or ip if it is known.
> 
>  int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
> 		       int len, int reset);
> 
>  @ops - the ops to set the filter with
>  @buf - the string that holds the function filter text.
>  @len - the length of the string.
>  @reset - non zero to reset all filters before applying this filter.
> 
>  Filters denote which functions should be enabled when tracing is enabled.
>  If @buf is NULL and reset is set, all functions will be enabled for tracing.

...maybe a pointer to documentation on the full filter-string syntax?

> The @buf can also be a glob expression to enable all functions that
> match a specific pattern.
> 
> To just trace the schedule function:
> 
>  ret = ftrace_set_filter(&ops, "schedule", strlen("schedule"), 0);
> 
> To add more functions, call the ftrace_set_filter() more than once with the
> @reset parameter set to zero. To remove the current filter and replace it
> with new functions to trace, have @reset be non zero.

non-zero

> Sometimes more than one function has the same name. To trace just a specific
> function in this case, ftrace_set_filter_ip() can be used.
> 
>  ret = ftrace_set_filter_ip(&ops, ip, 0, 0);
> 
> Although the ip must be the address where the call to fentry or mcount is
> located in the function.

How do I get that if I know I want to trace foo() in bar.c?

> If a glob is used to set the filter, to remove unwanted matches the
> ftrace_set_notrace() can also be used.
> 
>   int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
> 			 int len, int reset);
> 
> This takes the same parameters as ftrace_set_filter() but will add the
> functions it finds to not be traced. This doesn't remove them from the
> filter itself, but keeps them from being traced. If @reset is set,
> the filter is cleaded but the functions that match @buf will still not

cleaded? :)

> be traced (the callback will not be called on those functions).

So how do you clead the "notrace" list?

jon

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-09 15:33         ` Jonathan Corbet
@ 2017-10-09 16:20           ` Steven Rostedt
  2017-10-09 16:33             ` Jonathan Corbet
  2017-10-09 18:10           ` Steven Rostedt
  2017-10-10 14:02           ` Steven Rostedt
  2 siblings, 1 reply; 38+ messages in thread
From: Steven Rostedt @ 2017-10-09 16:20 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: Masami Hiramatsu, Kees Cook, Ingo Molnar, Linus Torvalds,
	Peter Zijlstra, Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Mon, 9 Oct 2017 09:33:50 -0600
Jonathan Corbet <corbet@lwn.net> wrote:

> On Fri, 6 Oct 2017 11:34:30 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Fri, 6 Oct 2017 13:49:59 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >   
> > > Steve, could you write a documentation how to use ftrace callback?
> > > I think I should update the Documentation/kprobes.txt so that jprobe
> > > user can easily migrate on that.    
> > 
> > I decided to do this now. Here's a first draft. What do you think?  
> 
> An overall request: this document is already 99% in the RST format.  It
> would be nice to just do it in RST to begin with and save somebody the
> effort of finishing the job later.

I guess that means I need to learn RST formatting ;-)

> 
> > -- Steve
> > 
> > 		Using ftrace to hook to functions
> > 		=================================
> > 
> > Copyright 2017 VMware Inc.
> >    Author:   Steven Rostedt <srostedt@goodmis.org>
> >   License:   The GNU Free Documentation License, Version 1.2
> >                (dual licensed under the GPL v2)
> > 
> > Written for: 4.14  
> 
> Experience says that strings like this go obsolete in a hurry, so they
> don't really reflect the state of the document.  The git history tends to
> be rather more reliable.

I actually use that for my own reference. I look at that number and it
tells me (oh crap, I need to update this).

Note, when I do an update, I will add:

 Updated for: v5.0

Again, it's more for me, as I don't do a git log on it to see when I
last updated the document. It's usually me referencing a document and
noticing the number that's very old.

> 
> > Introduction
> > ------------
> > 
> > The ftrace infrastructure was originially created to attach hooks to the
> > beginning of functions in order to record and trace the flow of the kernel.
> > But hooks to the start of a function can have other use cases. Either
> > for live kernel patching, or for security monitoring. This document describes
> > how to use ftrace to implement your own function hooks.
> > 
> > 
> > The ftrace context
> > ==================
> > 
> > WARNING: The ability to add a callback to almost any function within the
> > kernel comes with risks. A callback can be called from any context
> > (normal, softirq, irq, and NMI). Callbacks can also be called just before
> > going to idle, during CPU bring up and takedown, or going to user space.
> > This requires extra care to what can be done inside a callback. A callback
> > can be called outside the protective scope of RCU.
> > 
> > The ftrace infrastructure has some protections agains recursions and RCU
> > but one must still be very careful how they use the callbacks.
> > 
> > 
> > The ftrace_ops structure
> > ========================
> > 
> > To register a function callback, a ftrace_ops is required.  
> 
> It would be nice to say which file should be #include'd to get these
> declarations. 

Sure.

/me thinks... I think that would be

#include <linux/ftrace.h>

Will add (after testing).

> 
> > This structure
> > is used to tell ftrace what function should be called as the callback
> > as well as what protections the callback will perform and not require
> > ftrace to handle.
> > 
> > There are only two fields that are needed to be set when registering
> > an ftrace_ops with ftrace. The rest should be NULL.
> > 
> > struct ftrace_ops ops = {
> >        .func			= my_callback_func,
> >        .flags			= MY_FTRACE_FLAGS
> >        .private			= any_private_data_structure,
> > };
> > 
> > Both .flags and .private are optional. Only .func is required.  
> 
> So...only two fields are needed, but three are shown, and only one is
> required.  Dumb people like me are easily confused by such things...

Ah, no that was a mistake. Thanks for catching it. It should have said,
only one field is required.

> 
> > To enable tracing call:
> > 
> >   register_ftrace_function(&ops);
> > 
> > To disable tracing call:
> > 
> >   unregister_ftrace_function(@ops);  
> 
> I would presume that the callback can be called immediately, perhaps even
> before register_ftrace_function() returns, right?  It never hurts to warn
> readers that they need to be ready from the outset.

Yes. I'll add that. I'll also state that it should be safe to assume
that after unregister_ftrace_function() returns, the ops.func will no
longer be called. The unregister_ftrace_function() goes to great pains
to guarantee that. But I should also say that, in order to do that,
unregister_ftrace_function() can take a while to execute (it does a lot
of synchronization to make sure all callbacks have finished).

> 
> > 
> > The callback function
> > =====================
> > 
> > The prototype of the callback function is as follows (as of v4.14):
> > 
> >  void callback_func(unsigned long ip, unsigned long parent_ip,
> > 		    struct ftrace_ops *op, struct pt_regs *regs);
> > 
> > @ip - This is the instruction pointer of the function that is being traced.
> >       (where the fentry or mcount is within the function)
> > 
> > @parent_ip - This is the instruction pointer of the function that called the
> >       the function being traced (where the call of the function occurred).
> > 
> > @op - This is a pointer to ftrace_ops that was used to register the callback.
> >       This can be used to pass data to the callback via the private pointer.
> > 
> > @regs - If the FTRACE_OPS_FL_SAVE_REGS or FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED
> >       flags are set in the ftrace_ops structure, then this will be pointing
> >       to the pt_regs structure like it would be if an breakpoint was placed
> >       at the start of the function where ftrace was tracing. Otherwise it
> >       either contains garbage, or NULL.
> > 
> > 
> > The ftrace FLAGS
> > ================
> > 
> > The ftrace_ops flags are all defined and documented in include/linux/ftrace.h.
> > Some of the flags are used for internal infrastructure of ftrace, but the
> > ones that users should be aware of are the following:
> > 
> > (All of these are prefixed with FTRACE_OPS_FL_)  
> 
> I hate to say it, but you'll reduce the cognitive load on the reader if you
> just spell the flags out.

You mean for each one? OK. Personally I prefer not seeing the duplicate
prefix, but I trust your judgment on this one.

> 
> > PER_CPU - When set, the callback can be enabled or disabled per cpu with the
> >       following functions:
> > 
> >       void ftrace_function_local_enable(struct ftrace_ops *ops);
> >       void ftrace_function_local_disable(struct ftrace_ops *ops);
> > 
> >       These two functions must be called with preemption disabled.  
> 
> s/per cpu/per-CPU/
> 
> More importantly, though, what does this actually mean?  Presumably that
> the callback is only called if the function is hit on an enabled CPU?  It
> seems you can only enable/disable the local CPU?  Is the default state
> enabled or disabled?  Inquiring minds want to know.

Heh, I don't know the answer to that. I'll have to go look. This was
added for perf, as perf likes to have more fine grain control.


> 
> > SAVE_REGS - If the callback requires reading or modifying the pt_regs
> >       passed to the callback, then it must set this flag. Registering
> >       a ftrace_ops with this flag set on an architecture that does not
> >       support passing of pt_regs to the callback, will fail.  
> 
> Drop that last comma :)

OK.

> 
> > SAVE_REGS_IF_SUPPORTED - Similar to SAVE_REGS but the registering of a
> >       ftrace_ops on an architecture that does not support passing of regs
> >       will not fail with this flag set. But the callback must check if
> >       regs is NULL or not to determine if the architecture supports it.
> > 
> > RECURSION_SAFE - By default, a wrapper is added around the callback to
> >       make sure that recursion of the function does not occur. That is
> >       if a function within the callback itself is also traced, ftrace  
> 
> s/within the/called by the/

I put in "within" because it is usually a function that is nested
within a function called by the callback. This bug has come up with
"gotchas", where some function that the callback calls has a path to a
function that was unexpectedly traced.

The issue hasn't been caused by a function being traced that was
directly called by the callback. It is usually some deeper nested
function.

I don't want to limit it to just checking functions that the callback
calls. Thoughts on how to stress this?


> 
> >       will prevent the callback from being called again. But this wrapper
> >       adds some overhead, and if the callback is safe from recursion,
> >       it can set this flag to disable the ftrace protection.  
> 
> What happens if you mark a function safe and it recurses anyway?  Say, if
> somebody else has hooked a different function unknown to the current
> caller? 

It crashes the box ;-)


> 
> > IPMODIFY - Requires SAVE_REGS set. If the callback is to "hijack" the
> >       traced function (have another function called instead of the traced
> >       function), it requires setting this flag. This is what live kernel
> >       patches uses. Without this flag the pt_regs->ip can not be modified.
> >       Note, only one ftrace_ops with IPMODIFY set may be registered to
> >       any given function at a time.  
> 
> I assume this requires being able to get the regs too?  

Yes, this is why I stated "Requires SAVE_REGS" which would pass the
regs to the callback. Should I rewrite that somehow?

> 
> > RCU - If this is set, then the callback will only be called by functions
> >       where RCU is "watching". This is required if the callback function
> >       performs any rcu_read_lock() operation.  
> 
> What does that mean, exactly?  When might RCU not be watching?

Hmm, I thought I stated this someplace else, but I'll state it here
again too.

RCU is not watching when the CPU goes idle or goes offline, and when it
is coming back on line. Also in some cases, RCU is not watching
switching from kernel to user space and back again. If a function is
called that is traced while RCU is in the process of switching between
any of these transactions, it may not be watching (no RCU protection
with any of the RCU synchronize functions).

> 
> 
> > Filtering what functions to trace  
> 
> s/what/which/

OK

> 
> > =================================
> > 
> > If a callback is only to be called from specific functions, a filter must be
> > set up. The filters are added by name, or ip if it is known.
> > 
> >  int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
> > 		       int len, int reset);
> > 
> >  @ops - the ops to set the filter with
> >  @buf - the string that holds the function filter text.
> >  @len - the length of the string.
> >  @reset - non zero to reset all filters before applying this filter.
> > 
> >  Filters denote which functions should be enabled when tracing is enabled.
> >  If @buf is NULL and reset is set, all functions will be enabled for tracing.  
> 
> ...maybe a pointer to documentation on the full filter-string syntax?
> 
> > The @buf can also be a glob expression to enable all functions that
> > match a specific pattern.
> > 
> > To just trace the schedule function:
> > 
> >  ret = ftrace_set_filter(&ops, "schedule", strlen("schedule"), 0);
> > 
> > To add more functions, call the ftrace_set_filter() more than once with the
> > @reset parameter set to zero. To remove the current filter and replace it
> > with new functions to trace, have @reset be non zero.  
> 
> non-zero

OK

> 
> > Sometimes more than one function has the same name. To trace just a specific
> > function in this case, ftrace_set_filter_ip() can be used.
> > 
> >  ret = ftrace_set_filter_ip(&ops, ip, 0, 0);
> > 
> > Although the ip must be the address where the call to fentry or mcount is
> > located in the function.  
> 
> How do I get that if I know I want to trace foo() in bar.c?

There's no easy answer here. The current users of this is live patching
and kprobes, where the address is given directly by the subsystem using
it, and it doesn't need to look up by name.

> 
> > If a glob is used to set the filter, to remove unwanted matches the
> > ftrace_set_notrace() can also be used.
> > 
> >   int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
> > 			 int len, int reset);
> > 
> > This takes the same parameters as ftrace_set_filter() but will add the
> > functions it finds to not be traced. This doesn't remove them from the
> > filter itself, but keeps them from being traced. If @reset is set,
> > the filter is cleaded but the functions that match @buf will still not  
> 
> cleaded? :)

Hmm, I'll have to be more descriptive.

> 
> > be traced (the callback will not be called on those functions).  
> 
> So how do you clead the "notrace" list?

With passing in reset non-zero. I'll add that.

Thanks for reviewing!

-- Steve

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-09 16:20           ` Steven Rostedt
@ 2017-10-09 16:33             ` Jonathan Corbet
  2017-10-09 16:41               ` Steven Rostedt
  0 siblings, 1 reply; 38+ messages in thread
From: Jonathan Corbet @ 2017-10-09 16:33 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Masami Hiramatsu, Kees Cook, Ingo Molnar, Linus Torvalds,
	Peter Zijlstra, Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Mon, 9 Oct 2017 12:20:35 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > > SAVE_REGS_IF_SUPPORTED - Similar to SAVE_REGS but the registering of a
> > >       ftrace_ops on an architecture that does not support passing of regs
> > >       will not fail with this flag set. But the callback must check if
> > >       regs is NULL or not to determine if the architecture supports it.
> > > 
> > > RECURSION_SAFE - By default, a wrapper is added around the callback to
> > >       make sure that recursion of the function does not occur. That is
> > >       if a function within the callback itself is also traced, ftrace    
> > 
> > s/within the/called by the/  
> 
> I put in "within" because it is usually a function that is nested
> within a function called by the callback. This bug has come up with
> "gotchas", where some function that the callback calls has a path to a
> function that was unexpectedly traced.
> 
> The issue hasn't been caused by a function being traced that was
> directly called by the callback. It is usually some deeper nested
> function.
> 
> I don't want to limit it to just checking functions that the callback
> calls. Thoughts on how to stress this?

"if a function that is called as a result of the callback's execution is
also traced" ?

> > > IPMODIFY - Requires SAVE_REGS set. If the callback is to "hijack" the
> > >       traced function (have another function called instead of the traced
> > >       function), it requires setting this flag. This is what live kernel
> > >       patches uses. Without this flag the pt_regs->ip can not be modified.
> > >       Note, only one ftrace_ops with IPMODIFY set may be registered to
> > >       any given function at a time.    
> > 
> > I assume this requires being able to get the regs too?    
> 
> Yes, this is why I stated "Requires SAVE_REGS" which would pass the
> regs to the callback. Should I rewrite that somehow?

No, just ship me another cup of coffee and that one should be OK.  Though
perhaps if you'd spelled out the flag completely I wouldn't have been so
dense :)

> > > If a glob is used to set the filter, to remove unwanted matches the
> > > ftrace_set_notrace() can also be used.
> > > 
> > >   int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
> > > 			 int len, int reset);
> > > 
> > > This takes the same parameters as ftrace_set_filter() but will add the
> > > functions it finds to not be traced. This doesn't remove them from the
> > > filter itself, but keeps them from being traced. If @reset is set,
> > > the filter is cleaded but the functions that match @buf will still not    
> > 
> > cleaded? :)  
> 
> Hmm, I'll have to be more descriptive.
> 
> >   
> > > be traced (the callback will not be called on those functions).    
> > 
> > So how do you clead the "notrace" list?  
> 
> With passing in reset non-zero. I'll add that.

My confusion remains here.  The text says that if reset is "set", then the
"notrace" list remains in place.  So a non-zero "reset" value will remove
previous notrace entries, along with the filter itself?  So if you wanted
to clear the notrace list entirely you would use buf="", reset=1?  It would
be good to be explicit there.

Thanks,

jon

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-09 16:33             ` Jonathan Corbet
@ 2017-10-09 16:41               ` Steven Rostedt
  0 siblings, 0 replies; 38+ messages in thread
From: Steven Rostedt @ 2017-10-09 16:41 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: Masami Hiramatsu, Kees Cook, Ingo Molnar, Linus Torvalds,
	Peter Zijlstra, Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Mon, 9 Oct 2017 10:33:17 -0600
Jonathan Corbet <corbet@lwn.net> wrote:

> On Mon, 9 Oct 2017 12:20:35 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > > SAVE_REGS_IF_SUPPORTED - Similar to SAVE_REGS but the registering of a
> > > >       ftrace_ops on an architecture that does not support passing of regs
> > > >       will not fail with this flag set. But the callback must check if
> > > >       regs is NULL or not to determine if the architecture supports it.
> > > > 
> > > > RECURSION_SAFE - By default, a wrapper is added around the callback to
> > > >       make sure that recursion of the function does not occur. That is
> > > >       if a function within the callback itself is also traced, ftrace      
> > > 
> > > s/within the/called by the/    
> > 
> > I put in "within" because it is usually a function that is nested
> > within a function called by the callback. This bug has come up with
> > "gotchas", where some function that the callback calls has a path to a
> > function that was unexpectedly traced.
> > 
> > The issue hasn't been caused by a function being traced that was
> > directly called by the callback. It is usually some deeper nested
> > function.
> > 
> > I don't want to limit it to just checking functions that the callback
> > calls. Thoughts on how to stress this?  
> 
> "if a function that is called as a result of the callback's execution is
> also traced" ?

Sure, I'm not sure I could come up with a better way to say it.

> 
> > > > IPMODIFY - Requires SAVE_REGS set. If the callback is to "hijack" the
> > > >       traced function (have another function called instead of the traced
> > > >       function), it requires setting this flag. This is what live kernel
> > > >       patches uses. Without this flag the pt_regs->ip can not be modified.
> > > >       Note, only one ftrace_ops with IPMODIFY set may be registered to
> > > >       any given function at a time.      
> > > 
> > > I assume this requires being able to get the regs too?      
> > 
> > Yes, this is why I stated "Requires SAVE_REGS" which would pass the
> > regs to the callback. Should I rewrite that somehow?  
> 
> No, just ship me another cup of coffee and that one should be OK.  Though
> perhaps if you'd spelled out the flag completely I wouldn't have been so
> dense :)

OK OK, I'll extend the names.

> 
> > > > If a glob is used to set the filter, to remove unwanted matches the
> > > > ftrace_set_notrace() can also be used.
> > > > 
> > > >   int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
> > > > 			 int len, int reset);
> > > > 
> > > > This takes the same parameters as ftrace_set_filter() but will add the
> > > > functions it finds to not be traced. This doesn't remove them from the
> > > > filter itself, but keeps them from being traced. If @reset is set,
> > > > the filter is cleaded but the functions that match @buf will still not      
> > > 
> > > cleaded? :)    
> > 
> > Hmm, I'll have to be more descriptive.
> >   
> > >     
> > > > be traced (the callback will not be called on those functions).      
> > > 
> > > So how do you clead the "notrace" list?    
> > 
> > With passing in reset non-zero. I'll add that.  
> 
> My confusion remains here.  The text says that if reset is "set", then the
> "notrace" list remains in place.  So a non-zero "reset" value will remove
> previous notrace entries, along with the filter itself?  So if you wanted
> to clear the notrace list entirely you would use buf="", reset=1?  It would
> be good to be explicit there.

Ah I see what you mean. I'll try to be more explicit. A non-zero value
for reset means to clear the notrace buffer before making modifications.
In fact, I think the text is a bit more confusing, as I don't believe
that this function even modifies the "set_filter" list, even if reset
is set.

And yes, to clear all functions in either the set_filter or set_notrace
lists just pass in (&ops, NULL, 0, 1)

I'll start working on this document and post a real patch.

Thanks again for the review.

-- Steve

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-07  8:55         ` Ingo Molnar
@ 2017-10-09 16:45           ` Steven Rostedt
  0 siblings, 0 replies; 38+ messages in thread
From: Steven Rostedt @ 2017-10-09 16:45 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Masami Hiramatsu, Kees Cook, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Sat, 7 Oct 2017 10:55:00 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> So the text first starts talking about 'hooks' then uses the 'callback' 
> terminology in the rest of th document. Could we please change it all to 
> 'callback'?
> 
> [ This is a pet peeve of mine as 'hook' gives me the cringe! ;-) ]
> 

Will do.

Thanks for the feedback.

-- Steve

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-07  5:24         ` Stafford Horne
@ 2017-10-09 16:48           ` Steven Rostedt
  0 siblings, 0 replies; 38+ messages in thread
From: Steven Rostedt @ 2017-10-09 16:48 UTC (permalink / raw)
  To: Stafford Horne
  Cc: Masami Hiramatsu, Kees Cook, Ingo Molnar, Linus Torvalds,
	Peter Zijlstra, Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Sat, 7 Oct 2017 14:24:53 +0900
Stafford Horne <shorne@gmail.com> wrote:

> Hello,
> 
> Nice read, see some comments below

Thanks.


> > To enable tracing call:
> > 
> >   register_ftrace_function(&ops);  
> 
> Maybe it would help to have a small section on 'The register function'
> below to answer?
> 
> Is it possible to make changes to the filter after calling
> register_ftrace_function()?  Or do you need to call
> register_ftrace_function() again?

OK, I'll think about this.

> 


> > This takes the same parameters as ftrace_set_filter() but will add the
> > functions it finds to not be traced. This doesn't remove them from the
> > filter itself, but keeps them from being traced. If @reset is set,
> > the filter is cleaded but the functions that match @buf will still not  
> 
> 'cleared'?

Yeah, Jon pointed out the confusion here too.

> 
> > be traced (the callback will not be called on those functions).  
> 
> This is a bit confusing, I guess it means 'the existng filter is cleared
> and the filter *will match all* functions excluding those that match @buf'.

Yep, I confused Jon with it as well. I think by the time I got to this
part of the document, I was just trying to finish it and did a bit of
rambling and less cognitive thought.

Thanks for reviewing.

-- Steve

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-07  9:35         ` Masami Hiramatsu
@ 2017-10-09 16:59           ` Steven Rostedt
  0 siblings, 0 replies; 38+ messages in thread
From: Steven Rostedt @ 2017-10-09 16:59 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Kees Cook, Ingo Molnar, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Sat, 7 Oct 2017 18:35:40 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:


> > The ftrace context
> > ==================
> > 
> > WARNING: The ability to add a callback to almost any function within the
> > kernel comes with risks. A callback can be called from any context
> > (normal, softirq, irq, and NMI). Callbacks can also be called just before
> > going to idle, during CPU bring up and takedown, or going to user space.
> > This requires extra care to what can be done inside a callback. A callback
> > can be called outside the protective scope of RCU.
> > 
> > The ftrace infrastructure has some protections agains recursions and RCU
> > but one must still be very careful how they use the callbacks.  
> 
> Q: As far as I know the ftrace handler is called under preempt-disabled,
>  don't we need to mention that here?

Actually it is not. Well not always. I should document this. If the
ftrace handler has the right flags and the arch supports it, the ftrace
handler may be called directly by the function being traced via a
simple trampoline. The trampoline is called directly by the function,
and that trampoline will do minimal register set up to call the
callback function. There will be no preempt disabling when it gets
called.

Now, if more than one function is registered to the same function, or
if the function is not recursive safe, or requires to be per-cpu, then
ftrace itself may disable preemption before calling the callback. The
callback should be prepare for either. And yes, this should be added to
the document.

> 
> > 
> > The ftrace_ops structure
> > ========================
> > 
> > To register a function callback, a ftrace_ops is required. This structure
> > is used to tell ftrace what function should be called as the callback
> > as well as what protections the callback will perform and not require
> > ftrace to handle.
> > 
> > There are only two fields that are needed to be set when registering
> > an ftrace_ops with ftrace. The rest should be NULL.
> > 
> > struct ftrace_ops ops = {
> >        .func			= my_callback_func,
> >        .flags			= MY_FTRACE_FLAGS
> >        .private			= any_private_data_structure,
> > };
> > 
> > Both .flags and .private are optional. Only .func is required.
> > 
> > To enable tracing call:
> > 
> >   register_ftrace_function(&ops);
> > 
> > To disable tracing call:
> > 
> >   unregister_ftrace_function(@ops);  
>                                ^^^^Is this &ops?

Oops, yes :-)

> 
> > 
> > 
> > The callback function
> > =====================
> > 
> > The prototype of the callback function is as follows (as of v4.14):  
>                                                                 ^^^^^^^^^^^^^
> If we put this document under Documentation/, do we still need it?
> (since it should be updated along with the code)

It should be, but I'm also a realist. Stuff in Documentation is slow to
update. I added this there so that it can catch my eye and if I see
that, and know that I updated the functions, I know that the document
needs to be updated too.

> 
> >  void callback_func(unsigned long ip, unsigned long parent_ip,
> > 		    struct ftrace_ops *op, struct pt_regs *regs);
> > 
> > @ip - This is the instruction pointer of the function that is being traced.
> >       (where the fentry or mcount is within the function)
> > 
> > @parent_ip - This is the instruction pointer of the function that called the
> >       the function being traced (where the call of the function occurred).
> > 
> > @op - This is a pointer to ftrace_ops that was used to register the callback.
> >       This can be used to pass data to the callback via the private pointer.
> > 
> > @regs - If the FTRACE_OPS_FL_SAVE_REGS or FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED
> >       flags are set in the ftrace_ops structure, then this will be pointing
> >       to the pt_regs structure like it would be if an breakpoint was placed
> >       at the start of the function where ftrace was tracing. Otherwise it
> >       either contains garbage, or NULL.
> > 
> > 
> > The ftrace FLAGS
> > ================
> > 
> > The ftrace_ops flags are all defined and documented in include/linux/ftrace.h.
> > Some of the flags are used for internal infrastructure of ftrace, but the
> > ones that users should be aware of are the following:
> > 
> > (All of these are prefixed with FTRACE_OPS_FL_)
> > 
> > PER_CPU - When set, the callback can be enabled or disabled per cpu with the
> >       following functions:
> > 
> >       void ftrace_function_local_enable(struct ftrace_ops *ops);
> >       void ftrace_function_local_disable(struct ftrace_ops *ops);
> > 
> >       These two functions must be called with preemption disabled.
> > 
> > SAVE_REGS - If the callback requires reading or modifying the pt_regs
> >       passed to the callback, then it must set this flag. Registering
> >       a ftrace_ops with this flag set on an architecture that does not
> >       support passing of pt_regs to the callback, will fail.
> > 
> > SAVE_REGS_IF_SUPPORTED - Similar to SAVE_REGS but the registering of a
> >       ftrace_ops on an architecture that does not support passing of regs
> >       will not fail with this flag set. But the callback must check if
> >       regs is NULL or not to determine if the architecture supports it.
> > 
> > RECURSION_SAFE - By default, a wrapper is added around the callback to
> >       make sure that recursion of the function does not occur. That is
> >       if a function within the callback itself is also traced, ftrace
> >       will prevent the callback from being called again. But this wrapper
> >       adds some overhead, and if the callback is safe from recursion,
> >       it can set this flag to disable the ftrace protection.  
> 
> ( Ah, I didn't noticed this flag )

Yes, with this flag set, preemption may not be disabled. I should
document this a bit better.

> 
> > 
> > IPMODIFY - Requires SAVE_REGS set. If the callback is to "hijack" the
> >       traced function (have another function called instead of the traced
> >       function), it requires setting this flag. This is what live kernel
> >       patches uses. Without this flag the pt_regs->ip can not be modified.
> >       Note, only one ftrace_ops with IPMODIFY set may be registered to
> >       any given function at a time.
> > 
> > RCU - If this is set, then the callback will only be called by functions
> >       where RCU is "watching". This is required if the callback function
> >       performs any rcu_read_lock() operation.
> > 
> > 
> > Filtering what functions to trace
> > =================================
> > 
> > If a callback is only to be called from specific functions, a filter must be
> > set up. The filters are added by name, or ip if it is known.
> > 
> >  int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
> > 		       int len, int reset);
> > 
> >  @ops - the ops to set the filter with
> >  @buf - the string that holds the function filter text.
> >  @len - the length of the string.
> >  @reset - non zero to reset all filters before applying this filter.
> > 
> >  Filters denote which functions should be enabled when tracing is enabled.
> >  If @buf is NULL and reset is set, all functions will be enabled for tracing.
> > 
> > 
> > The @buf can also be a glob expression to enable all functions that
> > match a specific pattern.
> > 
> > To just trace the schedule function:
> > 
> >  ret = ftrace_set_filter(&ops, "schedule", strlen("schedule"), 0);
> > 
> > To add more functions, call the ftrace_set_filter() more than once with the
> > @reset parameter set to zero. To remove the current filter and replace it
> > with new functions to trace, have @reset be non zero.
> > 
> > Sometimes more than one function has the same name. To trace just a specific
> > function in this case, ftrace_set_filter_ip() can be used.
> > 
> >  ret = ftrace_set_filter_ip(&ops, ip, 0, 0);
> > 
> > Although the ip must be the address where the call to fentry or mcount is
> > located in the function.
> > 
> > If a glob is used to set the filter, to remove unwanted matches the
> > ftrace_set_notrace() can also be used.
> > 
> >   int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
> > 			 int len, int reset);
> > 
> > This takes the same parameters as ftrace_set_filter() but will add the
> > functions it finds to not be traced. This doesn't remove them from the
> > filter itself, but keeps them from being traced. If @reset is set,
> > the filter is cleaded but the functions that match @buf will still not  
>                   ^^^^^^^^
>                        cleared?
> 
> > be traced (the callback will not be called on those functions).  

Yep, I guess that stood out like a sore thumb.

-- Steve

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-09 15:33         ` Jonathan Corbet
  2017-10-09 16:20           ` Steven Rostedt
@ 2017-10-09 18:10           ` Steven Rostedt
  2017-10-10 14:02           ` Steven Rostedt
  2 siblings, 0 replies; 38+ messages in thread
From: Steven Rostedt @ 2017-10-09 18:10 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: Masami Hiramatsu, Kees Cook, Ingo Molnar, Linus Torvalds,
	Peter Zijlstra, Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Mon, 9 Oct 2017 09:33:50 -0600
Jonathan Corbet <corbet@lwn.net> wrote:

> >       will prevent the callback from being called again. But this wrapper
> >       adds some overhead, and if the callback is safe from recursion,
> >       it can set this flag to disable the ftrace protection.  
> 
> What happens if you mark a function safe and it recurses anyway?  Say, if
> somebody else has hooked a different function unknown to the current
> caller? 

Oh, if another callback is attached to a function that gets called
within this function, that is fine. In fact I do that all the time. The
problem is if the callback gets called again and then goes into a
recursive loop.

-- Steve

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-09 15:33         ` Jonathan Corbet
  2017-10-09 16:20           ` Steven Rostedt
  2017-10-09 18:10           ` Steven Rostedt
@ 2017-10-10 14:02           ` Steven Rostedt
  2 siblings, 0 replies; 38+ messages in thread
From: Steven Rostedt @ 2017-10-10 14:02 UTC (permalink / raw)
  To: Jonathan Corbet
  Cc: Masami Hiramatsu, Kees Cook, Ingo Molnar, Linus Torvalds,
	Peter Zijlstra, Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Thomas Gleixner, LKML, H . Peter Anvin,
	Anil S Keshavamurthy, David S . Miller, Ian McDonald,
	Vlad Yasevich, Stephen Hemminger

On Mon, 9 Oct 2017 09:33:50 -0600
Jonathan Corbet <corbet@lwn.net> wrote:

> > 		Using ftrace to hook to functions
> > 		=================================
> > 
> > Copyright 2017 VMware Inc.
> >    Author:   Steven Rostedt <srostedt@goodmis.org>
> >   License:   The GNU Free Documentation License, Version 1.2
> >                (dual licensed under the GPL v2)
> > 
> > Written for: 4.14  
> 
> Experience says that strings like this go obsolete in a hurry, so they
> don't really reflect the state of the document.  The git history tends to
> be rather more reliable.

And this document is about to become stale by 4.15 ;-)


> > The ftrace FLAGS
> > ================
> > 
> > The ftrace_ops flags are all defined and documented in include/linux/ftrace.h.
> > Some of the flags are used for internal infrastructure of ftrace, but the
> > ones that users should be aware of are the following:
> > 
> > (All of these are prefixed with FTRACE_OPS_FL_)  
> 
> I hate to say it, but you'll reduce the cognitive load on the reader if you
> just spell the flags out.
> 
> > PER_CPU - When set, the callback can be enabled or disabled per cpu with the
> >       following functions:
> > 
> >       void ftrace_function_local_enable(struct ftrace_ops *ops);
> >       void ftrace_function_local_disable(struct ftrace_ops *ops);
> > 
> >       These two functions must be called with preemption disabled.  
> 
> s/per cpu/per-CPU/
> 
> More importantly, though, what does this actually mean?  Presumably that
> the callback is only called if the function is hit on an enabled CPU?  It
> seems you can only enable/disable the local CPU?  Is the default state
> enabled or disabled?  Inquiring minds want to know.

This may be removed by 4.15.

Link: http://lkml.kernel.org/r/20171010130448.4goubbka7e4bguar@hirez.programming.kicks-ass.net

-- Steve

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

* Re: [RFC PATCH -tip 1/5] kprobes: Use ENOTSUPP instead of ENOSYS
  2017-10-05 23:13 ` [RFC PATCH -tip 1/5] kprobes: Use ENOTSUPP instead of ENOSYS Masami Hiramatsu
@ 2017-10-20  8:57   ` Ingo Molnar
  2017-10-20 15:51     ` Masami Hiramatsu
  0 siblings, 1 reply; 38+ messages in thread
From: Ingo Molnar @ 2017-10-20  8:57 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Linus Torvalds, Peter Zijlstra, Alexei Starovoitov,
	Ananth N Mavinakayanahalli, Paul E . McKenney, Steven Rostedt,
	Thomas Gleixner, LKML, H . Peter Anvin, Anil S Keshavamurthy,
	David S . Miller, Kees Cook, Ian McDonald, Vlad Yasevich,
	Stephen Hemminger


* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Use ENOTSUPP instead of ENOSYS because ENOSYS is reserved
> only for invalid syscall number.

Is this actually true? We use -ENOSYS in a ton of code in kernel/ already, not 
just for non-existing syscall number.

	Ingo

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-05 23:13 [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
                   ` (5 preceding siblings ...)
  2017-10-05 23:35 ` [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Kees Cook
@ 2017-10-20 12:22 ` Ingo Molnar
  2017-10-20 13:32   ` Kees Cook
  6 siblings, 1 reply; 38+ messages in thread
From: Ingo Molnar @ 2017-10-20 12:22 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Linus Torvalds, Peter Zijlstra, Alexei Starovoitov,
	Ananth N Mavinakayanahalli, Paul E . McKenney, Steven Rostedt,
	Thomas Gleixner, LKML, H . Peter Anvin, Anil S Keshavamurthy,
	David S . Miller, Kees Cook, Ian McDonald, Vlad Yasevich,
	Stephen Hemminger


Ok, could we now also fix these:

net/dccp/probe.c:166:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
net/dccp/probe.c:170:4: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
net/dccp/probe.c:190:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
drivers/misc/lkdtm_core.c:317:3: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
drivers/misc/lkdtm_core.c:322:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
drivers/misc/lkdtm_core.c:560:3: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
net/sctp/probe.c:189:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
net/sctp/probe.c:194:3: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
net/sctp/probe.c:240:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
net/ipv4/tcp_probe.c:280:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
net/ipv4/tcp_probe.c:298:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]

I'd suggest removing the networking ones and Cc:-ing it to the networking folks - 
I strongly doubt anyone is using that functionality for real.

The LKDTM one Kees had some ideas (patches?) for?

Thanks,

	Ingo

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

* [tip:perf/core] kprobes: Disable the jprobes APIs
  2017-10-05 23:14 ` [RFC PATCH -tip 2/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
@ 2017-10-20 12:26   ` tip-bot for Masami Hiramatsu
  0 siblings, 0 replies; 38+ messages in thread
From: tip-bot for Masami Hiramatsu @ 2017-10-20 12:26 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: anil.s.keshavamurthy, stephen, mingo, peterz, vyasevich,
	mhiramat, ian.mcdonald, linux-kernel, rostedt, hpa, paulmck,
	ananth, ast, davem, torvalds, tglx, keescook

Commit-ID:  590c845930457d25d27dc1fdd964a1ce18ef2d7d
Gitweb:     https://git.kernel.org/tip/590c845930457d25d27dc1fdd964a1ce18ef2d7d
Author:     Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate: Fri, 6 Oct 2017 08:14:37 +0900
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 20 Oct 2017 11:02:29 +0200

kprobes: Disable the jprobes APIs

Disable the jprobes APIs and comment out the jprobes API function
code. This is in preparation of removing all jprobes related
code (including kprobe's break_handler).

Nowadays ftrace and other tracing features are mature enough
to replace jprobes use-cases. Users can safely use ftrace and
perf probe etc. for their use cases.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Cc: David S . Miller <davem@davemloft.net>
Cc: Ian McDonald <ian.mcdonald@jandi.co.nz>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vlad Yasevich <vyasevich@gmail.com>
Link: http://lkml.kernel.org/r/150724527741.5014.15465541485637899227.stgit@devbox
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/kprobes.h | 40 ++++++++++++++++++----------------------
 kernel/kprobes.c        |  2 ++
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index bd26847..56b2e69 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -391,10 +391,6 @@ int register_kprobes(struct kprobe **kps, int num);
 void unregister_kprobes(struct kprobe **kps, int num);
 int setjmp_pre_handler(struct kprobe *, struct pt_regs *);
 int longjmp_break_handler(struct kprobe *, struct pt_regs *);
-int register_jprobe(struct jprobe *p);
-void unregister_jprobe(struct jprobe *p);
-int register_jprobes(struct jprobe **jps, int num);
-void unregister_jprobes(struct jprobe **jps, int num);
 void jprobe_return(void);
 unsigned long arch_deref_entry_point(void *);
 
@@ -443,20 +439,6 @@ static inline void unregister_kprobe(struct kprobe *p)
 static inline void unregister_kprobes(struct kprobe **kps, int num)
 {
 }
-static inline int register_jprobe(struct jprobe *p)
-{
-	return -ENOSYS;
-}
-static inline int register_jprobes(struct jprobe **jps, int num)
-{
-	return -ENOSYS;
-}
-static inline void unregister_jprobe(struct jprobe *p)
-{
-}
-static inline void unregister_jprobes(struct jprobe **jps, int num)
-{
-}
 static inline void jprobe_return(void)
 {
 }
@@ -486,6 +468,20 @@ static inline int enable_kprobe(struct kprobe *kp)
 	return -ENOSYS;
 }
 #endif /* CONFIG_KPROBES */
+static inline int __deprecated register_jprobe(struct jprobe *p)
+{
+	return -ENOSYS;
+}
+static inline int __deprecated register_jprobes(struct jprobe **jps, int num)
+{
+	return -ENOSYS;
+}
+static inline void __deprecated unregister_jprobe(struct jprobe *p)
+{
+}
+static inline void __deprecated unregister_jprobes(struct jprobe **jps, int num)
+{
+}
 static inline int disable_kretprobe(struct kretprobe *rp)
 {
 	return disable_kprobe(&rp->kp);
@@ -494,13 +490,13 @@ static inline int enable_kretprobe(struct kretprobe *rp)
 {
 	return enable_kprobe(&rp->kp);
 }
-static inline int disable_jprobe(struct jprobe *jp)
+static inline int __deprecated disable_jprobe(struct jprobe *jp)
 {
-	return disable_kprobe(&jp->kp);
+	return -ENOSYS;
 }
-static inline int enable_jprobe(struct jprobe *jp)
+static inline int __deprecated enable_jprobe(struct jprobe *jp)
 {
-	return enable_kprobe(&jp->kp);
+	return -ENOSYS;
 }
 
 #ifndef CONFIG_KPROBES
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index a8fc149..da2ccf1 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1771,6 +1771,7 @@ unsigned long __weak arch_deref_entry_point(void *entry)
 	return (unsigned long)entry;
 }
 
+#if 0
 int register_jprobes(struct jprobe **jps, int num)
 {
 	int ret = 0, i;
@@ -1839,6 +1840,7 @@ void unregister_jprobes(struct jprobe **jps, int num)
 	}
 }
 EXPORT_SYMBOL_GPL(unregister_jprobes);
+#endif
 
 #ifdef CONFIG_KRETPROBES
 /*

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

* [tip:perf/core] kprobes: Disable the jprobes test code
  2017-10-05 23:15 ` [RFC PATCH -tip 3/5] kprobes: Disable jprobe test code Masami Hiramatsu
@ 2017-10-20 12:26   ` tip-bot for Masami Hiramatsu
  0 siblings, 0 replies; 38+ messages in thread
From: tip-bot for Masami Hiramatsu @ 2017-10-20 12:26 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: keescook, peterz, linux-kernel, vyasevich, mhiramat, hpa,
	rostedt, ast, davem, ananth, paulmck, mingo, stephen,
	ian.mcdonald, anil.s.keshavamurthy, torvalds, tglx

Commit-ID:  2c7d662e2647aa55fa56dc449b3878ac24e17adf
Gitweb:     https://git.kernel.org/tip/2c7d662e2647aa55fa56dc449b3878ac24e17adf
Author:     Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate: Fri, 6 Oct 2017 08:15:17 +0900
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 20 Oct 2017 11:02:54 +0200

kprobes: Disable the jprobes test code

Disable jprobes test code because jprobes are deprecated.
This code will be completely removed when the jprobe code
is removed.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Cc: David S . Miller <davem@davemloft.net>
Cc: Ian McDonald <ian.mcdonald@jandi.co.nz>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vlad Yasevich <vyasevich@gmail.com>
Link: http://lkml.kernel.org/r/150724531730.5014.6377596890962355763.stgit@devbox
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/test_kprobes.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/test_kprobes.c b/kernel/test_kprobes.c
index 47106a1..dd53e35 100644
--- a/kernel/test_kprobes.c
+++ b/kernel/test_kprobes.c
@@ -22,7 +22,7 @@
 
 #define div_factor 3
 
-static u32 rand1, preh_val, posth_val, jph_val;
+static u32 rand1, preh_val, posth_val;
 static int errors, handler_errors, num_tests;
 static u32 (*target)(u32 value);
 static u32 (*target2)(u32 value);
@@ -162,6 +162,9 @@ static int test_kprobes(void)
 
 }
 
+#if 0
+static u32 jph_val;
+
 static u32 j_kprobe_target(u32 value)
 {
 	if (preemptible()) {
@@ -239,6 +242,10 @@ static int test_jprobes(void)
 
 	return 0;
 }
+#else
+#define test_jprobe() (0)
+#define test_jprobes() (0)
+#endif
 #ifdef CONFIG_KRETPROBES
 static u32 krph_val;
 

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

* [tip:perf/core] kprobes: Remove the jprobes sample code
  2017-10-05 23:15 ` [RFC PATCH -tip 4/5] kprobes: Remove jprobe sample code Masami Hiramatsu
@ 2017-10-20 12:27   ` tip-bot for Masami Hiramatsu
  0 siblings, 0 replies; 38+ messages in thread
From: tip-bot for Masami Hiramatsu @ 2017-10-20 12:27 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: ananth, rostedt, stephen, mingo, vyasevich, davem, tglx, ast,
	hpa, keescook, linux-kernel, peterz, mhiramat, ian.mcdonald,
	anil.s.keshavamurthy, torvalds, paulmck

Commit-ID:  9be95bdc53c12ada23e39027237fd05e1393d893
Gitweb:     https://git.kernel.org/tip/9be95bdc53c12ada23e39027237fd05e1393d893
Author:     Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate: Fri, 6 Oct 2017 08:15:57 +0900
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 20 Oct 2017 11:02:55 +0200

kprobes: Remove the jprobes sample code

Remove the jprobes sample module because jprobes are deprecated.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Cc: David S . Miller <davem@davemloft.net>
Cc: Ian McDonald <ian.mcdonald@jandi.co.nz>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vlad Yasevich <vyasevich@gmail.com>
Link: http://lkml.kernel.org/r/150724535709.5014.7261513316230565780.stgit@devbox
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 samples/kprobes/Makefile         |  2 +-
 samples/kprobes/jprobe_example.c | 67 ----------------------------------------
 2 files changed, 1 insertion(+), 68 deletions(-)

diff --git a/samples/kprobes/Makefile b/samples/kprobes/Makefile
index 68739bc..880e54d 100644
--- a/samples/kprobes/Makefile
+++ b/samples/kprobes/Makefile
@@ -1,5 +1,5 @@
 # builds the kprobes example kernel modules;
 # then to use one (as root):  insmod <module_name.ko>
 
-obj-$(CONFIG_SAMPLE_KPROBES) += kprobe_example.o jprobe_example.o
+obj-$(CONFIG_SAMPLE_KPROBES) += kprobe_example.o
 obj-$(CONFIG_SAMPLE_KRETPROBES) += kretprobe_example.o
diff --git a/samples/kprobes/jprobe_example.c b/samples/kprobes/jprobe_example.c
deleted file mode 100644
index e3c0a40..0000000
--- a/samples/kprobes/jprobe_example.c
+++ /dev/null
@@ -1,67 +0,0 @@
-/*
- * Here's a sample kernel module showing the use of jprobes to dump
- * the arguments of _do_fork().
- *
- * For more information on theory of operation of jprobes, see
- * Documentation/kprobes.txt
- *
- * Build and insert the kernel module as done in the kprobe example.
- * You will see the trace data in /var/log/messages and on the
- * console whenever _do_fork() is invoked to create a new process.
- * (Some messages may be suppressed if syslogd is configured to
- * eliminate duplicate messages.)
- */
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/kprobes.h>
-
-/*
- * Jumper probe for _do_fork.
- * Mirror principle enables access to arguments of the probed routine
- * from the probe handler.
- */
-
-/* Proxy routine having the same arguments as actual _do_fork() routine */
-static long j_do_fork(unsigned long clone_flags, unsigned long stack_start,
-	      unsigned long stack_size, int __user *parent_tidptr,
-	      int __user *child_tidptr, unsigned long tls)
-{
-	pr_info("jprobe: clone_flags = 0x%lx, stack_start = 0x%lx "
-		"stack_size = 0x%lx\n", clone_flags, stack_start, stack_size);
-
-	/* Always end with a call to jprobe_return(). */
-	jprobe_return();
-	return 0;
-}
-
-static struct jprobe my_jprobe = {
-	.entry			= j_do_fork,
-	.kp = {
-		.symbol_name	= "_do_fork",
-	},
-};
-
-static int __init jprobe_init(void)
-{
-	int ret;
-
-	ret = register_jprobe(&my_jprobe);
-	if (ret < 0) {
-		pr_err("register_jprobe failed, returned %d\n", ret);
-		return -1;
-	}
-	pr_info("Planted jprobe at %p, handler addr %p\n",
-	       my_jprobe.kp.addr, my_jprobe.entry);
-	return 0;
-}
-
-static void __exit jprobe_exit(void)
-{
-	unregister_jprobe(&my_jprobe);
-	pr_info("jprobe at %p unregistered\n", my_jprobe.kp.addr);
-}
-
-module_init(jprobe_init)
-module_exit(jprobe_exit)
-MODULE_LICENSE("GPL");

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

* [tip:perf/core] kprobes/docs: Remove jprobes related documents
  2017-10-05 23:16 ` [RFC PATCH -tip 5/5] kprobes: docs: Remove jprobe related document Masami Hiramatsu
@ 2017-10-20 12:27   ` tip-bot for Masami Hiramatsu
  0 siblings, 0 replies; 38+ messages in thread
From: tip-bot for Masami Hiramatsu @ 2017-10-20 12:27 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: stephen, davem, mhiramat, anil.s.keshavamurthy, ast, rostedt,
	ian.mcdonald, paulmck, linux-kernel, keescook, peterz, tglx,
	ananth, hpa, torvalds, mingo, vyasevich

Commit-ID:  9b17374e11c7ce2cf0b2b990fa4aa0360921aa2b
Gitweb:     https://git.kernel.org/tip/9b17374e11c7ce2cf0b2b990fa4aa0360921aa2b
Author:     Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate: Fri, 6 Oct 2017 08:16:37 +0900
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 20 Oct 2017 11:02:55 +0200

kprobes/docs: Remove jprobes related documents

Remove jprobes related documentation from kprobes.txt.

Also add some migration advice for the people who are
still using jprobes.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Cc: David S . Miller <davem@davemloft.net>
Cc: Ian McDonald <ian.mcdonald@jandi.co.nz>
Cc: Kees Cook <keescook@chromium.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E . McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vlad Yasevich <vyasevich@gmail.com>
Link: http://lkml.kernel.org/r/150724539698.5014.7300022363980503141.stgit@devbox
[ Fixes to the new documentation. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 Documentation/kprobes.txt | 159 +++++++++++++++++-----------------------------
 1 file changed, 57 insertions(+), 102 deletions(-)

diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
index 2335715..22208bf 100644
--- a/Documentation/kprobes.txt
+++ b/Documentation/kprobes.txt
@@ -8,7 +8,7 @@ Kernel Probes (Kprobes)
 
 .. CONTENTS
 
-  1. Concepts: Kprobes, Jprobes, Return Probes
+  1. Concepts: Kprobes, and Return Probes
   2. Architectures Supported
   3. Configuring Kprobes
   4. API Reference
@@ -16,12 +16,12 @@ Kernel Probes (Kprobes)
   6. Probe Overhead
   7. TODO
   8. Kprobes Example
-  9. Jprobes Example
-  10. Kretprobes Example
+  9. Kretprobes Example
+  10. Deprecated Features
   Appendix A: The kprobes debugfs interface
   Appendix B: The kprobes sysctl interface
 
-Concepts: Kprobes, Jprobes, Return Probes
+Concepts: Kprobes and Return Probes
 =========================================
 
 Kprobes enables you to dynamically break into any kernel routine and
@@ -32,12 +32,10 @@ routine to be invoked when the breakpoint is hit.
 .. [1] some parts of the kernel code can not be trapped, see
        :ref:`kprobes_blacklist`)
 
-There are currently three types of probes: kprobes, jprobes, and
-kretprobes (also called return probes).  A kprobe can be inserted
-on virtually any instruction in the kernel.  A jprobe is inserted at
-the entry to a kernel function, and provides convenient access to the
-function's arguments.  A return probe fires when a specified function
-returns.
+There are currently two types of probes: kprobes, and kretprobes
+(also called return probes).  A kprobe can be inserted on virtually
+any instruction in the kernel.  A return probe fires when a specified
+function returns.
 
 In the typical case, Kprobes-based instrumentation is packaged as
 a kernel module.  The module's init function installs ("registers")
@@ -82,45 +80,6 @@ After the instruction is single-stepped, Kprobes executes the
 "post_handler," if any, that is associated with the kprobe.
 Execution then continues with the instruction following the probepoint.
 
-How Does a Jprobe Work?
------------------------
-
-A jprobe is implemented using a kprobe that is placed on a function's
-entry point.  It employs a simple mirroring principle to allow
-seamless access to the probed function's arguments.  The jprobe
-handler routine should have the same signature (arg list and return
-type) as the function being probed, and must always end by calling
-the Kprobes function jprobe_return().
-
-Here's how it works.  When the probe is hit, Kprobes makes a copy of
-the saved registers and a generous portion of the stack (see below).
-Kprobes then points the saved instruction pointer at the jprobe's
-handler routine, and returns from the trap.  As a result, control
-passes to the handler, which is presented with the same register and
-stack contents as the probed function.  When it is done, the handler
-calls jprobe_return(), which traps again to restore the original stack
-contents and processor state and switch to the probed function.
-
-By convention, the callee owns its arguments, so gcc may produce code
-that unexpectedly modifies that portion of the stack.  This is why
-Kprobes saves a copy of the stack and restores it after the jprobe
-handler has run.  Up to MAX_STACK_SIZE bytes are copied -- e.g.,
-64 bytes on i386.
-
-Note that the probed function's args may be passed on the stack
-or in registers.  The jprobe will work in either case, so long as the
-handler's prototype matches that of the probed function.
-
-Note that in some architectures (e.g.: arm64 and sparc64) the stack
-copy is not done, as the actual location of stacked parameters may be
-outside of a reasonable MAX_STACK_SIZE value and because that location
-cannot be determined by the jprobes code. In this case the jprobes
-user must be careful to make certain the calling signature of the
-function does not cause parameters to be passed on the stack (e.g.:
-more than eight function arguments, an argument of more than sixteen
-bytes, or more than 64 bytes of argument data, depending on
-architecture).
-
 Return Probes
 -------------
 
@@ -245,8 +204,7 @@ Pre-optimization
 After preparing the detour buffer, Kprobes verifies that none of the
 following situations exist:
 
-- The probe has either a break_handler (i.e., it's a jprobe) or a
-  post_handler.
+- The probe has a post_handler.
 - Other instructions in the optimized region are probed.
 - The probe is disabled.
 
@@ -331,7 +289,7 @@ rejects registering it, if the given address is in the blacklist.
 Architectures Supported
 =======================
 
-Kprobes, jprobes, and return probes are implemented on the following
+Kprobes and return probes are implemented on the following
 architectures:
 
 - i386 (Supports jump optimization)
@@ -446,27 +404,6 @@ architecture-specific trap number associated with the fault (e.g.,
 on i386, 13 for a general protection fault or 14 for a page fault).
 Returns 1 if it successfully handled the exception.
 
-register_jprobe
----------------
-
-::
-
-	#include <linux/kprobes.h>
-	int register_jprobe(struct jprobe *jp)
-
-Sets a breakpoint at the address jp->kp.addr, which must be the address
-of the first instruction of a function.  When the breakpoint is hit,
-Kprobes runs the handler whose address is jp->entry.
-
-The handler should have the same arg list and return type as the probed
-function; and just before it returns, it must call jprobe_return().
-(The handler never actually returns, since jprobe_return() returns
-control to Kprobes.)  If the probed function is declared asmlinkage
-or anything else that affects how args are passed, the handler's
-declaration must match.
-
-register_jprobe() returns 0 on success, or a negative errno otherwise.
-
 register_kretprobe
 ------------------
 
@@ -513,7 +450,6 @@ unregister_*probe
 
 	#include <linux/kprobes.h>
 	void unregister_kprobe(struct kprobe *kp);
-	void unregister_jprobe(struct jprobe *jp);
 	void unregister_kretprobe(struct kretprobe *rp);
 
 Removes the specified probe.  The unregister function can be called
@@ -532,7 +468,6 @@ register_*probes
 	#include <linux/kprobes.h>
 	int register_kprobes(struct kprobe **kps, int num);
 	int register_kretprobes(struct kretprobe **rps, int num);
-	int register_jprobes(struct jprobe **jps, int num);
 
 Registers each of the num probes in the specified array.  If any
 error occurs during registration, all probes in the array, up to
@@ -555,7 +490,6 @@ unregister_*probes
 	#include <linux/kprobes.h>
 	void unregister_kprobes(struct kprobe **kps, int num);
 	void unregister_kretprobes(struct kretprobe **rps, int num);
-	void unregister_jprobes(struct jprobe **jps, int num);
 
 Removes each of the num probes in the specified array at once.
 
@@ -574,7 +508,6 @@ disable_*probe
 	#include <linux/kprobes.h>
 	int disable_kprobe(struct kprobe *kp);
 	int disable_kretprobe(struct kretprobe *rp);
-	int disable_jprobe(struct jprobe *jp);
 
 Temporarily disables the specified ``*probe``. You can enable it again by using
 enable_*probe(). You must specify the probe which has been registered.
@@ -587,7 +520,6 @@ enable_*probe
 	#include <linux/kprobes.h>
 	int enable_kprobe(struct kprobe *kp);
 	int enable_kretprobe(struct kretprobe *rp);
-	int enable_jprobe(struct jprobe *jp);
 
 Enables ``*probe`` which has been disabled by disable_*probe(). You must specify
 the probe which has been registered.
@@ -595,12 +527,10 @@ the probe which has been registered.
 Kprobes Features and Limitations
 ================================
 
-Kprobes allows multiple probes at the same address.  Currently,
-however, there cannot be multiple jprobes on the same function at
-the same time.  Also, a probepoint for which there is a jprobe or
-a post_handler cannot be optimized.  So if you install a jprobe,
-or a kprobe with a post_handler, at an optimized probepoint, the
-probepoint will be unoptimized automatically.
+Kprobes allows multiple probes at the same address. Also,
+a probepoint for which there is a post_handler cannot be optimized.
+So if you install a kprobe with a post_handler, at an optimized
+probepoint, the probepoint will be unoptimized automatically.
 
 In general, you can install a probe anywhere in the kernel.
 In particular, you can probe interrupt handlers.  Known exceptions
@@ -662,7 +592,7 @@ We're unaware of other specific cases where this could be a problem.
 If, upon entry to or exit from a function, the CPU is running on
 a stack other than that of the current task, registering a return
 probe on that function may produce undesirable results.  For this
-reason, Kprobes doesn't support return probes (or kprobes or jprobes)
+reason, Kprobes doesn't support return probes (or kprobes)
 on the x86_64 version of __switch_to(); the registration functions
 return -EINVAL.
 
@@ -706,24 +636,24 @@ Probe Overhead
 On a typical CPU in use in 2005, a kprobe hit takes 0.5 to 1.0
 microseconds to process.  Specifically, a benchmark that hits the same
 probepoint repeatedly, firing a simple handler each time, reports 1-2
-million hits per second, depending on the architecture.  A jprobe or
-return-probe hit typically takes 50-75% longer than a kprobe hit.
+million hits per second, depending on the architecture.  A return-probe
+hit typically takes 50-75% longer than a kprobe hit.
 When you have a return probe set on a function, adding a kprobe at
 the entry to that function adds essentially no overhead.
 
 Here are sample overhead figures (in usec) for different architectures::
 
-  k = kprobe; j = jprobe; r = return probe; kr = kprobe + return probe
-  on same function; jr = jprobe + return probe on same function::
+  k = kprobe; r = return probe; kr = kprobe + return probe
+  on same function
 
   i386: Intel Pentium M, 1495 MHz, 2957.31 bogomips
-  k = 0.57 usec; j = 1.00; r = 0.92; kr = 0.99; jr = 1.40
+  k = 0.57 usec; r = 0.92; kr = 0.99
 
   x86_64: AMD Opteron 246, 1994 MHz, 3971.48 bogomips
-  k = 0.49 usec; j = 0.76; r = 0.80; kr = 0.82; jr = 1.07
+  k = 0.49 usec; r = 0.80; kr = 0.82
 
   ppc64: POWER5 (gr), 1656 MHz (SMT disabled, 1 virtual CPU per physical CPU)
-  k = 0.77 usec; j = 1.31; r = 1.26; kr = 1.45; jr = 1.99
+  k = 0.77 usec; r = 1.26; kr = 1.45
 
 Optimized Probe Overhead
 ------------------------
@@ -755,11 +685,6 @@ Kprobes Example
 
 See samples/kprobes/kprobe_example.c
 
-Jprobes Example
-===============
-
-See samples/kprobes/jprobe_example.c
-
 Kretprobes Example
 ==================
 
@@ -772,6 +697,37 @@ For additional information on Kprobes, refer to the following URLs:
 - http://www-users.cs.umn.edu/~boutcher/kprobes/
 - http://www.linuxsymposium.org/2006/linuxsymposium_procv2.pdf (pages 101-115)
 
+Deprecated Features
+===================
+
+Jprobes is now a deprecated feature. People who are depending on it should
+migrate to other tracing features or use older kernels. Please consider to
+migrate your tool to one of the following options:
+
+- Use trace-event to trace target function with arguments.
+
+  trace-event is a low-overhead (and almost no visible overhead if it
+  is off) statically defined event interface. You can define new events
+  and trace it via ftrace or any other tracing tools.
+
+  See the following urls:
+
+    - https://lwn.net/Articles/379903/
+    - https://lwn.net/Articles/381064/
+    - https://lwn.net/Articles/383362/
+
+- Use ftrace dynamic events (kprobe event) with perf-probe.
+
+  If you build your kernel with debug info (CONFIG_DEBUG_INFO=y), you can
+  find which register/stack is assigned to which local variable or arguments
+  by using perf-probe and set up new event to trace it.
+
+  See following documents:
+
+  - Documentation/trace/kprobetrace.txt
+  - Documentation/trace/events.txt
+  - tools/perf/Documentation/perf-probe.txt
+
 
 The kprobes debugfs interface
 =============================
@@ -783,14 +739,13 @@ under the /sys/kernel/debug/kprobes/ directory (assuming debugfs is mounted at /
 /sys/kernel/debug/kprobes/list: Lists all registered probes on the system::
 
 	c015d71a  k  vfs_read+0x0
-	c011a316  j  do_fork+0x0
 	c03dedc5  r  tcp_v4_rcv+0x0
 
 The first column provides the kernel address where the probe is inserted.
-The second column identifies the type of probe (k - kprobe, r - kretprobe
-and j - jprobe), while the third column specifies the symbol+offset of
-the probe. If the probed function belongs to a module, the module name
-is also specified. Following columns show probe status. If the probe is on
+The second column identifies the type of probe (k - kprobe and r - kretprobe)
+while the third column specifies the symbol+offset of the probe.
+If the probed function belongs to a module, the module name is also
+specified. Following columns show probe status. If the probe is on
 a virtual address that is no longer valid (module init sections, module
 virtual addresses that correspond to modules that've been unloaded),
 such probes are marked with [GONE]. If the probe is temporarily disabled,

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-20 12:22 ` Ingo Molnar
@ 2017-10-20 13:32   ` Kees Cook
  2017-10-20 15:17     ` Ingo Molnar
  0 siblings, 1 reply; 38+ messages in thread
From: Kees Cook @ 2017-10-20 13:32 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Masami Hiramatsu, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Steven Rostedt, Thomas Gleixner, LKML,
	H . Peter Anvin, Anil S Keshavamurthy, David S . Miller,
	Ian McDonald, Vlad Yasevich, Stephen Hemminger

On Fri, Oct 20, 2017 at 5:22 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> Ok, could we now also fix these:
>
> net/dccp/probe.c:166:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> net/dccp/probe.c:170:4: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> net/dccp/probe.c:190:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> drivers/misc/lkdtm_core.c:317:3: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> drivers/misc/lkdtm_core.c:322:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> drivers/misc/lkdtm_core.c:560:3: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> net/sctp/probe.c:189:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> net/sctp/probe.c:194:3: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> net/sctp/probe.c:240:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> net/ipv4/tcp_probe.c:280:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> net/ipv4/tcp_probe.c:298:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
>
> I'd suggest removing the networking ones and Cc:-ing it to the networking folks -
> I strongly doubt anyone is using that functionality for real.
>
> The LKDTM one Kees had some ideas (patches?) for?

Oops, yes, I've sent this patch to Greg now. I had let it sit for a
few days of 0-day testing and promptly forgot about it. ;)

-Kees

-- 
Kees Cook
Pixel Security

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-20 13:32   ` Kees Cook
@ 2017-10-20 15:17     ` Ingo Molnar
  2017-10-20 16:28       ` Kees Cook
  0 siblings, 1 reply; 38+ messages in thread
From: Ingo Molnar @ 2017-10-20 15:17 UTC (permalink / raw)
  To: Kees Cook, Greg Kroah-Hartman
  Cc: Masami Hiramatsu, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Steven Rostedt, Thomas Gleixner, LKML,
	H . Peter Anvin, Anil S Keshavamurthy, David S . Miller,
	Ian McDonald, Vlad Yasevich, Stephen Hemminger


* Kees Cook <keescook@chromium.org> wrote:

> On Fri, Oct 20, 2017 at 5:22 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > Ok, could we now also fix these:
> >
> > net/dccp/probe.c:166:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> > net/dccp/probe.c:170:4: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> > net/dccp/probe.c:190:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> > drivers/misc/lkdtm_core.c:317:3: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> > drivers/misc/lkdtm_core.c:322:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> > drivers/misc/lkdtm_core.c:560:3: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> > net/sctp/probe.c:189:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> > net/sctp/probe.c:194:3: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> > net/sctp/probe.c:240:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> > net/ipv4/tcp_probe.c:280:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> > net/ipv4/tcp_probe.c:298:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> >
> > I'd suggest removing the networking ones and Cc:-ing it to the networking folks -
> > I strongly doubt anyone is using that functionality for real.
> >
> > The LKDTM one Kees had some ideas (patches?) for?
> 
> Oops, yes, I've sent this patch to Greg now. I had let it sit for a
> few days of 0-day testing and promptly forgot about it. ;)

Could we, as a special exception, carry this patch in -tip, so that the probes 
related changes stay togher - or is it too distruptive to the flow of other 
in-flight LKDTM patches, creating conflicts, etc.?

Thanks,

	Ingo

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

* Re: [RFC PATCH -tip 1/5] kprobes: Use ENOTSUPP instead of ENOSYS
  2017-10-20  8:57   ` Ingo Molnar
@ 2017-10-20 15:51     ` Masami Hiramatsu
  0 siblings, 0 replies; 38+ messages in thread
From: Masami Hiramatsu @ 2017-10-20 15:51 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Peter Zijlstra, Alexei Starovoitov,
	Ananth N Mavinakayanahalli, Paul E . McKenney, Steven Rostedt,
	Thomas Gleixner, LKML, H . Peter Anvin, Anil S Keshavamurthy,
	David S . Miller, Kees Cook, Ian McDonald, Vlad Yasevich,
	Stephen Hemminger

On Fri, 20 Oct 2017 10:57:22 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Use ENOTSUPP instead of ENOSYS because ENOSYS is reserved
> > only for invalid syscall number.
> 
> Is this actually true? We use -ENOSYS in a ton of code in kernel/ already, not 
> just for non-existing syscall number.

I got a warning from checkpatches.pl, also in include/uapi/asm-generic/errno.h,

/*
 * This error code is special: arch syscall entry code will return
 * -ENOSYS if users try to call a syscall that doesn't exist.  To keep
 * failures of syscalls that really do exist distinguishable from
 * failures due to attempts to use a nonexistent syscall, syscall
 * implementations should refrain from returning -ENOSYS.
 */
#define ENOSYS          38      /* Invalid system call number */

So, I decided not to use ENOSYS for this error.

Thank you,


> 
> 	Ingo


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-20 15:17     ` Ingo Molnar
@ 2017-10-20 16:28       ` Kees Cook
  2017-10-21  8:06         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 38+ messages in thread
From: Kees Cook @ 2017-10-20 16:28 UTC (permalink / raw)
  To: Ingo Molnar, Greg Kroah-Hartman
  Cc: Masami Hiramatsu, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Steven Rostedt, Thomas Gleixner, LKML,
	H . Peter Anvin, Anil S Keshavamurthy, David S . Miller,
	Ian McDonald, Vlad Yasevich, Stephen Hemminger

On Fri, Oct 20, 2017 at 8:17 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> On Fri, Oct 20, 2017 at 5:22 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > Ok, could we now also fix these:
>> >
>> > net/dccp/probe.c:166:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
>> > net/dccp/probe.c:170:4: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
>> > net/dccp/probe.c:190:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
>> > drivers/misc/lkdtm_core.c:317:3: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
>> > drivers/misc/lkdtm_core.c:322:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
>> > drivers/misc/lkdtm_core.c:560:3: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
>> > net/sctp/probe.c:189:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
>> > net/sctp/probe.c:194:3: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
>> > net/sctp/probe.c:240:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
>> > net/ipv4/tcp_probe.c:280:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
>> > net/ipv4/tcp_probe.c:298:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
>> >
>> > I'd suggest removing the networking ones and Cc:-ing it to the networking folks -
>> > I strongly doubt anyone is using that functionality for real.
>> >
>> > The LKDTM one Kees had some ideas (patches?) for?
>>
>> Oops, yes, I've sent this patch to Greg now. I had let it sit for a
>> few days of 0-day testing and promptly forgot about it. ;)
>
> Could we, as a special exception, carry this patch in -tip, so that the probes
> related changes stay togher - or is it too distruptive to the flow of other
> in-flight LKDTM patches, creating conflicts, etc.?

I have no problem with that, sure; these were the only outstanding
lkdtm patches (one for jprobes removal, and another for const
clean-ups noticed during the first patch's work). However, Greg pulled
them to his -testing tree already, so I guess it's up to him. Just let
me know what I can do to help. :)

-Kees

-- 
Kees Cook
Pixel Security

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

* Re: [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs
  2017-10-20 16:28       ` Kees Cook
@ 2017-10-21  8:06         ` Greg Kroah-Hartman
  0 siblings, 0 replies; 38+ messages in thread
From: Greg Kroah-Hartman @ 2017-10-21  8:06 UTC (permalink / raw)
  To: Kees Cook
  Cc: Ingo Molnar, Masami Hiramatsu, Linus Torvalds, Peter Zijlstra,
	Alexei Starovoitov, Ananth N Mavinakayanahalli,
	Paul E . McKenney, Steven Rostedt, Thomas Gleixner, LKML,
	H . Peter Anvin, Anil S Keshavamurthy, David S . Miller,
	Ian McDonald, Vlad Yasevich, Stephen Hemminger

On Fri, Oct 20, 2017 at 09:28:33AM -0700, Kees Cook wrote:
> On Fri, Oct 20, 2017 at 8:17 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Kees Cook <keescook@chromium.org> wrote:
> >
> >> On Fri, Oct 20, 2017 at 5:22 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >
> >> > Ok, could we now also fix these:
> >> >
> >> > net/dccp/probe.c:166:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> >> > net/dccp/probe.c:170:4: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> >> > net/dccp/probe.c:190:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> >> > drivers/misc/lkdtm_core.c:317:3: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> >> > drivers/misc/lkdtm_core.c:322:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> >> > drivers/misc/lkdtm_core.c:560:3: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> >> > net/sctp/probe.c:189:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> >> > net/sctp/probe.c:194:3: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> >> > net/sctp/probe.c:240:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> >> > net/ipv4/tcp_probe.c:280:2: warning: ‘register_jprobe’ is deprecated [-Wdeprecated-declarations]
> >> > net/ipv4/tcp_probe.c:298:2: warning: ‘unregister_jprobe’ is deprecated [-Wdeprecated-declarations]
> >> >
> >> > I'd suggest removing the networking ones and Cc:-ing it to the networking folks -
> >> > I strongly doubt anyone is using that functionality for real.
> >> >
> >> > The LKDTM one Kees had some ideas (patches?) for?
> >>
> >> Oops, yes, I've sent this patch to Greg now. I had let it sit for a
> >> few days of 0-day testing and promptly forgot about it. ;)
> >
> > Could we, as a special exception, carry this patch in -tip, so that the probes
> > related changes stay togher - or is it too distruptive to the flow of other
> > in-flight LKDTM patches, creating conflicts, etc.?
> 
> I have no problem with that, sure; these were the only outstanding
> lkdtm patches (one for jprobes removal, and another for const
> clean-ups noticed during the first patch's work). However, Greg pulled
> them to his -testing tree already, so I guess it's up to him. Just let
> me know what I can do to help. :)

It's in my tree as well, Ingo, you can take it too, there shouldn't be
any merge issues that I can tell.

thanks,

greg k-h

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

end of thread, other threads:[~2017-10-21  8:06 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-05 23:13 [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
2017-10-05 23:13 ` [RFC PATCH -tip 1/5] kprobes: Use ENOTSUPP instead of ENOSYS Masami Hiramatsu
2017-10-20  8:57   ` Ingo Molnar
2017-10-20 15:51     ` Masami Hiramatsu
2017-10-05 23:14 ` [RFC PATCH -tip 2/5] kprobes: Abolish jprobe APIs Masami Hiramatsu
2017-10-20 12:26   ` [tip:perf/core] kprobes: Disable the jprobes APIs tip-bot for Masami Hiramatsu
2017-10-05 23:15 ` [RFC PATCH -tip 3/5] kprobes: Disable jprobe test code Masami Hiramatsu
2017-10-20 12:26   ` [tip:perf/core] kprobes: Disable the jprobes " tip-bot for Masami Hiramatsu
2017-10-05 23:15 ` [RFC PATCH -tip 4/5] kprobes: Remove jprobe sample code Masami Hiramatsu
2017-10-20 12:27   ` [tip:perf/core] kprobes: Remove the jprobes " tip-bot for Masami Hiramatsu
2017-10-05 23:16 ` [RFC PATCH -tip 5/5] kprobes: docs: Remove jprobe related document Masami Hiramatsu
2017-10-20 12:27   ` [tip:perf/core] kprobes/docs: Remove jprobes related documents tip-bot for Masami Hiramatsu
2017-10-05 23:35 ` [RFC PATCH -tip 0/5] kprobes: Abolish jprobe APIs Kees Cook
2017-10-05 23:58   ` Steven Rostedt
2017-10-06  0:06     ` Kees Cook
2017-10-06  4:49     ` Masami Hiramatsu
2017-10-06 12:58       ` Steven Rostedt
2017-10-06 15:34       ` Steven Rostedt
2017-10-07  5:24         ` Stafford Horne
2017-10-09 16:48           ` Steven Rostedt
2017-10-07  8:55         ` Ingo Molnar
2017-10-09 16:45           ` Steven Rostedt
2017-10-07  9:35         ` Masami Hiramatsu
2017-10-09 16:59           ` Steven Rostedt
2017-10-09 15:33         ` Jonathan Corbet
2017-10-09 16:20           ` Steven Rostedt
2017-10-09 16:33             ` Jonathan Corbet
2017-10-09 16:41               ` Steven Rostedt
2017-10-09 18:10           ` Steven Rostedt
2017-10-10 14:02           ` Steven Rostedt
2017-10-06  0:32   ` Masami Hiramatsu
2017-10-06  1:11     ` Steven Rostedt
2017-10-06  4:47       ` Masami Hiramatsu
2017-10-20 12:22 ` Ingo Molnar
2017-10-20 13:32   ` Kees Cook
2017-10-20 15:17     ` Ingo Molnar
2017-10-20 16:28       ` Kees Cook
2017-10-21  8:06         ` Greg Kroah-Hartman

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