[PATCH 0/4] tracing: fprobe-events: Register fprobe only when the event is enabled

[PATCH 0/4] tracing: fprobe-events: Register fprobe only when the event is enabled

[Masami Hiramatsu (Google)]

Hi,
Here is a series of patches to register fprobe only when the fprobe
event is enabled. Steve suggested the fprobe-events are always
registered when it is defined, even if it is disabled and that makes
system overhead. This series registeres the fprobes only when the event
is enabled.

NOTE: tracepoint has a AB-BA locking issue[*], so it is still enabled
when the event is defined. That should be eventually solved, but it may
need more complicated change. Thus this series focuses on registering
fprobe when the event is enabled.


(*) AB-BA lock if we enable tracepoint when enabling tprobe event:

[event enable]
__ftrace_event_enable_disable() ----> event_mutex
  __regsiter_tracepoint_fprobe()
    find_tracepoint()
      for_each_module_tracepoint() ----> tracepoint_module_list_mutex

[module loading]
prepare_coming_module()
  tracepoint_module_notify()
    tracepoint_module_coming() ----> tracepoint_module_list_mutex
      __tracepoint_probe_module_cb() ---> event_mutex 

I have an idea to defer fprobe event enablement when module loading
so that we can avoid taking event_mutex in
__tracepoint_probe_module_cb(), but it needs to introduce another
list of tracepoint_user and another mutex.

Thank you,

---

Masami Hiramatsu (Google) (4):
      tracing: tprobe-events: Remove mod field from tprobe-event
      tracing: tprobe-events: Support multiple tprobes on the same tracepoint
      tracing: fprobe-events: Register fprobe-events only when it is enabled
      selftests: tracing: Enable fprobe events before checking enable_functions


 include/linux/fprobe.h                             |    8 
 include/linux/module.h                             |    4 
 kernel/trace/fprobe.c                              |   29 +
 kernel/trace/trace_fprobe.c                        |  490 +++++++++++++-------
 .../ftrace/test.d/dynevent/add_remove_fprobe.tc    |   30 +
 5 files changed, 362 insertions(+), 199 deletions(-)

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

[PATCH 1/4] tracing: tprobe-events: Remove mod field from tprobe-event

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Remove unneeded 'mod' struct module pointer field from trace_fprobe
because we don't need to save this info.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_fprobe.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index 5d7ca80173ea..08def94f9ca6 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -46,7 +46,6 @@ struct trace_fprobe {
 	struct fprobe		fp;
 	const char		*symbol;
 	struct tracepoint	*tpoint;
-	struct module		*mod;
 	struct trace_probe	tp;
 };
 
@@ -426,7 +425,6 @@ static struct trace_fprobe *alloc_trace_fprobe(const char *group,
 					       const char *event,
 					       const char *symbol,
 					       struct tracepoint *tpoint,
-					       struct module *mod,
 					       int nargs, bool is_return)
 {
 	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
@@ -446,7 +444,6 @@ static struct trace_fprobe *alloc_trace_fprobe(const char *group,
 		tf->fp.entry_handler = fentry_dispatcher;
 
 	tf->tpoint = tpoint;
-	tf->mod = mod;
 
 	ret = trace_probe_init(&tf->tp, event, group, false, nargs);
 	if (ret < 0)
@@ -776,7 +773,6 @@ static void __unregister_trace_fprobe(struct trace_fprobe *tf)
 			tracepoint_probe_unregister(tf->tpoint,
 					tf->tpoint->probestub, NULL);
 			tf->tpoint = NULL;
-			tf->mod = NULL;
 		}
 	}
 }
@@ -992,7 +988,6 @@ static int __tracepoint_probe_module_cb(struct notifier_block *self,
 			tpoint = find_tracepoint_in_module(tp_mod->mod, tf->symbol);
 			if (tpoint) {
 				tf->tpoint = tpoint;
-				tf->mod = tp_mod->mod;
 				if (!WARN_ON_ONCE(__regsiter_tracepoint_fprobe(tf)) &&
 				    trace_probe_is_enabled(&tf->tp))
 					reenable_trace_fprobe(tf);
@@ -1003,7 +998,6 @@ static int __tracepoint_probe_module_cb(struct notifier_block *self,
 				tracepoint_probe_unregister(tf->tpoint,
 					tf->tpoint->probestub, NULL);
 				tf->tpoint = TRACEPOINT_STUB;
-				tf->mod = NULL;
 			}
 		}
 	}
@@ -1210,8 +1204,7 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 		return ret;
 
 	/* setup a probe */
-	tf = alloc_trace_fprobe(group, event, symbol, tpoint, tp_mod,
-				argc, is_return);
+	tf = alloc_trace_fprobe(group, event, symbol, tpoint, argc, is_return);
 	if (IS_ERR(tf)) {
 		ret = PTR_ERR(tf);
 		/* This must return -ENOMEM, else there is a bug */

[PATCH 2/4] tracing: tprobe-events: Support multiple tprobes on the same tracepoint

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Allow user to set multiple tracepoint-probe events on the same
tracepoint. After the last tprobe-event is removed, the tracepoint
callback is unregistered.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 include/linux/module.h      |    4 +
 kernel/trace/trace_fprobe.c |  259 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 208 insertions(+), 55 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 30e5b19bafa9..01d5208cf473 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -14,6 +14,7 @@
 #include <linux/buildid.h>
 #include <linux/compiler.h>
 #include <linux/cache.h>
+#include <linux/cleanup.h>
 #include <linux/kmod.h>
 #include <linux/init.h>
 #include <linux/elf.h>
@@ -1027,4 +1028,7 @@ static inline unsigned long find_kallsyms_symbol_value(struct module *mod,
 
 #endif  /* CONFIG_MODULES && CONFIG_KALLSYMS */
 
+/* Define __free(module_put) macro for struct module *. */
+DEFINE_FREE(module_put, struct module *, if (_T) module_put(_T))
+
 #endif /* _LINUX_MODULE_H */
diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index 08def94f9ca6..863c9343a939 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -21,7 +21,6 @@
 #define FPROBE_EVENT_SYSTEM "fprobes"
 #define TRACEPOINT_EVENT_SYSTEM "tracepoints"
 #define RETHOOK_MAXACTIVE_MAX 4096
-#define TRACEPOINT_STUB ERR_PTR(-ENOENT)
 
 static int trace_fprobe_create(const char *raw_command);
 static int trace_fprobe_show(struct seq_file *m, struct dyn_event *ev);
@@ -38,6 +37,89 @@ static struct dyn_event_operations trace_fprobe_ops = {
 	.match = trace_fprobe_match,
 };
 
+struct tracepoint_user {
+	struct tracepoint	*tpoint;
+	unsigned int		refcount;
+};
+
+static bool tracepoint_user_is_registered(struct tracepoint_user *tuser)
+{
+	return tuser && tuser->tpoint;
+}
+
+static int tracepoint_user_register(struct tracepoint_user *tuser)
+{
+	struct tracepoint *tpoint = tuser->tpoint;
+
+	if (!tpoint)
+		return 0;
+
+	return tracepoint_probe_register_prio_may_exist(tpoint,
+					tpoint->probestub, NULL, 0);
+}
+
+static void tracepoint_user_unregister(struct tracepoint_user *tuser)
+{
+	if (!tuser->tpoint)
+		return;
+
+	WARN_ON_ONCE(tracepoint_probe_unregister(tuser->tpoint, tuser->tpoint->probestub, NULL));
+	tuser->tpoint = NULL;
+}
+
+static unsigned long tracepoint_user_ip(struct tracepoint_user *tuser)
+{
+	if (!tuser->tpoint)
+		return 0UL;
+
+	return (unsigned long)tuser->tpoint->probestub;
+}
+
+static bool tracepoint_user_within_module(struct tracepoint_user *tuser,
+					  struct module *mod)
+{
+	return within_module(tracepoint_user_ip(tuser), mod);
+}
+
+static struct tracepoint_user *tracepoint_user_allocate(struct tracepoint *tpoint)
+{
+	struct tracepoint_user *tuser __free(kfree) = NULL;
+
+	tuser = kzalloc(sizeof(*tuser), GFP_KERNEL);
+	if (!tuser)
+		return NULL;
+	tuser->tpoint = tpoint;
+	tuser->refcount = 1;
+
+	return_ptr(tuser);
+}
+
+/* These must be called with event_mutex */
+static void tracepoint_user_get(struct tracepoint_user *tuser)
+{
+	tuser->refcount++;
+}
+
+static void tracepoint_user_put(struct tracepoint_user *tuser)
+{
+	if (--tuser->refcount > 0)
+		return;
+
+	if (tracepoint_user_is_registered(tuser))
+		tracepoint_user_unregister(tuser);
+	kfree(tuser);
+}
+
+static const char *tracepoint_user_lookup(struct tracepoint_user *tuser, char *buf)
+{
+	struct tracepoint *tpoint = tuser->tpoint;
+
+	if (!tpoint)
+		return NULL;
+
+	return kallsyms_lookup((unsigned long)tpoint->probestub, NULL, NULL, NULL, buf);
+}
+
 /*
  * Fprobe event core functions
  */
@@ -45,7 +127,7 @@ struct trace_fprobe {
 	struct dyn_event	devent;
 	struct fprobe		fp;
 	const char		*symbol;
-	struct tracepoint	*tpoint;
+	struct tracepoint_user	*tuser;
 	struct trace_probe	tp;
 };
 
@@ -75,7 +157,7 @@ static bool trace_fprobe_is_return(struct trace_fprobe *tf)
 
 static bool trace_fprobe_is_tracepoint(struct trace_fprobe *tf)
 {
-	return tf->tpoint != NULL;
+	return tf->tuser != NULL;
 }
 
 static const char *trace_fprobe_symbol(struct trace_fprobe *tf)
@@ -125,6 +207,57 @@ static bool trace_fprobe_is_registered(struct trace_fprobe *tf)
 	return fprobe_is_registered(&tf->fp);
 }
 
+static struct tracepoint *find_tracepoint(const char *tp_name,
+	struct module **tp_mod);
+
+/*
+ * Get tracepoint_user if exist, or allocate new one. If tracepoint is on a
+ * module, get its refcounter.
+ */
+static struct tracepoint_user *
+trace_fprobe_get_tracepoint_user(const char *name, struct module **pmod)
+{
+	struct tracepoint_user *tuser __free(kfree) = NULL;
+	struct tracepoint *tpoint;
+	struct trace_fprobe *tf;
+	struct dyn_event *dpos;
+	struct module *mod __free(module_put) = NULL;
+	int ret;
+
+	tpoint = find_tracepoint(name, &mod);
+	if (mod && !try_module_get(mod)) {
+		mod = NULL;
+		tpoint = NULL;
+	}
+	/* tpoint can be NULL, but we don't care here. */
+
+	/* Search existing tracepoint_user */
+	for_each_trace_fprobe(tf, dpos) {
+		if (!trace_fprobe_is_tracepoint(tf))
+			continue;
+		if (!strcmp(tf->symbol, name)) {
+			tracepoint_user_get(tf->tuser);
+			*pmod = no_free_ptr(mod);
+			return tf->tuser;
+		}
+	}
+
+	/* Not found, allocate and register new tracepoint_user. */
+	tuser = tracepoint_user_allocate(tpoint);
+	if (!tuser)
+		return NULL;
+
+	if (tpoint) {
+		/* If the tracepoint is not loaded, tpoint can be NULL. */
+		ret = tracepoint_user_register(tuser);
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
+	*pmod = no_free_ptr(mod);
+	return_ptr(tuser);
+}
+
 /*
  * Note that we don't verify the fetch_insn code, since it does not come
  * from user space.
@@ -410,6 +543,8 @@ static void free_trace_fprobe(struct trace_fprobe *tf)
 {
 	if (tf) {
 		trace_probe_cleanup(&tf->tp);
+		if (tf->tuser)
+			tracepoint_user_put(tf->tuser);
 		kfree(tf->symbol);
 		kfree(tf);
 	}
@@ -424,7 +559,7 @@ DEFINE_FREE(free_trace_fprobe, struct trace_fprobe *, if (!IS_ERR_OR_NULL(_T)) f
 static struct trace_fprobe *alloc_trace_fprobe(const char *group,
 					       const char *event,
 					       const char *symbol,
-					       struct tracepoint *tpoint,
+					       struct tracepoint_user *tuser,
 					       int nargs, bool is_return)
 {
 	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
@@ -443,7 +578,7 @@ static struct trace_fprobe *alloc_trace_fprobe(const char *group,
 	else
 		tf->fp.entry_handler = fentry_dispatcher;
 
-	tf->tpoint = tpoint;
+	tf->tuser = tuser;
 
 	ret = trace_probe_init(&tf->tp, event, group, false, nargs);
 	if (ret < 0)
@@ -709,19 +844,11 @@ static int unregister_fprobe_event(struct trace_fprobe *tf)
 
 static int __regsiter_tracepoint_fprobe(struct trace_fprobe *tf)
 {
-	struct tracepoint *tpoint = tf->tpoint;
-	unsigned long ip = (unsigned long)tpoint->probestub;
-	int ret;
+	unsigned long ip = tracepoint_user_ip(tf->tuser);
+
+	if (!ip)
+		return -ENOENT;
 
-	/*
-	 * Here, we do 2 steps to enable fprobe on a tracepoint.
-	 * At first, put __probestub_##TP function on the tracepoint
-	 * and put a fprobe on the stub function.
-	 */
-	ret = tracepoint_probe_register_prio_may_exist(tpoint,
-				tpoint->probestub, NULL, 0);
-	if (ret < 0)
-		return ret;
 	return register_fprobe_ips(&tf->fp, &ip, 1);
 }
 
@@ -753,7 +880,7 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
 	if (trace_fprobe_is_tracepoint(tf)) {
 
 		/* This tracepoint is not loaded yet */
-		if (tf->tpoint == TRACEPOINT_STUB)
+		if (!tracepoint_user_is_registered(tf->tuser))
 			return 0;
 
 		return __regsiter_tracepoint_fprobe(tf);
@@ -770,9 +897,8 @@ static void __unregister_trace_fprobe(struct trace_fprobe *tf)
 		unregister_fprobe(&tf->fp);
 		memset(&tf->fp, 0, sizeof(tf->fp));
 		if (trace_fprobe_is_tracepoint(tf)) {
-			tracepoint_probe_unregister(tf->tpoint,
-					tf->tpoint->probestub, NULL);
-			tf->tpoint = NULL;
+			tracepoint_user_put(tf->tuser);
+			tf->tuser = NULL;
 		}
 	}
 }
@@ -975,7 +1101,7 @@ static int __tracepoint_probe_module_cb(struct notifier_block *self,
 					unsigned long val, void *data)
 {
 	struct tp_module *tp_mod = data;
-	struct tracepoint *tpoint;
+	struct tracepoint_user *tuser;
 	struct trace_fprobe *tf;
 	struct dyn_event *pos;
 
@@ -984,21 +1110,48 @@ static int __tracepoint_probe_module_cb(struct notifier_block *self,
 
 	mutex_lock(&event_mutex);
 	for_each_trace_fprobe(tf, pos) {
-		if (val == MODULE_STATE_COMING && tf->tpoint == TRACEPOINT_STUB) {
+		if (!trace_fprobe_is_tracepoint(tf))
+			continue;
+
+		if (val == MODULE_STATE_COMING) {
+			/*
+			 * If any tracepoint used by tprobe is in the module,
+			 * register the stub.
+			 */
+			struct tracepoint *tpoint;
+
 			tpoint = find_tracepoint_in_module(tp_mod->mod, tf->symbol);
-			if (tpoint) {
-				tf->tpoint = tpoint;
-				if (!WARN_ON_ONCE(__regsiter_tracepoint_fprobe(tf)) &&
-				    trace_probe_is_enabled(&tf->tp))
-					reenable_trace_fprobe(tf);
-			}
-		} else if (val == MODULE_STATE_GOING && tp_mod->mod == tf->mod) {
-			unregister_fprobe(&tf->fp);
-			if (trace_fprobe_is_tracepoint(tf)) {
-				tracepoint_probe_unregister(tf->tpoint,
-					tf->tpoint->probestub, NULL);
-				tf->tpoint = TRACEPOINT_STUB;
+			/* This is not a tracepoint in this module. Skip it. */
+			if (!tpoint)
+				continue;
+
+			tuser = tf->tuser;
+			/* If the tracepoint is no registered yet, register it. */
+			if (!tracepoint_user_is_registered(tuser)) {
+				tuser->tpoint = tpoint;
+				if (WARN_ON_ONCE(tracepoint_user_register(tuser)))
+					continue;
 			}
+
+			/* Finally enable fprobe on this module. */
+			if (!WARN_ON_ONCE(__regsiter_tracepoint_fprobe(tf)) &&
+			    trace_probe_is_enabled(&tf->tp))
+				reenable_trace_fprobe(tf);
+		} else if (val == MODULE_STATE_GOING) {
+			tuser = tf->tuser;
+			/* Unregister all tracepoint_user in this module. */
+			if (tracepoint_user_is_registered(tuser) &&
+			    tracepoint_user_within_module(tuser, tp_mod->mod))
+				tracepoint_user_unregister(tuser);
+
+			/*
+			 * Here we need to handle shared tracepoint_user case.
+			 * Such tuser is unregistered, but trace_fprobe itself
+			 * is registered. (Note this only handles tprobes.)
+			 */
+			if (!tracepoint_user_is_registered(tuser) &&
+			    trace_fprobe_is_registered(tf))
+				unregister_fprobe(&tf->fp);
 		}
 	}
 	mutex_unlock(&event_mutex);
@@ -1067,7 +1220,9 @@ static int parse_symbol_and_return(int argc, const char *argv[],
 	return 0;
 }
 
-DEFINE_FREE(module_put, struct module *, if (_T) module_put(_T))
+DEFINE_FREE(tuser_put, struct tracepoint_user *,
+	if (!IS_ERR_OR_NULL(_T))
+		tracepoint_user_put(_T))
 
 static int trace_fprobe_create_internal(int argc, const char *argv[],
 					struct traceprobe_parse_context *ctx)
@@ -1097,6 +1252,8 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 	 *  FETCHARG:TYPE : use TYPE instead of unsigned long.
 	 */
 	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
+	struct tracepoint_user *tuser __free(tuser_put) = NULL;
+	struct module *mod __free(module_put) = NULL;
 	int i, new_argc = 0, ret = 0;
 	bool is_return = false;
 	char *symbol __free(kfree) = NULL;
@@ -1108,8 +1265,6 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 	char abuf[MAX_BTF_ARGS_LEN];
 	char *dbuf __free(kfree) = NULL;
 	bool is_tracepoint = false;
-	struct module *tp_mod __free(module_put) = NULL;
-	struct tracepoint *tpoint = NULL;
 
 	if ((argv[0][0] != 'f' && argv[0][0] != 't') || argc < 2)
 		return -ECANCELED;
@@ -1162,25 +1317,18 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 
 	if (is_tracepoint) {
 		ctx->flags |= TPARG_FL_TPOINT;
-		tpoint = find_tracepoint(symbol, &tp_mod);
-		/* lock module until register this tprobe. */
-		if (tp_mod && !try_module_get(tp_mod)) {
-			tpoint = NULL;
-			tp_mod = NULL;
-		}
-		if (tpoint) {
-			ctx->funcname = kallsyms_lookup(
-				(unsigned long)tpoint->probestub,
-				NULL, NULL, NULL, sbuf);
-		} else if (IS_ENABLED(CONFIG_MODULES)) {
-				/* This *may* be loaded afterwards */
-				tpoint = TRACEPOINT_STUB;
-				ctx->funcname = symbol;
-		} else {
+		tuser = trace_fprobe_get_tracepoint_user(symbol, &mod);
+		if (!tuser)
+			return -ENOMEM;
+		if (IS_ERR(tuser)) {
 			trace_probe_log_set_index(1);
 			trace_probe_log_err(0, NO_TRACEPOINT);
-			return -EINVAL;
+			return PTR_ERR(tuser);
 		}
+		ctx->funcname = tracepoint_user_lookup(tuser, sbuf);
+		/* If tracepoint is not loaded yet, use symbol name as funcname. */
+		if (!ctx->funcname)
+			ctx->funcname = symbol;
 	} else
 		ctx->funcname = symbol;
 
@@ -1204,13 +1352,14 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 		return ret;
 
 	/* setup a probe */
-	tf = alloc_trace_fprobe(group, event, symbol, tpoint, argc, is_return);
+	tf = alloc_trace_fprobe(group, event, symbol, tuser, argc, is_return);
 	if (IS_ERR(tf)) {
 		ret = PTR_ERR(tf);
 		/* This must return -ENOMEM, else there is a bug */
 		WARN_ON_ONCE(ret != -ENOMEM);
 		return ret;
 	}
+	tuser = NULL; /* Move tuser to tf. */
 
 	/* parse arguments */
 	for (i = 0; i < argc; i++) {

[PATCH 3/4] tracing: fprobe-events: Register fprobe-events only when it is enabled

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Currently fprobe events are registered when it is defined. Thus it will
give some overhead even if it is disabled. This changes it to register the
fprobe only when it is enabled.

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 include/linux/fprobe.h      |    8 +
 kernel/trace/fprobe.c       |   29 +++--
 kernel/trace/trace_fprobe.c |  234 +++++++++++++++++++++----------------------
 3 files changed, 140 insertions(+), 131 deletions(-)

diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
index 702099f08929..9635a24d5a25 100644
--- a/include/linux/fprobe.h
+++ b/include/linux/fprobe.h
@@ -94,6 +94,8 @@ int register_fprobe_ips(struct fprobe *fp, unsigned long *addrs, int num);
 int register_fprobe_syms(struct fprobe *fp, const char **syms, int num);
 int unregister_fprobe(struct fprobe *fp);
 bool fprobe_is_registered(struct fprobe *fp);
+int fprobe_alloc_ip_list_from_filter(const char *filter, const char *notfilter,
+	unsigned long **addrs);
 #else
 static inline int register_fprobe(struct fprobe *fp, const char *filter, const char *notfilter)
 {
@@ -115,6 +117,12 @@ static inline bool fprobe_is_registered(struct fprobe *fp)
 {
 	return false;
 }
+static inline int fprobe_alloc_ip_list_from_filter(const char *filter,
+						   const char *notfilter,
+						   unsigned long **addrs)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 
 /**
diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index 33082c4e8154..05050f1c2239 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -486,6 +486,24 @@ static int ip_list_from_filter(const char *filter, const char *notfilter,
 	return match.index ?: -ENOENT;
 }
 
+#define FPROBE_IPS_MAX	INT_MAX
+
+int fprobe_alloc_ip_list_from_filter(const char *filter, const char *notfilter,
+				     unsigned long **addrs)
+{
+	int ret;
+
+	/* Count the number of ips from filter. */
+	ret = ip_list_from_filter(filter, notfilter, NULL, FPROBE_IPS_MAX);
+	if (ret < 0)
+		return ret;
+
+	*addrs = kcalloc(ret, sizeof(unsigned long), GFP_KERNEL);
+	if (!*addrs)
+		return -ENOMEM;
+	return ip_list_from_filter(filter, notfilter, *addrs, ret);
+}
+
 static void fprobe_fail_cleanup(struct fprobe *fp)
 {
 	kfree(fp->hlist_array);
@@ -528,8 +546,6 @@ static int fprobe_init(struct fprobe *fp, unsigned long *addrs, int num)
 	return 0;
 }
 
-#define FPROBE_IPS_MAX	INT_MAX
-
 /**
  * register_fprobe() - Register fprobe to ftrace by pattern.
  * @fp: A fprobe data structure to be registered.
@@ -549,14 +565,7 @@ int register_fprobe(struct fprobe *fp, const char *filter, const char *notfilter
 	if (!fp || !filter)
 		return -EINVAL;
 
-	ret = ip_list_from_filter(filter, notfilter, NULL, FPROBE_IPS_MAX);
-	if (ret < 0)
-		return ret;
-
-	addrs = kcalloc(ret, sizeof(unsigned long), GFP_KERNEL);
-	if (!addrs)
-		return -ENOMEM;
-	ret = ip_list_from_filter(filter, notfilter, addrs, ret);
+	ret = fprobe_alloc_ip_list_from_filter(filter, notfilter, &addrs);
 	if (ret > 0)
 		ret = register_fprobe_ips(fp, addrs, ret);
 
diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index 863c9343a939..ac3b0a34810a 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -601,98 +601,6 @@ static struct trace_fprobe *find_trace_fprobe(const char *event,
 	return NULL;
 }
 
-static inline int __enable_trace_fprobe(struct trace_fprobe *tf)
-{
-	if (trace_fprobe_is_registered(tf))
-		enable_fprobe(&tf->fp);
-
-	return 0;
-}
-
-static void __disable_trace_fprobe(struct trace_probe *tp)
-{
-	struct trace_fprobe *tf;
-
-	list_for_each_entry(tf, trace_probe_probe_list(tp), tp.list) {
-		if (!trace_fprobe_is_registered(tf))
-			continue;
-		disable_fprobe(&tf->fp);
-	}
-}
-
-/*
- * Enable trace_probe
- * if the file is NULL, enable "perf" handler, or enable "trace" handler.
- */
-static int enable_trace_fprobe(struct trace_event_call *call,
-			       struct trace_event_file *file)
-{
-	struct trace_probe *tp;
-	struct trace_fprobe *tf;
-	bool enabled;
-	int ret = 0;
-
-	tp = trace_probe_primary_from_call(call);
-	if (WARN_ON_ONCE(!tp))
-		return -ENODEV;
-	enabled = trace_probe_is_enabled(tp);
-
-	/* This also changes "enabled" state */
-	if (file) {
-		ret = trace_probe_add_file(tp, file);
-		if (ret)
-			return ret;
-	} else
-		trace_probe_set_flag(tp, TP_FLAG_PROFILE);
-
-	if (!enabled) {
-		list_for_each_entry(tf, trace_probe_probe_list(tp), tp.list) {
-			/* TODO: check the fprobe is gone */
-			__enable_trace_fprobe(tf);
-		}
-	}
-
-	return 0;
-}
-
-/*
- * Disable trace_probe
- * if the file is NULL, disable "perf" handler, or disable "trace" handler.
- */
-static int disable_trace_fprobe(struct trace_event_call *call,
-				struct trace_event_file *file)
-{
-	struct trace_probe *tp;
-
-	tp = trace_probe_primary_from_call(call);
-	if (WARN_ON_ONCE(!tp))
-		return -ENODEV;
-
-	if (file) {
-		if (!trace_probe_get_file_link(tp, file))
-			return -ENOENT;
-		if (!trace_probe_has_single_file(tp))
-			goto out;
-		trace_probe_clear_flag(tp, TP_FLAG_TRACE);
-	} else
-		trace_probe_clear_flag(tp, TP_FLAG_PROFILE);
-
-	if (!trace_probe_is_enabled(tp))
-		__disable_trace_fprobe(tp);
-
- out:
-	if (file)
-		/*
-		 * Synchronization is done in below function. For perf event,
-		 * file == NULL and perf_trace_event_unreg() calls
-		 * tracepoint_synchronize_unregister() to ensure synchronize
-		 * event. We don't need to care about it.
-		 */
-		trace_probe_remove_file(tp, file);
-
-	return 0;
-}
-
 /* Event entry printers */
 static enum print_line_t
 print_fentry_event(struct trace_iterator *iter, int flags,
@@ -852,6 +760,26 @@ static int __regsiter_tracepoint_fprobe(struct trace_fprobe *tf)
 	return register_fprobe_ips(&tf->fp, &ip, 1);
 }
 
+/* Returns an error if the target function is not available, or 0 */
+static int trace_fprobe_verify_target(struct trace_fprobe *tf)
+{
+	unsigned long *addrs __free(kfree) = NULL;
+	int ret;
+
+	if (trace_fprobe_is_tracepoint(tf)) {
+
+		/* This tracepoint is not loaded yet */
+		if (!tracepoint_user_is_registered(tf->tuser))
+			return 0;
+
+		/* We assume all stab function is tracable. */
+		return tracepoint_user_ip(tf->tuser) ? 0 : -ENOENT;
+	}
+
+	ret = fprobe_alloc_ip_list_from_filter(tf->symbol, NULL, &addrs);
+	return (ret < 0) ? ret : 0;
+}
+
 /* Internal register function - just handle fprobe and flags */
 static int __register_trace_fprobe(struct trace_fprobe *tf)
 {
@@ -871,11 +799,7 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
 			return ret;
 	}
 
-	/* Set/clear disabled flag according to tp->flag */
-	if (trace_probe_is_enabled(&tf->tp))
-		tf->fp.flags &= ~FPROBE_FL_DISABLED;
-	else
-		tf->fp.flags |= FPROBE_FL_DISABLED;
+	tf->fp.flags &= ~FPROBE_FL_DISABLED;
 
 	if (trace_fprobe_is_tracepoint(tf)) {
 
@@ -896,10 +820,10 @@ static void __unregister_trace_fprobe(struct trace_fprobe *tf)
 	if (trace_fprobe_is_registered(tf)) {
 		unregister_fprobe(&tf->fp);
 		memset(&tf->fp, 0, sizeof(tf->fp));
-		if (trace_fprobe_is_tracepoint(tf)) {
-			tracepoint_user_put(tf->tuser);
-			tf->tuser = NULL;
-		}
+	}
+	if (trace_fprobe_is_tracepoint(tf)) {
+		tracepoint_user_put(tf->tuser);
+		tf->tuser = NULL;
 	}
 }
 
@@ -959,7 +883,7 @@ static bool trace_fprobe_has_same_fprobe(struct trace_fprobe *orig,
 	return false;
 }
 
-static int append_trace_fprobe(struct trace_fprobe *tf, struct trace_fprobe *to)
+static int append_trace_fprobe_event(struct trace_fprobe *tf, struct trace_fprobe *to)
 {
 	int ret;
 
@@ -987,7 +911,7 @@ static int append_trace_fprobe(struct trace_fprobe *tf, struct trace_fprobe *to)
 	if (ret)
 		return ret;
 
-	ret = __register_trace_fprobe(tf);
+	ret = trace_fprobe_verify_target(tf);
 	if (ret)
 		trace_probe_unlink(&tf->tp);
 	else
@@ -996,8 +920,8 @@ static int append_trace_fprobe(struct trace_fprobe *tf, struct trace_fprobe *to)
 	return ret;
 }
 
-/* Register a trace_probe and probe_event */
-static int register_trace_fprobe(struct trace_fprobe *tf)
+/* Register a trace_probe and probe_event, and check the fprobe is available. */
+static int register_trace_fprobe_event(struct trace_fprobe *tf)
 {
 	struct trace_fprobe *old_tf;
 	int ret;
@@ -1007,7 +931,7 @@ static int register_trace_fprobe(struct trace_fprobe *tf)
 	old_tf = find_trace_fprobe(trace_probe_name(&tf->tp),
 				   trace_probe_group_name(&tf->tp));
 	if (old_tf)
-		return append_trace_fprobe(tf, old_tf);
+		return append_trace_fprobe_event(tf, old_tf);
 
 	/* Register new event */
 	ret = register_fprobe_event(tf);
@@ -1020,8 +944,8 @@ static int register_trace_fprobe(struct trace_fprobe *tf)
 		return ret;
 	}
 
-	/* Register fprobe */
-	ret = __register_trace_fprobe(tf);
+	/* Verify fprobe is sane. */
+	ret = trace_fprobe_verify_target(tf);
 	if (ret < 0)
 		unregister_fprobe_event(tf);
 	else
@@ -1075,15 +999,6 @@ static struct tracepoint *find_tracepoint(const char *tp_name,
 }
 
 #ifdef CONFIG_MODULES
-static void reenable_trace_fprobe(struct trace_fprobe *tf)
-{
-	struct trace_probe *tp = &tf->tp;
-
-	list_for_each_entry(tf, trace_probe_probe_list(tp), tp.list) {
-		__enable_trace_fprobe(tf);
-	}
-}
-
 /* Find a tracepoint from specified module. */
 static struct tracepoint *find_tracepoint_in_module(struct module *mod,
 						    const char *tp_name)
@@ -1134,9 +1049,8 @@ static int __tracepoint_probe_module_cb(struct notifier_block *self,
 			}
 
 			/* Finally enable fprobe on this module. */
-			if (!WARN_ON_ONCE(__regsiter_tracepoint_fprobe(tf)) &&
-			    trace_probe_is_enabled(&tf->tp))
-				reenable_trace_fprobe(tf);
+			if (trace_probe_is_enabled(&tf->tp) && !trace_fprobe_is_registered(tf))
+				WARN_ON_ONCE(__regsiter_tracepoint_fprobe(tf));
 		} else if (val == MODULE_STATE_GOING) {
 			tuser = tf->tuser;
 			/* Unregister all tracepoint_user in this module. */
@@ -1385,7 +1299,7 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 	if (ret < 0)
 		return ret;
 
-	ret = register_trace_fprobe(tf);
+	ret = register_trace_fprobe_event(tf);
 	if (ret) {
 		trace_probe_log_set_index(1);
 		if (ret == -EILSEQ)
@@ -1454,6 +1368,84 @@ static int trace_fprobe_show(struct seq_file *m, struct dyn_event *ev)
 	return 0;
 }
 
+/*
+ * Enable trace_probe
+ * if the file is NULL, enable "perf" handler, or enable "trace" handler.
+ */
+static int enable_trace_fprobe(struct trace_event_call *call,
+			       struct trace_event_file *file)
+{
+	struct trace_probe *tp;
+	struct trace_fprobe *tf;
+	bool enabled;
+	int ret = 0;
+
+	tp = trace_probe_primary_from_call(call);
+	if (WARN_ON_ONCE(!tp))
+		return -ENODEV;
+	enabled = trace_probe_is_enabled(tp);
+
+	/* This also changes "enabled" state */
+	if (file) {
+		ret = trace_probe_add_file(tp, file);
+		if (ret)
+			return ret;
+	} else
+		trace_probe_set_flag(tp, TP_FLAG_PROFILE);
+
+	if (!enabled) {
+		list_for_each_entry(tf, trace_probe_probe_list(tp), tp.list) {
+			ret = __register_trace_fprobe(tf);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * Disable trace_probe
+ * if the file is NULL, disable "perf" handler, or disable "trace" handler.
+ */
+static int disable_trace_fprobe(struct trace_event_call *call,
+				struct trace_event_file *file)
+{
+	struct trace_fprobe *tf;
+	struct trace_probe *tp;
+
+	tp = trace_probe_primary_from_call(call);
+	if (WARN_ON_ONCE(!tp))
+		return -ENODEV;
+
+	if (file) {
+		if (!trace_probe_get_file_link(tp, file))
+			return -ENOENT;
+		if (!trace_probe_has_single_file(tp))
+			goto out;
+		trace_probe_clear_flag(tp, TP_FLAG_TRACE);
+	} else
+		trace_probe_clear_flag(tp, TP_FLAG_PROFILE);
+
+	if (!trace_probe_is_enabled(tp)) {
+		list_for_each_entry(tf, trace_probe_probe_list(tp), tp.list) {
+			unregister_fprobe(&tf->fp);
+		}
+	}
+
+ out:
+	if (file)
+		/*
+		 * Synchronization is done in below function. For perf event,
+		 * file == NULL and perf_trace_event_unreg() calls
+		 * tracepoint_synchronize_unregister() to ensure synchronize
+		 * event. We don't need to care about it.
+		 */
+		trace_probe_remove_file(tp, file);
+
+	return 0;
+}
+
 /*
  * called by perf_trace_init() or __ftrace_set_clr_event() under event_mutex.
  */

[PATCH 4/4] selftests: tracing: Enable fprobe events before checking enable_functions

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 .../ftrace/test.d/dynevent/add_remove_fprobe.tc    |   30 +++++++++++++-------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_fprobe.tc b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_fprobe.tc
index 449f9d8be746..603dad73e7e8 100644
--- a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_fprobe.tc
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_fprobe.tc
@@ -12,6 +12,18 @@ PLACE3="schedule_timeout"
 
 echo "f:myevent1 $PLACE" >> dynamic_events
 
+echo "f:myevent2 $PLACE%return" >> dynamic_events
+
+# add another event
+echo "f:myevent3 $PLACE2" >> dynamic_events
+
+grep -q myevent1 dynamic_events
+grep -q myevent2 dynamic_events
+grep -q myevent3 dynamic_events
+test -d events/fprobes/myevent1
+test -d events/fprobes/myevent2
+
+echo 1 > events/fprobes/myevent1/enable
 # Make sure the event is attached and is the only one
 grep -q $PLACE enabled_functions
 cnt=`cat enabled_functions | wc -l`
@@ -19,29 +31,22 @@ if [ $cnt -ne 1 ]; then
 	exit_fail
 fi
 
-echo "f:myevent2 $PLACE%return" >> dynamic_events
-
+echo 1 > events/fprobes/myevent2/enable
 # It should till be the only attached function
 cnt=`cat enabled_functions | wc -l`
 if [ $cnt -ne 1 ]; then
 	exit_fail
 fi
 
-# add another event
-echo "f:myevent3 $PLACE2" >> dynamic_events
-
+echo 1 > events/fprobes/myevent3/enable
+# If the function is different, the attached function should be increased
 grep -q $PLACE2 enabled_functions
 cnt=`cat enabled_functions | wc -l`
 if [ $cnt -ne 2 ]; then
 	exit_fail
 fi
 
-grep -q myevent1 dynamic_events
-grep -q myevent2 dynamic_events
-grep -q myevent3 dynamic_events
-test -d events/fprobes/myevent1
-test -d events/fprobes/myevent2
-
+echo 0 > events/fprobes/myevent2/enable
 echo "-:myevent2" >> dynamic_events
 
 grep -q myevent1 dynamic_events
@@ -53,6 +58,7 @@ if [ $cnt -ne 2 ]; then
 	exit_fail
 fi
 
+echo 0 > events/fprobes/enable
 echo > dynamic_events
 
 # Should have none left
@@ -63,12 +69,14 @@ fi
 
 echo "f:myevent4 $PLACE" >> dynamic_events
 
+echo 1 > events/fprobes/myevent4/enable
 # Should only have one enabled
 cnt=`cat enabled_functions | wc -l`
 if [ $cnt -ne 1 ]; then
 	exit_fail
 fi
 
+echo 0 > events/fprobes/enable
 echo > dynamic_events
 
 # Should have none left

[RFC PATCH] tracing: tprobe-events: Register tracepoint when enable tprobe event

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

As same as fprobe, register tracepoint stub function only when enabling
tprobe events. The major changes are introducing a list of
tracepoint_user and its lock, and tprobe_event_module_nb, which is
another module notifier for module loading/unloading.  By spliting the
lock from event_mutex and a module notifier for trace_fprobe, it
solved AB-BA lock dependency issue between event_mutex and
tracepoint_module_list_mutex.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_fprobe.c |  382 +++++++++++++++++++++++++------------------
 1 file changed, 218 insertions(+), 164 deletions(-)

diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index ac3b0a34810a..2c4352f4dc11 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c
@@ -7,7 +7,9 @@
 #include <asm/ptrace.h>
 
 #include <linux/fprobe.h>
+#include <linux/list.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/rculist.h>
 #include <linux/security.h>
 #include <linux/tracepoint.h>
@@ -37,15 +39,21 @@ static struct dyn_event_operations trace_fprobe_ops = {
 	.match = trace_fprobe_match,
 };
 
+/* List of tracepoint_user */
+static LIST_HEAD(tracepoint_user_list);
+static DEFINE_MUTEX(tracepoint_user_mutex);
+
+/* While living tracepoint_user, @tpoint can be NULL and @refcount != 0. */
 struct tracepoint_user {
+	struct list_head	list;
+	const char		*name;
 	struct tracepoint	*tpoint;
 	unsigned int		refcount;
 };
 
-static bool tracepoint_user_is_registered(struct tracepoint_user *tuser)
-{
-	return tuser && tuser->tpoint;
-}
+/* NOTE: you must lock tracepoint_user_mutex. */
+#define for_each_tracepoint_user(tuser)		\
+	list_for_each_entry(tuser, &tracepoint_user_list, list)
 
 static int tracepoint_user_register(struct tracepoint_user *tuser)
 {
@@ -75,58 +83,112 @@ static unsigned long tracepoint_user_ip(struct tracepoint_user *tuser)
 	return (unsigned long)tuser->tpoint->probestub;
 }
 
-static bool tracepoint_user_within_module(struct tracepoint_user *tuser,
-					  struct module *mod)
+static void __tracepoint_user_free(struct tracepoint_user *tuser)
 {
-	return within_module(tracepoint_user_ip(tuser), mod);
+	if (!tuser)
+		return;
+	kfree(tuser->name);
+	kfree(tuser);
 }
 
-static struct tracepoint_user *tracepoint_user_allocate(struct tracepoint *tpoint)
+DEFINE_FREE(tuser_free, struct tracepoint_user *, __tracepoint_user_free(_T))
+
+static struct tracepoint_user *__tracepoint_user_init(const char *name, struct tracepoint *tpoint)
 {
-	struct tracepoint_user *tuser __free(kfree) = NULL;
+	struct tracepoint_user *tuser __free(tuser_free) = NULL;
+	int ret;
 
 	tuser = kzalloc(sizeof(*tuser), GFP_KERNEL);
 	if (!tuser)
 		return NULL;
+	tuser->name = kstrdup(name, GFP_KERNEL);
+	if (!tuser->name)
+		return NULL;
+
+	if (tpoint) {
+		ret = tracepoint_user_register(tuser);
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
 	tuser->tpoint = tpoint;
 	tuser->refcount = 1;
+	INIT_LIST_HEAD(&tuser->list);
+	list_add(&tuser->list, &tracepoint_user_list);
 
 	return_ptr(tuser);
 }
 
-/* These must be called with event_mutex */
-static void tracepoint_user_get(struct tracepoint_user *tuser)
-{
-	tuser->refcount++;
-}
+static struct tracepoint *find_tracepoint(const char *tp_name,
+	struct module **tp_mod);
 
-static void tracepoint_user_put(struct tracepoint_user *tuser)
+/*
+ * Get tracepoint_user if exist, or allocate new one and register it.
+ * If tracepoint is on a module, get its refcounter too.
+ * This returns errno or NULL (not loaded yet) or tracepoint_user.
+ */
+static struct tracepoint_user *tracepoint_user_find_get(const char *name, struct module **pmod)
 {
-	if (--tuser->refcount > 0)
-		return;
+	struct module *mod __free(module_put) = NULL;
+	struct tracepoint_user *tuser;
+	struct tracepoint *tpoint;
 
-	if (tracepoint_user_is_registered(tuser))
-		tracepoint_user_unregister(tuser);
-	kfree(tuser);
+	/* Get and lock the module which has tracepoint. */
+	tpoint = find_tracepoint(name, &mod);
+	if (mod && !try_module_get(mod)) {
+		mod = NULL;
+		tpoint = NULL;
+	}
+
+	guard(mutex)(&tracepoint_user_mutex);
+	/* Search existing tracepoint_user */
+	for_each_tracepoint_user(tuser) {
+		if (!strcmp(tuser->name, name)) {
+			tuser->refcount++;
+			*pmod = no_free_ptr(mod);
+			return tuser;
+		}
+	}
+
+	/* The corresponding tracepoint_user is not found. */
+	tuser = __tracepoint_user_init(name, tpoint);
+	if (!IS_ERR_OR_NULL(tuser))
+		*pmod = no_free_ptr(mod);
+
+	return tuser;
 }
 
-static const char *tracepoint_user_lookup(struct tracepoint_user *tuser, char *buf)
+static void tracepoint_user_put(struct tracepoint_user *tuser)
 {
-	struct tracepoint *tpoint = tuser->tpoint;
+	scoped_guard(mutex, &tracepoint_user_mutex) {
+		if (--tuser->refcount > 0)
+			return;
 
-	if (!tpoint)
-		return NULL;
+		list_del(&tuser->list);
+		tracepoint_user_unregister(tuser);
+	}
 
-	return kallsyms_lookup((unsigned long)tpoint->probestub, NULL, NULL, NULL, buf);
+	__tracepoint_user_free(tuser);
 }
 
+DEFINE_FREE(tuser_put, struct tracepoint_user *,
+	if (!IS_ERR_OR_NULL(_T))
+		tracepoint_user_put(_T))
+
 /*
  * Fprobe event core functions
  */
+
+/*
+ * @tprobe is true for tracepoint probe.
+ * @tuser can be NULL if the trace_fprobe is disabled or the tracepoint is not
+ * loaded with a module. If @tuser != NULL, this trace_fprobe is enabled.
+ */
 struct trace_fprobe {
 	struct dyn_event	devent;
 	struct fprobe		fp;
 	const char		*symbol;
+	bool			tprobe;
 	struct tracepoint_user	*tuser;
 	struct trace_probe	tp;
 };
@@ -157,7 +219,7 @@ static bool trace_fprobe_is_return(struct trace_fprobe *tf)
 
 static bool trace_fprobe_is_tracepoint(struct trace_fprobe *tf)
 {
-	return tf->tuser != NULL;
+	return tf->tprobe;
 }
 
 static const char *trace_fprobe_symbol(struct trace_fprobe *tf)
@@ -207,57 +269,6 @@ static bool trace_fprobe_is_registered(struct trace_fprobe *tf)
 	return fprobe_is_registered(&tf->fp);
 }
 
-static struct tracepoint *find_tracepoint(const char *tp_name,
-	struct module **tp_mod);
-
-/*
- * Get tracepoint_user if exist, or allocate new one. If tracepoint is on a
- * module, get its refcounter.
- */
-static struct tracepoint_user *
-trace_fprobe_get_tracepoint_user(const char *name, struct module **pmod)
-{
-	struct tracepoint_user *tuser __free(kfree) = NULL;
-	struct tracepoint *tpoint;
-	struct trace_fprobe *tf;
-	struct dyn_event *dpos;
-	struct module *mod __free(module_put) = NULL;
-	int ret;
-
-	tpoint = find_tracepoint(name, &mod);
-	if (mod && !try_module_get(mod)) {
-		mod = NULL;
-		tpoint = NULL;
-	}
-	/* tpoint can be NULL, but we don't care here. */
-
-	/* Search existing tracepoint_user */
-	for_each_trace_fprobe(tf, dpos) {
-		if (!trace_fprobe_is_tracepoint(tf))
-			continue;
-		if (!strcmp(tf->symbol, name)) {
-			tracepoint_user_get(tf->tuser);
-			*pmod = no_free_ptr(mod);
-			return tf->tuser;
-		}
-	}
-
-	/* Not found, allocate and register new tracepoint_user. */
-	tuser = tracepoint_user_allocate(tpoint);
-	if (!tuser)
-		return NULL;
-
-	if (tpoint) {
-		/* If the tracepoint is not loaded, tpoint can be NULL. */
-		ret = tracepoint_user_register(tuser);
-		if (ret)
-			return ERR_PTR(ret);
-	}
-
-	*pmod = no_free_ptr(mod);
-	return_ptr(tuser);
-}
-
 /*
  * Note that we don't verify the fetch_insn code, since it does not come
  * from user space.
@@ -559,8 +570,8 @@ DEFINE_FREE(free_trace_fprobe, struct trace_fprobe *, if (!IS_ERR_OR_NULL(_T)) f
 static struct trace_fprobe *alloc_trace_fprobe(const char *group,
 					       const char *event,
 					       const char *symbol,
-					       struct tracepoint_user *tuser,
-					       int nargs, bool is_return)
+					       int nargs, bool is_return,
+					       bool is_tracepoint)
 {
 	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
 	int ret = -ENOMEM;
@@ -578,7 +589,7 @@ static struct trace_fprobe *alloc_trace_fprobe(const char *group,
 	else
 		tf->fp.entry_handler = fentry_dispatcher;
 
-	tf->tuser = tuser;
+	tf->tprobe = is_tracepoint;
 
 	ret = trace_probe_init(&tf->tp, event, group, false, nargs);
 	if (ret < 0)
@@ -752,12 +763,35 @@ static int unregister_fprobe_event(struct trace_fprobe *tf)
 
 static int __regsiter_tracepoint_fprobe(struct trace_fprobe *tf)
 {
-	unsigned long ip = tracepoint_user_ip(tf->tuser);
+	struct tracepoint_user *tuser __free(tuser_put) = NULL;
+	struct module *mod __free(module_put) = NULL;
+	unsigned long ip;
+	int ret;
 
-	if (!ip)
-		return -ENOENT;
+	if (WARN_ON_ONCE(tf->tuser))
+		return -EINVAL;
+
+	/* If the tracepoint is in a module, it must be locked in this function. */
+	tuser = tracepoint_user_find_get(tf->symbol, &mod);
+	/* This tracepoint is not loaded yet */
+	if (IS_ERR(tuser))
+		return PTR_ERR(tuser);
+	if (!tuser)
+		return -ENOMEM;
+
+	/* Register fprobe only if the tracepoint is loaded. */
+	if (tuser->tpoint) {
+		ip = tracepoint_user_ip(tuser);
+		if (WARN_ON_ONCE(!ip))
+			return -ENOENT;
 
-	return register_fprobe_ips(&tf->fp, &ip, 1);
+		ret = register_fprobe_ips(&tf->fp, &ip, 1);
+		if (ret < 0)
+			return ret;
+	}
+
+	tf->tuser = no_free_ptr(tuser);
+	return 0;
 }
 
 /* Returns an error if the target function is not available, or 0 */
@@ -766,15 +800,9 @@ static int trace_fprobe_verify_target(struct trace_fprobe *tf)
 	unsigned long *addrs __free(kfree) = NULL;
 	int ret;
 
-	if (trace_fprobe_is_tracepoint(tf)) {
-
-		/* This tracepoint is not loaded yet */
-		if (!tracepoint_user_is_registered(tf->tuser))
-			return 0;
-
-		/* We assume all stab function is tracable. */
-		return tracepoint_user_ip(tf->tuser) ? 0 : -ENOENT;
-	}
+	/* Tracepoint should have a stub function. */
+	if (trace_fprobe_is_tracepoint(tf))
+		return 0;
 
 	ret = fprobe_alloc_ip_list_from_filter(tf->symbol, NULL, &addrs);
 	return (ret < 0) ? ret : 0;
@@ -801,14 +829,8 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
 
 	tf->fp.flags &= ~FPROBE_FL_DISABLED;
 
-	if (trace_fprobe_is_tracepoint(tf)) {
-
-		/* This tracepoint is not loaded yet */
-		if (!tracepoint_user_is_registered(tf->tuser))
-			return 0;
-
+	if (trace_fprobe_is_tracepoint(tf))
 		return __regsiter_tracepoint_fprobe(tf);
-	}
 
 	/* TODO: handle filter, nofilter or symbol list */
 	return register_fprobe(&tf->fp, tf->symbol, NULL);
@@ -817,11 +839,9 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
 /* Internal unregister function - just handle fprobe and flags */
 static void __unregister_trace_fprobe(struct trace_fprobe *tf)
 {
-	if (trace_fprobe_is_registered(tf)) {
+	if (trace_fprobe_is_registered(tf))
 		unregister_fprobe(&tf->fp);
-		memset(&tf->fp, 0, sizeof(tf->fp));
-	}
-	if (trace_fprobe_is_tracepoint(tf)) {
+	if (tf->tuser) {
 		tracepoint_user_put(tf->tuser);
 		tf->tuser = NULL;
 	}
@@ -1012,63 +1032,52 @@ static struct tracepoint *find_tracepoint_in_module(struct module *mod,
 	return data.tpoint;
 }
 
+/* These are CONFIG_MODULES=y specific functions. */
+static bool tracepoint_user_within_module(struct tracepoint_user *tuser,
+					  struct module *mod)
+{
+	return within_module(tracepoint_user_ip(tuser), mod);
+}
+
+static int tracepoint_user_register_again(struct tracepoint_user *tuser,
+					  struct tracepoint *tpoint)
+{
+	tuser->tpoint = tpoint;
+	return tracepoint_user_register(tuser);
+}
+
+static void tracepoint_user_unregister_clear(struct tracepoint_user *tuser)
+{
+	tracepoint_user_unregister(tuser);
+	tuser->tpoint = NULL;
+}
+
+/* module callback for tracepoint_user */
 static int __tracepoint_probe_module_cb(struct notifier_block *self,
 					unsigned long val, void *data)
 {
 	struct tp_module *tp_mod = data;
 	struct tracepoint_user *tuser;
-	struct trace_fprobe *tf;
-	struct dyn_event *pos;
+	struct tracepoint *tpoint;
 
 	if (val != MODULE_STATE_GOING && val != MODULE_STATE_COMING)
 		return NOTIFY_DONE;
 
-	mutex_lock(&event_mutex);
-	for_each_trace_fprobe(tf, pos) {
-		if (!trace_fprobe_is_tracepoint(tf))
-			continue;
-
+	mutex_lock(&tracepoint_user_mutex);
+	for_each_tracepoint_user(tuser) {
 		if (val == MODULE_STATE_COMING) {
-			/*
-			 * If any tracepoint used by tprobe is in the module,
-			 * register the stub.
-			 */
-			struct tracepoint *tpoint;
-
-			tpoint = find_tracepoint_in_module(tp_mod->mod, tf->symbol);
 			/* This is not a tracepoint in this module. Skip it. */
+			tpoint = find_tracepoint_in_module(tp_mod->mod, tuser->name);
 			if (!tpoint)
 				continue;
-
-			tuser = tf->tuser;
-			/* If the tracepoint is no registered yet, register it. */
-			if (!tracepoint_user_is_registered(tuser)) {
-				tuser->tpoint = tpoint;
-				if (WARN_ON_ONCE(tracepoint_user_register(tuser)))
-					continue;
-			}
-
-			/* Finally enable fprobe on this module. */
-			if (trace_probe_is_enabled(&tf->tp) && !trace_fprobe_is_registered(tf))
-				WARN_ON_ONCE(__regsiter_tracepoint_fprobe(tf));
-		} else if (val == MODULE_STATE_GOING) {
-			tuser = tf->tuser;
+			WARN_ON_ONCE(tracepoint_user_register_again(tuser, tpoint));
+		} else if (val == MODULE_STATE_GOING &&
+			  tracepoint_user_within_module(tuser, tp_mod->mod)) {
 			/* Unregister all tracepoint_user in this module. */
-			if (tracepoint_user_is_registered(tuser) &&
-			    tracepoint_user_within_module(tuser, tp_mod->mod))
-				tracepoint_user_unregister(tuser);
-
-			/*
-			 * Here we need to handle shared tracepoint_user case.
-			 * Such tuser is unregistered, but trace_fprobe itself
-			 * is registered. (Note this only handles tprobes.)
-			 */
-			if (!tracepoint_user_is_registered(tuser) &&
-			    trace_fprobe_is_registered(tf))
-				unregister_fprobe(&tf->fp);
+			tracepoint_user_unregister_clear(tuser);
 		}
 	}
-	mutex_unlock(&event_mutex);
+	mutex_unlock(&tracepoint_user_mutex);
 
 	return NOTIFY_DONE;
 }
@@ -1076,6 +1085,54 @@ static int __tracepoint_probe_module_cb(struct notifier_block *self,
 static struct notifier_block tracepoint_module_nb = {
 	.notifier_call = __tracepoint_probe_module_cb,
 };
+
+/* module callback for tprobe events */
+static int __tprobe_event_module_cb(struct notifier_block *self,
+				     unsigned long val, void *data)
+{
+	struct trace_fprobe *tf;
+	struct dyn_event *pos;
+	struct module *mod = data;
+
+	if (val != MODULE_STATE_GOING && val != MODULE_STATE_COMING)
+		return NOTIFY_DONE;
+
+	mutex_lock(&event_mutex);
+	for_each_trace_fprobe(tf, pos) {
+		/* Skip fprobe and disabled tprobe events. */
+		if (!trace_fprobe_is_tracepoint(tf) || !tf->tuser)
+			continue;
+
+		/* Before this notification, tracepoint notifier has already done. */
+		if (val == MODULE_STATE_COMING &&
+		    tracepoint_user_within_module(tf->tuser, mod)) {
+			unsigned long ip = tracepoint_user_ip(tf->tuser);
+
+			WARN_ON_ONCE(register_fprobe_ips(&tf->fp, &ip, 1));
+		} else if (val == MODULE_STATE_GOING &&
+			   /*
+			    * tracepoint_user_within_module() does not work here because
+			    * tracepoint_user is already unregistered and cleared tpoint.
+			    * Instead, checking whether the fprobe is registered but
+			    * tpoint is cleared(unregistered). Such unbalance probes
+			    * must be adjusted anyway.
+			    */
+			    trace_fprobe_is_registered(tf) &&
+			    !tf->tuser->tpoint) {
+			unregister_fprobe(&tf->fp);
+		}
+	}
+	mutex_unlock(&event_mutex);
+
+	return NOTIFY_DONE;
+}
+
+/* NOTE: this must be called after tracepoint callback */
+static struct notifier_block tprobe_event_module_nb = {
+	.notifier_call = __tprobe_event_module_cb,
+	/* Make sure this is later than tracepoint module notifier. */
+	.priority = -10,
+};
 #endif /* CONFIG_MODULES */
 
 static int parse_symbol_and_return(int argc, const char *argv[],
@@ -1134,10 +1191,6 @@ static int parse_symbol_and_return(int argc, const char *argv[],
 	return 0;
 }
 
-DEFINE_FREE(tuser_put, struct tracepoint_user *,
-	if (!IS_ERR_OR_NULL(_T))
-		tracepoint_user_put(_T))
-
 static int trace_fprobe_create_internal(int argc, const char *argv[],
 					struct traceprobe_parse_context *ctx)
 {
@@ -1166,7 +1219,6 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 	 *  FETCHARG:TYPE : use TYPE instead of unsigned long.
 	 */
 	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
-	struct tracepoint_user *tuser __free(tuser_put) = NULL;
 	struct module *mod __free(module_put) = NULL;
 	int i, new_argc = 0, ret = 0;
 	bool is_return = false;
@@ -1229,21 +1281,21 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 	else
 		ctx->flags |= TPARG_FL_FENTRY;
 
+	ctx->funcname = NULL;
 	if (is_tracepoint) {
+		/* Get tracepoint and lock its module until the end of the registration. */
+		struct tracepoint *tpoint = find_tracepoint(symbol, &mod);
+
 		ctx->flags |= TPARG_FL_TPOINT;
-		tuser = trace_fprobe_get_tracepoint_user(symbol, &mod);
-		if (!tuser)
-			return -ENOMEM;
-		if (IS_ERR(tuser)) {
-			trace_probe_log_set_index(1);
-			trace_probe_log_err(0, NO_TRACEPOINT);
-			return PTR_ERR(tuser);
+		if (mod && !try_module_get(mod)) {
+			mod = NULL;
+			tpoint = NULL;
 		}
-		ctx->funcname = tracepoint_user_lookup(tuser, sbuf);
-		/* If tracepoint is not loaded yet, use symbol name as funcname. */
-		if (!ctx->funcname)
-			ctx->funcname = symbol;
-	} else
+		if (tpoint)
+			ctx->funcname = kallsyms_lookup((unsigned long)tpoint->probestub,
+							NULL, NULL, NULL, sbuf);
+	}
+	if (!ctx->funcname)
 		ctx->funcname = symbol;
 
 	argc -= 2; argv += 2;
@@ -1266,14 +1318,13 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
 		return ret;
 
 	/* setup a probe */
-	tf = alloc_trace_fprobe(group, event, symbol, tuser, argc, is_return);
+	tf = alloc_trace_fprobe(group, event, symbol, argc, is_return, is_tracepoint);
 	if (IS_ERR(tf)) {
 		ret = PTR_ERR(tf);
 		/* This must return -ENOMEM, else there is a bug */
 		WARN_ON_ONCE(ret != -ENOMEM);
 		return ret;
 	}
-	tuser = NULL; /* Move tuser to tf. */
 
 	/* parse arguments */
 	for (i = 0; i < argc; i++) {
@@ -1491,6 +1542,9 @@ static __init int init_fprobe_trace_early(void)
 	ret = register_tracepoint_module_notifier(&tracepoint_module_nb);
 	if (ret)
 		return ret;
+	ret = register_module_notifier(&tprobe_event_module_nb);
+	if (ret)
+		return ret;
 #endif
 
 	return 0;

Re: [RFC PATCH] tracing: tprobe-events: Register tracepoint when enable tprobe event

[Masami Hiramatsu]

On Mon, 17 Mar 2025 17:03:16 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> As same as fprobe, register tracepoint stub function only when enabling
> tprobe events. The major changes are introducing a list of
> tracepoint_user and its lock, and tprobe_event_module_nb, which is
> another module notifier for module loading/unloading.  By spliting the
> lock from event_mutex and a module notifier for trace_fprobe, it
> solved AB-BA lock dependency issue between event_mutex and
> tracepoint_module_list_mutex.


Here is the patch to enable tracepoint only when the tprobe event
is enabled which avoids the AB-BA lock issue.

> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_fprobe.c |  382 +++++++++++++++++++++++++------------------
>  1 file changed, 218 insertions(+), 164 deletions(-)

So it is a bit complicated, but maybe acceptable size.
The main idea is to make a list of tracepoint_user for avoiding locking
event_mutex in tracepoint_module_notifier.

Thank you,

> 
> diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> index ac3b0a34810a..2c4352f4dc11 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -7,7 +7,9 @@
>  #include <asm/ptrace.h>
>  
>  #include <linux/fprobe.h>
> +#include <linux/list.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/rculist.h>
>  #include <linux/security.h>
>  #include <linux/tracepoint.h>
> @@ -37,15 +39,21 @@ static struct dyn_event_operations trace_fprobe_ops = {
>  	.match = trace_fprobe_match,
>  };
>  
> +/* List of tracepoint_user */
> +static LIST_HEAD(tracepoint_user_list);
> +static DEFINE_MUTEX(tracepoint_user_mutex);
> +
> +/* While living tracepoint_user, @tpoint can be NULL and @refcount != 0. */
>  struct tracepoint_user {
> +	struct list_head	list;
> +	const char		*name;
>  	struct tracepoint	*tpoint;
>  	unsigned int		refcount;
>  };
>  
> -static bool tracepoint_user_is_registered(struct tracepoint_user *tuser)
> -{
> -	return tuser && tuser->tpoint;
> -}
> +/* NOTE: you must lock tracepoint_user_mutex. */
> +#define for_each_tracepoint_user(tuser)		\
> +	list_for_each_entry(tuser, &tracepoint_user_list, list)
>  
>  static int tracepoint_user_register(struct tracepoint_user *tuser)
>  {
> @@ -75,58 +83,112 @@ static unsigned long tracepoint_user_ip(struct tracepoint_user *tuser)
>  	return (unsigned long)tuser->tpoint->probestub;
>  }
>  
> -static bool tracepoint_user_within_module(struct tracepoint_user *tuser,
> -					  struct module *mod)
> +static void __tracepoint_user_free(struct tracepoint_user *tuser)
>  {
> -	return within_module(tracepoint_user_ip(tuser), mod);
> +	if (!tuser)
> +		return;
> +	kfree(tuser->name);
> +	kfree(tuser);
>  }
>  
> -static struct tracepoint_user *tracepoint_user_allocate(struct tracepoint *tpoint)
> +DEFINE_FREE(tuser_free, struct tracepoint_user *, __tracepoint_user_free(_T))
> +
> +static struct tracepoint_user *__tracepoint_user_init(const char *name, struct tracepoint *tpoint)
>  {
> -	struct tracepoint_user *tuser __free(kfree) = NULL;
> +	struct tracepoint_user *tuser __free(tuser_free) = NULL;
> +	int ret;
>  
>  	tuser = kzalloc(sizeof(*tuser), GFP_KERNEL);
>  	if (!tuser)
>  		return NULL;
> +	tuser->name = kstrdup(name, GFP_KERNEL);
> +	if (!tuser->name)
> +		return NULL;
> +
> +	if (tpoint) {
> +		ret = tracepoint_user_register(tuser);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
>  	tuser->tpoint = tpoint;
>  	tuser->refcount = 1;
> +	INIT_LIST_HEAD(&tuser->list);
> +	list_add(&tuser->list, &tracepoint_user_list);
>  
>  	return_ptr(tuser);
>  }
>  
> -/* These must be called with event_mutex */
> -static void tracepoint_user_get(struct tracepoint_user *tuser)
> -{
> -	tuser->refcount++;
> -}
> +static struct tracepoint *find_tracepoint(const char *tp_name,
> +	struct module **tp_mod);
>  
> -static void tracepoint_user_put(struct tracepoint_user *tuser)
> +/*
> + * Get tracepoint_user if exist, or allocate new one and register it.
> + * If tracepoint is on a module, get its refcounter too.
> + * This returns errno or NULL (not loaded yet) or tracepoint_user.
> + */
> +static struct tracepoint_user *tracepoint_user_find_get(const char *name, struct module **pmod)
>  {
> -	if (--tuser->refcount > 0)
> -		return;
> +	struct module *mod __free(module_put) = NULL;
> +	struct tracepoint_user *tuser;
> +	struct tracepoint *tpoint;
>  
> -	if (tracepoint_user_is_registered(tuser))
> -		tracepoint_user_unregister(tuser);
> -	kfree(tuser);
> +	/* Get and lock the module which has tracepoint. */
> +	tpoint = find_tracepoint(name, &mod);
> +	if (mod && !try_module_get(mod)) {
> +		mod = NULL;
> +		tpoint = NULL;
> +	}
> +
> +	guard(mutex)(&tracepoint_user_mutex);
> +	/* Search existing tracepoint_user */
> +	for_each_tracepoint_user(tuser) {
> +		if (!strcmp(tuser->name, name)) {
> +			tuser->refcount++;
> +			*pmod = no_free_ptr(mod);
> +			return tuser;
> +		}
> +	}
> +
> +	/* The corresponding tracepoint_user is not found. */
> +	tuser = __tracepoint_user_init(name, tpoint);
> +	if (!IS_ERR_OR_NULL(tuser))
> +		*pmod = no_free_ptr(mod);
> +
> +	return tuser;
>  }
>  
> -static const char *tracepoint_user_lookup(struct tracepoint_user *tuser, char *buf)
> +static void tracepoint_user_put(struct tracepoint_user *tuser)
>  {
> -	struct tracepoint *tpoint = tuser->tpoint;
> +	scoped_guard(mutex, &tracepoint_user_mutex) {
> +		if (--tuser->refcount > 0)
> +			return;
>  
> -	if (!tpoint)
> -		return NULL;
> +		list_del(&tuser->list);
> +		tracepoint_user_unregister(tuser);
> +	}
>  
> -	return kallsyms_lookup((unsigned long)tpoint->probestub, NULL, NULL, NULL, buf);
> +	__tracepoint_user_free(tuser);
>  }
>  
> +DEFINE_FREE(tuser_put, struct tracepoint_user *,
> +	if (!IS_ERR_OR_NULL(_T))
> +		tracepoint_user_put(_T))
> +
>  /*
>   * Fprobe event core functions
>   */
> +
> +/*
> + * @tprobe is true for tracepoint probe.
> + * @tuser can be NULL if the trace_fprobe is disabled or the tracepoint is not
> + * loaded with a module. If @tuser != NULL, this trace_fprobe is enabled.
> + */
>  struct trace_fprobe {
>  	struct dyn_event	devent;
>  	struct fprobe		fp;
>  	const char		*symbol;
> +	bool			tprobe;
>  	struct tracepoint_user	*tuser;
>  	struct trace_probe	tp;
>  };
> @@ -157,7 +219,7 @@ static bool trace_fprobe_is_return(struct trace_fprobe *tf)
>  
>  static bool trace_fprobe_is_tracepoint(struct trace_fprobe *tf)
>  {
> -	return tf->tuser != NULL;
> +	return tf->tprobe;
>  }
>  
>  static const char *trace_fprobe_symbol(struct trace_fprobe *tf)
> @@ -207,57 +269,6 @@ static bool trace_fprobe_is_registered(struct trace_fprobe *tf)
>  	return fprobe_is_registered(&tf->fp);
>  }
>  
> -static struct tracepoint *find_tracepoint(const char *tp_name,
> -	struct module **tp_mod);
> -
> -/*
> - * Get tracepoint_user if exist, or allocate new one. If tracepoint is on a
> - * module, get its refcounter.
> - */
> -static struct tracepoint_user *
> -trace_fprobe_get_tracepoint_user(const char *name, struct module **pmod)
> -{
> -	struct tracepoint_user *tuser __free(kfree) = NULL;
> -	struct tracepoint *tpoint;
> -	struct trace_fprobe *tf;
> -	struct dyn_event *dpos;
> -	struct module *mod __free(module_put) = NULL;
> -	int ret;
> -
> -	tpoint = find_tracepoint(name, &mod);
> -	if (mod && !try_module_get(mod)) {
> -		mod = NULL;
> -		tpoint = NULL;
> -	}
> -	/* tpoint can be NULL, but we don't care here. */
> -
> -	/* Search existing tracepoint_user */
> -	for_each_trace_fprobe(tf, dpos) {
> -		if (!trace_fprobe_is_tracepoint(tf))
> -			continue;
> -		if (!strcmp(tf->symbol, name)) {
> -			tracepoint_user_get(tf->tuser);
> -			*pmod = no_free_ptr(mod);
> -			return tf->tuser;
> -		}
> -	}
> -
> -	/* Not found, allocate and register new tracepoint_user. */
> -	tuser = tracepoint_user_allocate(tpoint);
> -	if (!tuser)
> -		return NULL;
> -
> -	if (tpoint) {
> -		/* If the tracepoint is not loaded, tpoint can be NULL. */
> -		ret = tracepoint_user_register(tuser);
> -		if (ret)
> -			return ERR_PTR(ret);
> -	}
> -
> -	*pmod = no_free_ptr(mod);
> -	return_ptr(tuser);
> -}
> -
>  /*
>   * Note that we don't verify the fetch_insn code, since it does not come
>   * from user space.
> @@ -559,8 +570,8 @@ DEFINE_FREE(free_trace_fprobe, struct trace_fprobe *, if (!IS_ERR_OR_NULL(_T)) f
>  static struct trace_fprobe *alloc_trace_fprobe(const char *group,
>  					       const char *event,
>  					       const char *symbol,
> -					       struct tracepoint_user *tuser,
> -					       int nargs, bool is_return)
> +					       int nargs, bool is_return,
> +					       bool is_tracepoint)
>  {
>  	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
>  	int ret = -ENOMEM;
> @@ -578,7 +589,7 @@ static struct trace_fprobe *alloc_trace_fprobe(const char *group,
>  	else
>  		tf->fp.entry_handler = fentry_dispatcher;
>  
> -	tf->tuser = tuser;
> +	tf->tprobe = is_tracepoint;
>  
>  	ret = trace_probe_init(&tf->tp, event, group, false, nargs);
>  	if (ret < 0)
> @@ -752,12 +763,35 @@ static int unregister_fprobe_event(struct trace_fprobe *tf)
>  
>  static int __regsiter_tracepoint_fprobe(struct trace_fprobe *tf)
>  {
> -	unsigned long ip = tracepoint_user_ip(tf->tuser);
> +	struct tracepoint_user *tuser __free(tuser_put) = NULL;
> +	struct module *mod __free(module_put) = NULL;
> +	unsigned long ip;
> +	int ret;
>  
> -	if (!ip)
> -		return -ENOENT;
> +	if (WARN_ON_ONCE(tf->tuser))
> +		return -EINVAL;
> +
> +	/* If the tracepoint is in a module, it must be locked in this function. */
> +	tuser = tracepoint_user_find_get(tf->symbol, &mod);
> +	/* This tracepoint is not loaded yet */
> +	if (IS_ERR(tuser))
> +		return PTR_ERR(tuser);
> +	if (!tuser)
> +		return -ENOMEM;
> +
> +	/* Register fprobe only if the tracepoint is loaded. */
> +	if (tuser->tpoint) {
> +		ip = tracepoint_user_ip(tuser);
> +		if (WARN_ON_ONCE(!ip))
> +			return -ENOENT;
>  
> -	return register_fprobe_ips(&tf->fp, &ip, 1);
> +		ret = register_fprobe_ips(&tf->fp, &ip, 1);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	tf->tuser = no_free_ptr(tuser);
> +	return 0;
>  }
>  
>  /* Returns an error if the target function is not available, or 0 */
> @@ -766,15 +800,9 @@ static int trace_fprobe_verify_target(struct trace_fprobe *tf)
>  	unsigned long *addrs __free(kfree) = NULL;
>  	int ret;
>  
> -	if (trace_fprobe_is_tracepoint(tf)) {
> -
> -		/* This tracepoint is not loaded yet */
> -		if (!tracepoint_user_is_registered(tf->tuser))
> -			return 0;
> -
> -		/* We assume all stab function is tracable. */
> -		return tracepoint_user_ip(tf->tuser) ? 0 : -ENOENT;
> -	}
> +	/* Tracepoint should have a stub function. */
> +	if (trace_fprobe_is_tracepoint(tf))
> +		return 0;
>  
>  	ret = fprobe_alloc_ip_list_from_filter(tf->symbol, NULL, &addrs);
>  	return (ret < 0) ? ret : 0;
> @@ -801,14 +829,8 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
>  
>  	tf->fp.flags &= ~FPROBE_FL_DISABLED;
>  
> -	if (trace_fprobe_is_tracepoint(tf)) {
> -
> -		/* This tracepoint is not loaded yet */
> -		if (!tracepoint_user_is_registered(tf->tuser))
> -			return 0;
> -
> +	if (trace_fprobe_is_tracepoint(tf))
>  		return __regsiter_tracepoint_fprobe(tf);
> -	}
>  
>  	/* TODO: handle filter, nofilter or symbol list */
>  	return register_fprobe(&tf->fp, tf->symbol, NULL);
> @@ -817,11 +839,9 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
>  /* Internal unregister function - just handle fprobe and flags */
>  static void __unregister_trace_fprobe(struct trace_fprobe *tf)
>  {
> -	if (trace_fprobe_is_registered(tf)) {
> +	if (trace_fprobe_is_registered(tf))
>  		unregister_fprobe(&tf->fp);
> -		memset(&tf->fp, 0, sizeof(tf->fp));
> -	}
> -	if (trace_fprobe_is_tracepoint(tf)) {
> +	if (tf->tuser) {
>  		tracepoint_user_put(tf->tuser);
>  		tf->tuser = NULL;
>  	}
> @@ -1012,63 +1032,52 @@ static struct tracepoint *find_tracepoint_in_module(struct module *mod,
>  	return data.tpoint;
>  }
>  
> +/* These are CONFIG_MODULES=y specific functions. */
> +static bool tracepoint_user_within_module(struct tracepoint_user *tuser,
> +					  struct module *mod)
> +{
> +	return within_module(tracepoint_user_ip(tuser), mod);
> +}
> +
> +static int tracepoint_user_register_again(struct tracepoint_user *tuser,
> +					  struct tracepoint *tpoint)
> +{
> +	tuser->tpoint = tpoint;
> +	return tracepoint_user_register(tuser);
> +}
> +
> +static void tracepoint_user_unregister_clear(struct tracepoint_user *tuser)
> +{
> +	tracepoint_user_unregister(tuser);
> +	tuser->tpoint = NULL;
> +}
> +
> +/* module callback for tracepoint_user */
>  static int __tracepoint_probe_module_cb(struct notifier_block *self,
>  					unsigned long val, void *data)
>  {
>  	struct tp_module *tp_mod = data;
>  	struct tracepoint_user *tuser;
> -	struct trace_fprobe *tf;
> -	struct dyn_event *pos;
> +	struct tracepoint *tpoint;
>  
>  	if (val != MODULE_STATE_GOING && val != MODULE_STATE_COMING)
>  		return NOTIFY_DONE;
>  
> -	mutex_lock(&event_mutex);
> -	for_each_trace_fprobe(tf, pos) {
> -		if (!trace_fprobe_is_tracepoint(tf))
> -			continue;
> -
> +	mutex_lock(&tracepoint_user_mutex);
> +	for_each_tracepoint_user(tuser) {
>  		if (val == MODULE_STATE_COMING) {
> -			/*
> -			 * If any tracepoint used by tprobe is in the module,
> -			 * register the stub.
> -			 */
> -			struct tracepoint *tpoint;
> -
> -			tpoint = find_tracepoint_in_module(tp_mod->mod, tf->symbol);
>  			/* This is not a tracepoint in this module. Skip it. */
> +			tpoint = find_tracepoint_in_module(tp_mod->mod, tuser->name);
>  			if (!tpoint)
>  				continue;
> -
> -			tuser = tf->tuser;
> -			/* If the tracepoint is no registered yet, register it. */
> -			if (!tracepoint_user_is_registered(tuser)) {
> -				tuser->tpoint = tpoint;
> -				if (WARN_ON_ONCE(tracepoint_user_register(tuser)))
> -					continue;
> -			}
> -
> -			/* Finally enable fprobe on this module. */
> -			if (trace_probe_is_enabled(&tf->tp) && !trace_fprobe_is_registered(tf))
> -				WARN_ON_ONCE(__regsiter_tracepoint_fprobe(tf));
> -		} else if (val == MODULE_STATE_GOING) {
> -			tuser = tf->tuser;
> +			WARN_ON_ONCE(tracepoint_user_register_again(tuser, tpoint));
> +		} else if (val == MODULE_STATE_GOING &&
> +			  tracepoint_user_within_module(tuser, tp_mod->mod)) {
>  			/* Unregister all tracepoint_user in this module. */
> -			if (tracepoint_user_is_registered(tuser) &&
> -			    tracepoint_user_within_module(tuser, tp_mod->mod))
> -				tracepoint_user_unregister(tuser);
> -
> -			/*
> -			 * Here we need to handle shared tracepoint_user case.
> -			 * Such tuser is unregistered, but trace_fprobe itself
> -			 * is registered. (Note this only handles tprobes.)
> -			 */
> -			if (!tracepoint_user_is_registered(tuser) &&
> -			    trace_fprobe_is_registered(tf))
> -				unregister_fprobe(&tf->fp);
> +			tracepoint_user_unregister_clear(tuser);
>  		}
>  	}
> -	mutex_unlock(&event_mutex);
> +	mutex_unlock(&tracepoint_user_mutex);
>  
>  	return NOTIFY_DONE;
>  }
> @@ -1076,6 +1085,54 @@ static int __tracepoint_probe_module_cb(struct notifier_block *self,
>  static struct notifier_block tracepoint_module_nb = {
>  	.notifier_call = __tracepoint_probe_module_cb,
>  };
> +
> +/* module callback for tprobe events */
> +static int __tprobe_event_module_cb(struct notifier_block *self,
> +				     unsigned long val, void *data)
> +{
> +	struct trace_fprobe *tf;
> +	struct dyn_event *pos;
> +	struct module *mod = data;
> +
> +	if (val != MODULE_STATE_GOING && val != MODULE_STATE_COMING)
> +		return NOTIFY_DONE;
> +
> +	mutex_lock(&event_mutex);
> +	for_each_trace_fprobe(tf, pos) {
> +		/* Skip fprobe and disabled tprobe events. */
> +		if (!trace_fprobe_is_tracepoint(tf) || !tf->tuser)
> +			continue;
> +
> +		/* Before this notification, tracepoint notifier has already done. */
> +		if (val == MODULE_STATE_COMING &&
> +		    tracepoint_user_within_module(tf->tuser, mod)) {
> +			unsigned long ip = tracepoint_user_ip(tf->tuser);
> +
> +			WARN_ON_ONCE(register_fprobe_ips(&tf->fp, &ip, 1));
> +		} else if (val == MODULE_STATE_GOING &&
> +			   /*
> +			    * tracepoint_user_within_module() does not work here because
> +			    * tracepoint_user is already unregistered and cleared tpoint.
> +			    * Instead, checking whether the fprobe is registered but
> +			    * tpoint is cleared(unregistered). Such unbalance probes
> +			    * must be adjusted anyway.
> +			    */
> +			    trace_fprobe_is_registered(tf) &&
> +			    !tf->tuser->tpoint) {
> +			unregister_fprobe(&tf->fp);
> +		}
> +	}
> +	mutex_unlock(&event_mutex);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +/* NOTE: this must be called after tracepoint callback */
> +static struct notifier_block tprobe_event_module_nb = {
> +	.notifier_call = __tprobe_event_module_cb,
> +	/* Make sure this is later than tracepoint module notifier. */
> +	.priority = -10,
> +};
>  #endif /* CONFIG_MODULES */
>  
>  static int parse_symbol_and_return(int argc, const char *argv[],
> @@ -1134,10 +1191,6 @@ static int parse_symbol_and_return(int argc, const char *argv[],
>  	return 0;
>  }
>  
> -DEFINE_FREE(tuser_put, struct tracepoint_user *,
> -	if (!IS_ERR_OR_NULL(_T))
> -		tracepoint_user_put(_T))
> -
>  static int trace_fprobe_create_internal(int argc, const char *argv[],
>  					struct traceprobe_parse_context *ctx)
>  {
> @@ -1166,7 +1219,6 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  	 *  FETCHARG:TYPE : use TYPE instead of unsigned long.
>  	 */
>  	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
> -	struct tracepoint_user *tuser __free(tuser_put) = NULL;
>  	struct module *mod __free(module_put) = NULL;
>  	int i, new_argc = 0, ret = 0;
>  	bool is_return = false;
> @@ -1229,21 +1281,21 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  	else
>  		ctx->flags |= TPARG_FL_FENTRY;
>  
> +	ctx->funcname = NULL;
>  	if (is_tracepoint) {
> +		/* Get tracepoint and lock its module until the end of the registration. */
> +		struct tracepoint *tpoint = find_tracepoint(symbol, &mod);
> +
>  		ctx->flags |= TPARG_FL_TPOINT;
> -		tuser = trace_fprobe_get_tracepoint_user(symbol, &mod);
> -		if (!tuser)
> -			return -ENOMEM;
> -		if (IS_ERR(tuser)) {
> -			trace_probe_log_set_index(1);
> -			trace_probe_log_err(0, NO_TRACEPOINT);
> -			return PTR_ERR(tuser);
> +		if (mod && !try_module_get(mod)) {
> +			mod = NULL;
> +			tpoint = NULL;
>  		}
> -		ctx->funcname = tracepoint_user_lookup(tuser, sbuf);
> -		/* If tracepoint is not loaded yet, use symbol name as funcname. */
> -		if (!ctx->funcname)
> -			ctx->funcname = symbol;
> -	} else
> +		if (tpoint)
> +			ctx->funcname = kallsyms_lookup((unsigned long)tpoint->probestub,
> +							NULL, NULL, NULL, sbuf);
> +	}
> +	if (!ctx->funcname)
>  		ctx->funcname = symbol;
>  
>  	argc -= 2; argv += 2;
> @@ -1266,14 +1318,13 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  		return ret;
>  
>  	/* setup a probe */
> -	tf = alloc_trace_fprobe(group, event, symbol, tuser, argc, is_return);
> +	tf = alloc_trace_fprobe(group, event, symbol, argc, is_return, is_tracepoint);
>  	if (IS_ERR(tf)) {
>  		ret = PTR_ERR(tf);
>  		/* This must return -ENOMEM, else there is a bug */
>  		WARN_ON_ONCE(ret != -ENOMEM);
>  		return ret;
>  	}
> -	tuser = NULL; /* Move tuser to tf. */
>  
>  	/* parse arguments */
>  	for (i = 0; i < argc; i++) {
> @@ -1491,6 +1542,9 @@ static __init int init_fprobe_trace_early(void)
>  	ret = register_tracepoint_module_notifier(&tracepoint_module_nb);
>  	if (ret)
>  		return ret;
> +	ret = register_module_notifier(&tprobe_event_module_nb);
> +	if (ret)
> +		return ret;
>  #endif
>  
>  	return 0;
> 


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

Re: [PATCH 2/4] tracing: tprobe-events: Support multiple tprobes on the same tracepoint

[Steven Rostedt]

On Sun, 16 Mar 2025 21:21:33 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> index 08def94f9ca6..863c9343a939 100644
> --- a/kernel/trace/trace_fprobe.c
> +++ b/kernel/trace/trace_fprobe.c
> @@ -21,7 +21,6 @@
>  #define FPROBE_EVENT_SYSTEM "fprobes"
>  #define TRACEPOINT_EVENT_SYSTEM "tracepoints"
>  #define RETHOOK_MAXACTIVE_MAX 4096
> -#define TRACEPOINT_STUB ERR_PTR(-ENOENT)
>  
>  static int trace_fprobe_create(const char *raw_command);
>  static int trace_fprobe_show(struct seq_file *m, struct dyn_event *ev);
> @@ -38,6 +37,89 @@ static struct dyn_event_operations trace_fprobe_ops = {
>  	.match = trace_fprobe_match,
>  };
>  
> +struct tracepoint_user {
> +	struct tracepoint	*tpoint;
> +	unsigned int		refcount;
> +};
> +
> +static bool tracepoint_user_is_registered(struct tracepoint_user *tuser)
> +{
> +	return tuser && tuser->tpoint;
> +}
> +
> +static int tracepoint_user_register(struct tracepoint_user *tuser)
> +{
> +	struct tracepoint *tpoint = tuser->tpoint;
> +
> +	if (!tpoint)
> +		return 0;
> +
> +	return tracepoint_probe_register_prio_may_exist(tpoint,
> +					tpoint->probestub, NULL, 0);
> +}
> +
> +static void tracepoint_user_unregister(struct tracepoint_user *tuser)
> +{
> +	if (!tuser->tpoint)
> +		return;
> +
> +	WARN_ON_ONCE(tracepoint_probe_unregister(tuser->tpoint, tuser->tpoint->probestub, NULL));
> +	tuser->tpoint = NULL;
> +}
> +
> +static unsigned long tracepoint_user_ip(struct tracepoint_user *tuser)
> +{
> +	if (!tuser->tpoint)
> +		return 0UL;
> +
> +	return (unsigned long)tuser->tpoint->probestub;
> +}
> +
> +static bool tracepoint_user_within_module(struct tracepoint_user *tuser,
> +					  struct module *mod)
> +{
> +	return within_module(tracepoint_user_ip(tuser), mod);
> +}
> +
> +static struct tracepoint_user *tracepoint_user_allocate(struct tracepoint *tpoint)
> +{
> +	struct tracepoint_user *tuser __free(kfree) = NULL;

Do you really want to use __free() here?

> +
> +	tuser = kzalloc(sizeof(*tuser), GFP_KERNEL);
> +	if (!tuser)
> +		return NULL;

This is the only failure case in the function and tuser is NULL.


> +	tuser->tpoint = tpoint;
> +	tuser->refcount = 1;
> +
> +	return_ptr(tuser);

In fact, "kfree()" will never be called in this function, why the __free()?


> +}
> +
> +/* These must be called with event_mutex */
> +static void tracepoint_user_get(struct tracepoint_user *tuser)
> +{
> +	tuser->refcount++;
> +}
> +
> +static void tracepoint_user_put(struct tracepoint_user *tuser)
> +{
> +	if (--tuser->refcount > 0)
> +		return;
> +
> +	if (tracepoint_user_is_registered(tuser))
> +		tracepoint_user_unregister(tuser);
> +	kfree(tuser);
> +}
> +
> +static const char *tracepoint_user_lookup(struct tracepoint_user *tuser, char *buf)
> +{
> +	struct tracepoint *tpoint = tuser->tpoint;
> +
> +	if (!tpoint)
> +		return NULL;
> +
> +	return kallsyms_lookup((unsigned long)tpoint->probestub, NULL, NULL, NULL, buf);
> +}
> +
>  /*
>   * Fprobe event core functions
>   */
> @@ -45,7 +127,7 @@ struct trace_fprobe {
>  	struct dyn_event	devent;
>  	struct fprobe		fp;
>  	const char		*symbol;
> -	struct tracepoint	*tpoint;
> +	struct tracepoint_user	*tuser;
>  	struct trace_probe	tp;
>  };
>  
> @@ -75,7 +157,7 @@ static bool trace_fprobe_is_return(struct trace_fprobe *tf)
>  
>  static bool trace_fprobe_is_tracepoint(struct trace_fprobe *tf)
>  {
> -	return tf->tpoint != NULL;
> +	return tf->tuser != NULL;
>  }
>  
>  static const char *trace_fprobe_symbol(struct trace_fprobe *tf)
> @@ -125,6 +207,57 @@ static bool trace_fprobe_is_registered(struct trace_fprobe *tf)
>  	return fprobe_is_registered(&tf->fp);
>  }
>  
> +static struct tracepoint *find_tracepoint(const char *tp_name,
> +	struct module **tp_mod);
> +
> +/*
> + * Get tracepoint_user if exist, or allocate new one. If tracepoint is on a
> + * module, get its refcounter.
> + */
> +static struct tracepoint_user *
> +trace_fprobe_get_tracepoint_user(const char *name, struct module **pmod)
> +{
> +	struct tracepoint_user *tuser __free(kfree) = NULL;
> +	struct tracepoint *tpoint;
> +	struct trace_fprobe *tf;
> +	struct dyn_event *dpos;
> +	struct module *mod __free(module_put) = NULL;
> +	int ret;
> +
> +	tpoint = find_tracepoint(name, &mod);
> +	if (mod && !try_module_get(mod)) {

I'm curious to what would cause this, and why we would want to continue?

So, find_tracepoint() found a tracepoint with a module attached, but
try_module_get() fails.

> +		mod = NULL;
> +		tpoint = NULL;

What's the purpose of setting these to NULL and continuing?

I'm confused.

> +	}
> +	/* tpoint can be NULL, but we don't care here. */
> +
> +	/* Search existing tracepoint_user */
> +	for_each_trace_fprobe(tf, dpos) {
> +		if (!trace_fprobe_is_tracepoint(tf))
> +			continue;
> +		if (!strcmp(tf->symbol, name)) {

If the try_module_get() failed, can this every be true? What happens if the
tracepoint it found belonged to another module?

> +			tracepoint_user_get(tf->tuser);
> +			*pmod = no_free_ptr(mod);
> +			return tf->tuser;
> +		}
> +	}
> +
> +	/* Not found, allocate and register new tracepoint_user. */
> +	tuser = tracepoint_user_allocate(tpoint);
> +	if (!tuser)
> +		return NULL;
> +
> +	if (tpoint) {
> +		/* If the tracepoint is not loaded, tpoint can be NULL. */
> +		ret = tracepoint_user_register(tuser);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
> +	*pmod = no_free_ptr(mod);
> +	return_ptr(tuser);
> +}
> +
>  /*
>   * Note that we don't verify the fetch_insn code, since it does not come
>   * from user space.
> @@ -410,6 +543,8 @@ static void free_trace_fprobe(struct trace_fprobe *tf)
>  {
>  	if (tf) {
>  		trace_probe_cleanup(&tf->tp);
> +		if (tf->tuser)
> +			tracepoint_user_put(tf->tuser);
>  		kfree(tf->symbol);
>  		kfree(tf);
>  	}
> @@ -424,7 +559,7 @@ DEFINE_FREE(free_trace_fprobe, struct trace_fprobe *, if (!IS_ERR_OR_NULL(_T)) f
>  static struct trace_fprobe *alloc_trace_fprobe(const char *group,
>  					       const char *event,
>  					       const char *symbol,
> -					       struct tracepoint *tpoint,
> +					       struct tracepoint_user *tuser,
>  					       int nargs, bool is_return)
>  {
>  	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
> @@ -443,7 +578,7 @@ static struct trace_fprobe *alloc_trace_fprobe(const char *group,
>  	else
>  		tf->fp.entry_handler = fentry_dispatcher;
>  
> -	tf->tpoint = tpoint;
> +	tf->tuser = tuser;
>  
>  	ret = trace_probe_init(&tf->tp, event, group, false, nargs);
>  	if (ret < 0)
> @@ -709,19 +844,11 @@ static int unregister_fprobe_event(struct trace_fprobe *tf)
>  
>  static int __regsiter_tracepoint_fprobe(struct trace_fprobe *tf)
>  {
> -	struct tracepoint *tpoint = tf->tpoint;
> -	unsigned long ip = (unsigned long)tpoint->probestub;
> -	int ret;
> +	unsigned long ip = tracepoint_user_ip(tf->tuser);
> +
> +	if (!ip)
> +		return -ENOENT;
>  
> -	/*
> -	 * Here, we do 2 steps to enable fprobe on a tracepoint.
> -	 * At first, put __probestub_##TP function on the tracepoint
> -	 * and put a fprobe on the stub function.
> -	 */
> -	ret = tracepoint_probe_register_prio_may_exist(tpoint,
> -				tpoint->probestub, NULL, 0);
> -	if (ret < 0)
> -		return ret;
>  	return register_fprobe_ips(&tf->fp, &ip, 1);
>  }
>  
> @@ -753,7 +880,7 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
>  	if (trace_fprobe_is_tracepoint(tf)) {
>  
>  		/* This tracepoint is not loaded yet */
> -		if (tf->tpoint == TRACEPOINT_STUB)
> +		if (!tracepoint_user_is_registered(tf->tuser))
>  			return 0;
>  
>  		return __regsiter_tracepoint_fprobe(tf);
> @@ -770,9 +897,8 @@ static void __unregister_trace_fprobe(struct trace_fprobe *tf)
>  		unregister_fprobe(&tf->fp);
>  		memset(&tf->fp, 0, sizeof(tf->fp));
>  		if (trace_fprobe_is_tracepoint(tf)) {
> -			tracepoint_probe_unregister(tf->tpoint,
> -					tf->tpoint->probestub, NULL);
> -			tf->tpoint = NULL;
> +			tracepoint_user_put(tf->tuser);
> +			tf->tuser = NULL;
>  		}
>  	}
>  }
> @@ -975,7 +1101,7 @@ static int __tracepoint_probe_module_cb(struct notifier_block *self,
>  					unsigned long val, void *data)
>  {
>  	struct tp_module *tp_mod = data;
> -	struct tracepoint *tpoint;
> +	struct tracepoint_user *tuser;
>  	struct trace_fprobe *tf;
>  	struct dyn_event *pos;
>  
> @@ -984,21 +1110,48 @@ static int __tracepoint_probe_module_cb(struct notifier_block *self,
>  
>  	mutex_lock(&event_mutex);
>  	for_each_trace_fprobe(tf, pos) {
> -		if (val == MODULE_STATE_COMING && tf->tpoint == TRACEPOINT_STUB) {
> +		if (!trace_fprobe_is_tracepoint(tf))
> +			continue;
> +
> +		if (val == MODULE_STATE_COMING) {
> +			/*
> +			 * If any tracepoint used by tprobe is in the module,
> +			 * register the stub.
> +			 */
> +			struct tracepoint *tpoint;
> +
>  			tpoint = find_tracepoint_in_module(tp_mod->mod, tf->symbol);
> -			if (tpoint) {
> -				tf->tpoint = tpoint;
> -				if (!WARN_ON_ONCE(__regsiter_tracepoint_fprobe(tf)) &&
> -				    trace_probe_is_enabled(&tf->tp))
> -					reenable_trace_fprobe(tf);
> -			}
> -		} else if (val == MODULE_STATE_GOING && tp_mod->mod == tf->mod) {
> -			unregister_fprobe(&tf->fp);
> -			if (trace_fprobe_is_tracepoint(tf)) {
> -				tracepoint_probe_unregister(tf->tpoint,
> -					tf->tpoint->probestub, NULL);
> -				tf->tpoint = TRACEPOINT_STUB;
> +			/* This is not a tracepoint in this module. Skip it. */
> +			if (!tpoint)
> +				continue;
> +
> +			tuser = tf->tuser;
> +			/* If the tracepoint is no registered yet, register it. */

					    "is not registered yet"

> +			if (!tracepoint_user_is_registered(tuser)) {
> +				tuser->tpoint = tpoint;
> +				if (WARN_ON_ONCE(tracepoint_user_register(tuser)))
> +					continue;
>  			}
> +
> +			/* Finally enable fprobe on this module. */
> +			if (!WARN_ON_ONCE(__regsiter_tracepoint_fprobe(tf)) &&
> +			    trace_probe_is_enabled(&tf->tp))
> +				reenable_trace_fprobe(tf);
> +		} else if (val == MODULE_STATE_GOING) {
> +			tuser = tf->tuser;
> +			/* Unregister all tracepoint_user in this module. */
> +			if (tracepoint_user_is_registered(tuser) &&
> +			    tracepoint_user_within_module(tuser, tp_mod->mod))
> +				tracepoint_user_unregister(tuser);
> +


> +			/*
> +			 * Here we need to handle shared tracepoint_user case.
> +			 * Such tuser is unregistered, but trace_fprobe itself
> +			 * is registered. (Note this only handles tprobes.)
> +			 */
> +			if (!tracepoint_user_is_registered(tuser) &&
> +			    trace_fprobe_is_registered(tf))
> +				unregister_fprobe(&tf->fp);

This looks to be unregistering every tprobe even if it's not in the module.

The old code had:

		} else if (val == MODULE_STATE_GOING && tp_mod->mod == tf->mod) {

And the new code has:

		} else if (val == MODULE_STATE_GOING) {


Where's the check here to see if tf is used by the module?

-- Steve

>  		}
>  	}
>  	mutex_unlock(&event_mutex);
> @@ -1067,7 +1220,9 @@ static int parse_symbol_and_return(int argc, const char *argv[],
>  	return 0;
>  }
>  
> -DEFINE_FREE(module_put, struct module *, if (_T) module_put(_T))
> +DEFINE_FREE(tuser_put, struct tracepoint_user *,
> +	if (!IS_ERR_OR_NULL(_T))
> +		tracepoint_user_put(_T))
>  
>  static int trace_fprobe_create_internal(int argc, const char *argv[],
>  					struct traceprobe_parse_context *ctx)
> @@ -1097,6 +1252,8 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  	 *  FETCHARG:TYPE : use TYPE instead of unsigned long.
>  	 */
>  	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
> +	struct tracepoint_user *tuser __free(tuser_put) = NULL;
> +	struct module *mod __free(module_put) = NULL;
>  	int i, new_argc = 0, ret = 0;
>  	bool is_return = false;
>  	char *symbol __free(kfree) = NULL;
> @@ -1108,8 +1265,6 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  	char abuf[MAX_BTF_ARGS_LEN];
>  	char *dbuf __free(kfree) = NULL;
>  	bool is_tracepoint = false;
> -	struct module *tp_mod __free(module_put) = NULL;
> -	struct tracepoint *tpoint = NULL;
>  
>  	if ((argv[0][0] != 'f' && argv[0][0] != 't') || argc < 2)
>  		return -ECANCELED;
> @@ -1162,25 +1317,18 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  
>  	if (is_tracepoint) {
>  		ctx->flags |= TPARG_FL_TPOINT;
> -		tpoint = find_tracepoint(symbol, &tp_mod);
> -		/* lock module until register this tprobe. */
> -		if (tp_mod && !try_module_get(tp_mod)) {
> -			tpoint = NULL;
> -			tp_mod = NULL;
> -		}
> -		if (tpoint) {
> -			ctx->funcname = kallsyms_lookup(
> -				(unsigned long)tpoint->probestub,
> -				NULL, NULL, NULL, sbuf);
> -		} else if (IS_ENABLED(CONFIG_MODULES)) {
> -				/* This *may* be loaded afterwards */
> -				tpoint = TRACEPOINT_STUB;
> -				ctx->funcname = symbol;
> -		} else {
> +		tuser = trace_fprobe_get_tracepoint_user(symbol, &mod);
> +		if (!tuser)
> +			return -ENOMEM;
> +		if (IS_ERR(tuser)) {
>  			trace_probe_log_set_index(1);
>  			trace_probe_log_err(0, NO_TRACEPOINT);
> -			return -EINVAL;
> +			return PTR_ERR(tuser);
>  		}
> +		ctx->funcname = tracepoint_user_lookup(tuser, sbuf);
> +		/* If tracepoint is not loaded yet, use symbol name as funcname. */
> +		if (!ctx->funcname)
> +			ctx->funcname = symbol;
>  	} else
>  		ctx->funcname = symbol;
>  
> @@ -1204,13 +1352,14 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
>  		return ret;
>  
>  	/* setup a probe */
> -	tf = alloc_trace_fprobe(group, event, symbol, tpoint, argc, is_return);
> +	tf = alloc_trace_fprobe(group, event, symbol, tuser, argc, is_return);
>  	if (IS_ERR(tf)) {
>  		ret = PTR_ERR(tf);
>  		/* This must return -ENOMEM, else there is a bug */
>  		WARN_ON_ONCE(ret != -ENOMEM);
>  		return ret;
>  	}
> +	tuser = NULL; /* Move tuser to tf. */
>  
>  	/* parse arguments */
>  	for (i = 0; i < argc; i++) {

Re: [PATCH 3/4] tracing: fprobe-events: Register fprobe-events only when it is enabled

[Steven Rostedt]

On Sun, 16 Mar 2025 21:21:42 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Currently fprobe events are registered when it is defined. Thus it will
> give some overhead even if it is disabled. This changes it to register the
> fprobe only when it is enabled.
> 
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  include/linux/fprobe.h      |    8 +
>  kernel/trace/fprobe.c       |   29 +++--
>  kernel/trace/trace_fprobe.c |  234 +++++++++++++++++++++----------------------
>  3 files changed, 140 insertions(+), 131 deletions(-)
> 
> diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
> index 702099f08929..9635a24d5a25 100644
> --- a/include/linux/fprobe.h
> +++ b/include/linux/fprobe.h
> @@ -94,6 +94,8 @@ int register_fprobe_ips(struct fprobe *fp, unsigned long *addrs, int num);
>  int register_fprobe_syms(struct fprobe *fp, const char **syms, int num);
>  int unregister_fprobe(struct fprobe *fp);
>  bool fprobe_is_registered(struct fprobe *fp);
> +int fprobe_alloc_ip_list_from_filter(const char *filter, const char *notfilter,
> +	unsigned long **addrs);
>  #else
>  static inline int register_fprobe(struct fprobe *fp, const char *filter, const char *notfilter)
>  {
> @@ -115,6 +117,12 @@ static inline bool fprobe_is_registered(struct fprobe *fp)
>  {
>  	return false;
>  }
> +static inline int fprobe_alloc_ip_list_from_filter(const char *filter,
> +						   const char *notfilter,
> +						   unsigned long **addrs)
> +{
> +	return -EOPNOTSUPP;
> +}
>  #endif
>  
>  /**
> diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> index 33082c4e8154..05050f1c2239 100644
> --- a/kernel/trace/fprobe.c
> +++ b/kernel/trace/fprobe.c
> @@ -486,6 +486,24 @@ static int ip_list_from_filter(const char *filter, const char *notfilter,
>  	return match.index ?: -ENOENT;
>  }
>  
> +#define FPROBE_IPS_MAX	INT_MAX
> +
> +int fprobe_alloc_ip_list_from_filter(const char *filter, const char *notfilter,
> +				     unsigned long **addrs)
> +{
> +	int ret;
> +
> +	/* Count the number of ips from filter. */
> +	ret = ip_list_from_filter(filter, notfilter, NULL, FPROBE_IPS_MAX);
> +	if (ret < 0)
> +		return ret;
> +
> +	*addrs = kcalloc(ret, sizeof(unsigned long), GFP_KERNEL);
> +	if (!*addrs)
> +		return -ENOMEM;
> +	return ip_list_from_filter(filter, notfilter, *addrs, ret);

This was in the old code, but I'm wondering. Does this code prevent modules
from being loaded and unloaded too?

I'm asking because if we call the first instance of ip_list_from_filter()
and it finds a list of functions from a module, and then that module is
unloaded, the ip_list_from_filter() will return a failure, and *addrs would
be a memory leak.

-- Steve

> +}
> +
>  static void fprobe_fail_cleanup(struct fprobe *fp)
>  {
>  	kfree(fp->hlist_array);
> @@ -528,8 +546,6 @@ static int fprobe_init(struct fprobe *fp, unsigned long *addrs, int num)
>  	return 0;
>  }
>  
> -#define FPROBE_IPS_MAX	INT_MAX
> -
>  /**
>   * register_fprobe() - Register fprobe to ftrace by pattern.
>   * @fp: A fprobe data structure to be registered.
> @@ -549,14 +565,7 @@ int register_fprobe(struct fprobe *fp, const char *filter, const char *notfilter
>  	if (!fp || !filter)
>  		return -EINVAL;
>  
> -	ret = ip_list_from_filter(filter, notfilter, NULL, FPROBE_IPS_MAX);
> -	if (ret < 0)
> -		return ret;
> -
> -	addrs = kcalloc(ret, sizeof(unsigned long), GFP_KERNEL);
> -	if (!addrs)
> -		return -ENOMEM;
> -	ret = ip_list_from_filter(filter, notfilter, addrs, ret);
> +	ret = fprobe_alloc_ip_list_from_filter(filter, notfilter, &addrs);
>  	if (ret > 0)
>  		ret = register_fprobe_ips(fp, addrs, ret);
>  

Re: [PATCH 4/4] selftests: tracing: Enable fprobe events before checking enable_functions

[Steven Rostedt]

On Sun, 16 Mar 2025 21:21:52 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 

-ENOCHANGELOG

This still needs a description.

-- Steve


> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---

Re: [PATCH 4/4] selftests: tracing: Enable fprobe events before checking enable_functions

[Masami Hiramatsu]

On Tue, 25 Mar 2025 14:42:08 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sun, 16 Mar 2025 21:21:52 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> 
> -ENOCHANGELOG
> 
> This still needs a description.

Oops, I forgot to add a description!

Thanks!

> 
> -- Steve
> 
> 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---


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

Re: [PATCH 3/4] tracing: fprobe-events: Register fprobe-events only when it is enabled

[Masami Hiramatsu]

On Tue, 25 Mar 2025 14:41:11 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sun, 16 Mar 2025 21:21:42 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Currently fprobe events are registered when it is defined. Thus it will
> > give some overhead even if it is disabled. This changes it to register the
> > fprobe only when it is enabled.
> > 
> > Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  include/linux/fprobe.h      |    8 +
> >  kernel/trace/fprobe.c       |   29 +++--
> >  kernel/trace/trace_fprobe.c |  234 +++++++++++++++++++++----------------------
> >  3 files changed, 140 insertions(+), 131 deletions(-)
> > 
> > diff --git a/include/linux/fprobe.h b/include/linux/fprobe.h
> > index 702099f08929..9635a24d5a25 100644
> > --- a/include/linux/fprobe.h
> > +++ b/include/linux/fprobe.h
> > @@ -94,6 +94,8 @@ int register_fprobe_ips(struct fprobe *fp, unsigned long *addrs, int num);
> >  int register_fprobe_syms(struct fprobe *fp, const char **syms, int num);
> >  int unregister_fprobe(struct fprobe *fp);
> >  bool fprobe_is_registered(struct fprobe *fp);
> > +int fprobe_alloc_ip_list_from_filter(const char *filter, const char *notfilter,
> > +	unsigned long **addrs);
> >  #else
> >  static inline int register_fprobe(struct fprobe *fp, const char *filter, const char *notfilter)
> >  {
> > @@ -115,6 +117,12 @@ static inline bool fprobe_is_registered(struct fprobe *fp)
> >  {
> >  	return false;
> >  }
> > +static inline int fprobe_alloc_ip_list_from_filter(const char *filter,
> > +						   const char *notfilter,
> > +						   unsigned long **addrs)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> >  #endif
> >  
> >  /**
> > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> > index 33082c4e8154..05050f1c2239 100644
> > --- a/kernel/trace/fprobe.c
> > +++ b/kernel/trace/fprobe.c
> > @@ -486,6 +486,24 @@ static int ip_list_from_filter(const char *filter, const char *notfilter,
> >  	return match.index ?: -ENOENT;
> >  }
> >  
> > +#define FPROBE_IPS_MAX	INT_MAX
> > +
> > +int fprobe_alloc_ip_list_from_filter(const char *filter, const char *notfilter,
> > +				     unsigned long **addrs)
> > +{
> > +	int ret;
> > +
> > +	/* Count the number of ips from filter. */
> > +	ret = ip_list_from_filter(filter, notfilter, NULL, FPROBE_IPS_MAX);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*addrs = kcalloc(ret, sizeof(unsigned long), GFP_KERNEL);
> > +	if (!*addrs)
> > +		return -ENOMEM;
> > +	return ip_list_from_filter(filter, notfilter, *addrs, ret);
> 
> This was in the old code, but I'm wondering. Does this code prevent modules
> from being loaded and unloaded too?

Ah, no. In that case we should do module_get() for each module
found in module_kallsyms_on_each_symbol(), hmm.

> 
> I'm asking because if we call the first instance of ip_list_from_filter()
> and it finds a list of functions from a module, and then that module is
> unloaded, the ip_list_from_filter() will return a failure, and *addrs would
> be a memory leak.

Good catch! Let me fix it.

Thanks,

> 
> -- Steve
> 
> > +}
> > +
> >  static void fprobe_fail_cleanup(struct fprobe *fp)
> >  {
> >  	kfree(fp->hlist_array);
> > @@ -528,8 +546,6 @@ static int fprobe_init(struct fprobe *fp, unsigned long *addrs, int num)
> >  	return 0;
> >  }
> >  
> > -#define FPROBE_IPS_MAX	INT_MAX
> > -
> >  /**
> >   * register_fprobe() - Register fprobe to ftrace by pattern.
> >   * @fp: A fprobe data structure to be registered.
> > @@ -549,14 +565,7 @@ int register_fprobe(struct fprobe *fp, const char *filter, const char *notfilter
> >  	if (!fp || !filter)
> >  		return -EINVAL;
> >  
> > -	ret = ip_list_from_filter(filter, notfilter, NULL, FPROBE_IPS_MAX);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	addrs = kcalloc(ret, sizeof(unsigned long), GFP_KERNEL);
> > -	if (!addrs)
> > -		return -ENOMEM;
> > -	ret = ip_list_from_filter(filter, notfilter, addrs, ret);
> > +	ret = fprobe_alloc_ip_list_from_filter(filter, notfilter, &addrs);
> >  	if (ret > 0)
> >  		ret = register_fprobe_ips(fp, addrs, ret);
> >  


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

Re: [PATCH 2/4] tracing: tprobe-events: Support multiple tprobes on the same tracepoint

[Masami Hiramatsu]

On Tue, 25 Mar 2025 13:06:28 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sun, 16 Mar 2025 21:21:33 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
> > index 08def94f9ca6..863c9343a939 100644
> > --- a/kernel/trace/trace_fprobe.c
> > +++ b/kernel/trace/trace_fprobe.c
> > @@ -21,7 +21,6 @@
> >  #define FPROBE_EVENT_SYSTEM "fprobes"
> >  #define TRACEPOINT_EVENT_SYSTEM "tracepoints"
> >  #define RETHOOK_MAXACTIVE_MAX 4096
> > -#define TRACEPOINT_STUB ERR_PTR(-ENOENT)
> >  
> >  static int trace_fprobe_create(const char *raw_command);
> >  static int trace_fprobe_show(struct seq_file *m, struct dyn_event *ev);
> > @@ -38,6 +37,89 @@ static struct dyn_event_operations trace_fprobe_ops = {
> >  	.match = trace_fprobe_match,
> >  };
> >  
> > +struct tracepoint_user {
> > +	struct tracepoint	*tpoint;
> > +	unsigned int		refcount;
> > +};
> > +
> > +static bool tracepoint_user_is_registered(struct tracepoint_user *tuser)
> > +{
> > +	return tuser && tuser->tpoint;
> > +}
> > +
> > +static int tracepoint_user_register(struct tracepoint_user *tuser)
> > +{
> > +	struct tracepoint *tpoint = tuser->tpoint;
> > +
> > +	if (!tpoint)
> > +		return 0;
> > +
> > +	return tracepoint_probe_register_prio_may_exist(tpoint,
> > +					tpoint->probestub, NULL, 0);
> > +}
> > +
> > +static void tracepoint_user_unregister(struct tracepoint_user *tuser)
> > +{
> > +	if (!tuser->tpoint)
> > +		return;
> > +
> > +	WARN_ON_ONCE(tracepoint_probe_unregister(tuser->tpoint, tuser->tpoint->probestub, NULL));
> > +	tuser->tpoint = NULL;
> > +}
> > +
> > +static unsigned long tracepoint_user_ip(struct tracepoint_user *tuser)
> > +{
> > +	if (!tuser->tpoint)
> > +		return 0UL;
> > +
> > +	return (unsigned long)tuser->tpoint->probestub;
> > +}
> > +
> > +static bool tracepoint_user_within_module(struct tracepoint_user *tuser,
> > +					  struct module *mod)
> > +{
> > +	return within_module(tracepoint_user_ip(tuser), mod);
> > +}
> > +
> > +static struct tracepoint_user *tracepoint_user_allocate(struct tracepoint *tpoint)
> > +{
> > +	struct tracepoint_user *tuser __free(kfree) = NULL;
> 
> Do you really want to use __free() here?
> 
> > +
> > +	tuser = kzalloc(sizeof(*tuser), GFP_KERNEL);
> > +	if (!tuser)
> > +		return NULL;
> 
> This is the only failure case in the function and tuser is NULL.
> 
> 
> > +	tuser->tpoint = tpoint;
> > +	tuser->refcount = 1;
> > +
> > +	return_ptr(tuser);
> 
> In fact, "kfree()" will never be called in this function, why the __free()?

Oh, that's right! Thanks for pointing it out!

> 
> 
> > +}
> > +
> > +/* These must be called with event_mutex */
> > +static void tracepoint_user_get(struct tracepoint_user *tuser)
> > +{
> > +	tuser->refcount++;
> > +}
> > +
> > +static void tracepoint_user_put(struct tracepoint_user *tuser)
> > +{
> > +	if (--tuser->refcount > 0)
> > +		return;
> > +
> > +	if (tracepoint_user_is_registered(tuser))
> > +		tracepoint_user_unregister(tuser);
> > +	kfree(tuser);
> > +}
> > +
> > +static const char *tracepoint_user_lookup(struct tracepoint_user *tuser, char *buf)
> > +{
> > +	struct tracepoint *tpoint = tuser->tpoint;
> > +
> > +	if (!tpoint)
> > +		return NULL;
> > +
> > +	return kallsyms_lookup((unsigned long)tpoint->probestub, NULL, NULL, NULL, buf);
> > +}
> > +
> >  /*
> >   * Fprobe event core functions
> >   */
> > @@ -45,7 +127,7 @@ struct trace_fprobe {
> >  	struct dyn_event	devent;
> >  	struct fprobe		fp;
> >  	const char		*symbol;
> > -	struct tracepoint	*tpoint;
> > +	struct tracepoint_user	*tuser;
> >  	struct trace_probe	tp;
> >  };
> >  
> > @@ -75,7 +157,7 @@ static bool trace_fprobe_is_return(struct trace_fprobe *tf)
> >  
> >  static bool trace_fprobe_is_tracepoint(struct trace_fprobe *tf)
> >  {
> > -	return tf->tpoint != NULL;
> > +	return tf->tuser != NULL;
> >  }
> >  
> >  static const char *trace_fprobe_symbol(struct trace_fprobe *tf)
> > @@ -125,6 +207,57 @@ static bool trace_fprobe_is_registered(struct trace_fprobe *tf)
> >  	return fprobe_is_registered(&tf->fp);
> >  }
> >  
> > +static struct tracepoint *find_tracepoint(const char *tp_name,
> > +	struct module **tp_mod);
> > +
> > +/*
> > + * Get tracepoint_user if exist, or allocate new one. If tracepoint is on a
> > + * module, get its refcounter.
> > + */
> > +static struct tracepoint_user *
> > +trace_fprobe_get_tracepoint_user(const char *name, struct module **pmod)
> > +{
> > +	struct tracepoint_user *tuser __free(kfree) = NULL;
> > +	struct tracepoint *tpoint;
> > +	struct trace_fprobe *tf;
> > +	struct dyn_event *dpos;
> > +	struct module *mod __free(module_put) = NULL;
> > +	int ret;
> > +
> > +	tpoint = find_tracepoint(name, &mod);
> > +	if (mod && !try_module_get(mod)) {
> 
> I'm curious to what would cause this, and why we would want to continue?
> 
> So, find_tracepoint() found a tracepoint with a module attached, but
> try_module_get() fails.
> 
> > +		mod = NULL;
> > +		tpoint = NULL;
> 
> What's the purpose of setting these to NULL and continuing?

If we can not get the module, that means the module will be freed.
Ah, do you mean it can cause UAF?

> 
> I'm confused.
> 
> > +	}
> > +	/* tpoint can be NULL, but we don't care here. */
> > +
> > +	/* Search existing tracepoint_user */
> > +	for_each_trace_fprobe(tf, dpos) {
> > +		if (!trace_fprobe_is_tracepoint(tf))
> > +			continue;
> > +		if (!strcmp(tf->symbol, name)) {
> 
> If the try_module_get() failed, can this every be true?

Ah, that becomes true in the next patch. In this patch, it should
not be true.

> What happens if the
> tracepoint it found belonged to another module?

BTW I think same name tracepoint can exist at the same time,
isn't it correct?

> 
> > +			tracepoint_user_get(tf->tuser);
> > +			*pmod = no_free_ptr(mod);
> > +			return tf->tuser;
> > +		}
> > +	}
> > +
> > +	/* Not found, allocate and register new tracepoint_user. */
> > +	tuser = tracepoint_user_allocate(tpoint);
> > +	if (!tuser)
> > +		return NULL;
> > +
> > +	if (tpoint) {
> > +		/* If the tracepoint is not loaded, tpoint can be NULL. */
> > +		ret = tracepoint_user_register(tuser);
> > +		if (ret)
> > +			return ERR_PTR(ret);
> > +	}
> > +
> > +	*pmod = no_free_ptr(mod);
> > +	return_ptr(tuser);
> > +}
> > +
> >  /*
> >   * Note that we don't verify the fetch_insn code, since it does not come
> >   * from user space.
> > @@ -410,6 +543,8 @@ static void free_trace_fprobe(struct trace_fprobe *tf)
> >  {
> >  	if (tf) {
> >  		trace_probe_cleanup(&tf->tp);
> > +		if (tf->tuser)
> > +			tracepoint_user_put(tf->tuser);
> >  		kfree(tf->symbol);
> >  		kfree(tf);
> >  	}
> > @@ -424,7 +559,7 @@ DEFINE_FREE(free_trace_fprobe, struct trace_fprobe *, if (!IS_ERR_OR_NULL(_T)) f
> >  static struct trace_fprobe *alloc_trace_fprobe(const char *group,
> >  					       const char *event,
> >  					       const char *symbol,
> > -					       struct tracepoint *tpoint,
> > +					       struct tracepoint_user *tuser,
> >  					       int nargs, bool is_return)
> >  {
> >  	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
> > @@ -443,7 +578,7 @@ static struct trace_fprobe *alloc_trace_fprobe(const char *group,
> >  	else
> >  		tf->fp.entry_handler = fentry_dispatcher;
> >  
> > -	tf->tpoint = tpoint;
> > +	tf->tuser = tuser;
> >  
> >  	ret = trace_probe_init(&tf->tp, event, group, false, nargs);
> >  	if (ret < 0)
> > @@ -709,19 +844,11 @@ static int unregister_fprobe_event(struct trace_fprobe *tf)
> >  
> >  static int __regsiter_tracepoint_fprobe(struct trace_fprobe *tf)
> >  {
> > -	struct tracepoint *tpoint = tf->tpoint;
> > -	unsigned long ip = (unsigned long)tpoint->probestub;
> > -	int ret;
> > +	unsigned long ip = tracepoint_user_ip(tf->tuser);
> > +
> > +	if (!ip)
> > +		return -ENOENT;
> >  
> > -	/*
> > -	 * Here, we do 2 steps to enable fprobe on a tracepoint.
> > -	 * At first, put __probestub_##TP function on the tracepoint
> > -	 * and put a fprobe on the stub function.
> > -	 */
> > -	ret = tracepoint_probe_register_prio_may_exist(tpoint,
> > -				tpoint->probestub, NULL, 0);
> > -	if (ret < 0)
> > -		return ret;
> >  	return register_fprobe_ips(&tf->fp, &ip, 1);
> >  }
> >  
> > @@ -753,7 +880,7 @@ static int __register_trace_fprobe(struct trace_fprobe *tf)
> >  	if (trace_fprobe_is_tracepoint(tf)) {
> >  
> >  		/* This tracepoint is not loaded yet */
> > -		if (tf->tpoint == TRACEPOINT_STUB)
> > +		if (!tracepoint_user_is_registered(tf->tuser))
> >  			return 0;
> >  
> >  		return __regsiter_tracepoint_fprobe(tf);
> > @@ -770,9 +897,8 @@ static void __unregister_trace_fprobe(struct trace_fprobe *tf)
> >  		unregister_fprobe(&tf->fp);
> >  		memset(&tf->fp, 0, sizeof(tf->fp));
> >  		if (trace_fprobe_is_tracepoint(tf)) {
> > -			tracepoint_probe_unregister(tf->tpoint,
> > -					tf->tpoint->probestub, NULL);
> > -			tf->tpoint = NULL;
> > +			tracepoint_user_put(tf->tuser);
> > +			tf->tuser = NULL;
> >  		}
> >  	}
> >  }
> > @@ -975,7 +1101,7 @@ static int __tracepoint_probe_module_cb(struct notifier_block *self,
> >  					unsigned long val, void *data)
> >  {
> >  	struct tp_module *tp_mod = data;
> > -	struct tracepoint *tpoint;
> > +	struct tracepoint_user *tuser;
> >  	struct trace_fprobe *tf;
> >  	struct dyn_event *pos;
> >  
> > @@ -984,21 +1110,48 @@ static int __tracepoint_probe_module_cb(struct notifier_block *self,
> >  
> >  	mutex_lock(&event_mutex);
> >  	for_each_trace_fprobe(tf, pos) {
> > -		if (val == MODULE_STATE_COMING && tf->tpoint == TRACEPOINT_STUB) {
> > +		if (!trace_fprobe_is_tracepoint(tf))
> > +			continue;
> > +
> > +		if (val == MODULE_STATE_COMING) {
> > +			/*
> > +			 * If any tracepoint used by tprobe is in the module,
> > +			 * register the stub.
> > +			 */
> > +			struct tracepoint *tpoint;
> > +
> >  			tpoint = find_tracepoint_in_module(tp_mod->mod, tf->symbol);
> > -			if (tpoint) {
> > -				tf->tpoint = tpoint;
> > -				if (!WARN_ON_ONCE(__regsiter_tracepoint_fprobe(tf)) &&
> > -				    trace_probe_is_enabled(&tf->tp))
> > -					reenable_trace_fprobe(tf);
> > -			}
> > -		} else if (val == MODULE_STATE_GOING && tp_mod->mod == tf->mod) {
> > -			unregister_fprobe(&tf->fp);
> > -			if (trace_fprobe_is_tracepoint(tf)) {
> > -				tracepoint_probe_unregister(tf->tpoint,
> > -					tf->tpoint->probestub, NULL);
> > -				tf->tpoint = TRACEPOINT_STUB;
> > +			/* This is not a tracepoint in this module. Skip it. */
> > +			if (!tpoint)
> > +				continue;
> > +
> > +			tuser = tf->tuser;
> > +			/* If the tracepoint is no registered yet, register it. */
> 
> 					    "is not registered yet"

OK,

> 
> > +			if (!tracepoint_user_is_registered(tuser)) {
> > +				tuser->tpoint = tpoint;
> > +				if (WARN_ON_ONCE(tracepoint_user_register(tuser)))
> > +					continue;
> >  			}
> > +
> > +			/* Finally enable fprobe on this module. */
> > +			if (!WARN_ON_ONCE(__regsiter_tracepoint_fprobe(tf)) &&
> > +			    trace_probe_is_enabled(&tf->tp))
> > +				reenable_trace_fprobe(tf);
> > +		} else if (val == MODULE_STATE_GOING) {
> > +			tuser = tf->tuser;
> > +			/* Unregister all tracepoint_user in this module. */
> > +			if (tracepoint_user_is_registered(tuser) &&
> > +			    tracepoint_user_within_module(tuser, tp_mod->mod))
> > +				tracepoint_user_unregister(tuser);
> > +
> 
> 
> > +			/*
> > +			 * Here we need to handle shared tracepoint_user case.
> > +			 * Such tuser is unregistered, but trace_fprobe itself
> > +			 * is registered. (Note this only handles tprobes.)
> > +			 */
> > +			if (!tracepoint_user_is_registered(tuser) &&
> > +			    trace_fprobe_is_registered(tf))
> > +				unregister_fprobe(&tf->fp);
> 
> This looks to be unregistering every tprobe even if it's not in the module.
> 
> The old code had:
> 
> 		} else if (val == MODULE_STATE_GOING && tp_mod->mod == tf->mod) {
> 
> And the new code has:
> 
> 		} else if (val == MODULE_STATE_GOING) {
> 
> 
> Where's the check here to see if tf is used by the module?

At this moment, tprobe is always be registered unless its module is
unloaeded. So `!tracepoint_user_is_registered(tuser)` must not be true
for tprobes in other modules. It's a bit tricky.

Thank you,

> 
> -- Steve
> 
> >  		}
> >  	}
> >  	mutex_unlock(&event_mutex);
> > @@ -1067,7 +1220,9 @@ static int parse_symbol_and_return(int argc, const char *argv[],
> >  	return 0;
> >  }
> >  
> > -DEFINE_FREE(module_put, struct module *, if (_T) module_put(_T))
> > +DEFINE_FREE(tuser_put, struct tracepoint_user *,
> > +	if (!IS_ERR_OR_NULL(_T))
> > +		tracepoint_user_put(_T))
> >  
> >  static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  					struct traceprobe_parse_context *ctx)
> > @@ -1097,6 +1252,8 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  	 *  FETCHARG:TYPE : use TYPE instead of unsigned long.
> >  	 */
> >  	struct trace_fprobe *tf __free(free_trace_fprobe) = NULL;
> > +	struct tracepoint_user *tuser __free(tuser_put) = NULL;
> > +	struct module *mod __free(module_put) = NULL;
> >  	int i, new_argc = 0, ret = 0;
> >  	bool is_return = false;
> >  	char *symbol __free(kfree) = NULL;
> > @@ -1108,8 +1265,6 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  	char abuf[MAX_BTF_ARGS_LEN];
> >  	char *dbuf __free(kfree) = NULL;
> >  	bool is_tracepoint = false;
> > -	struct module *tp_mod __free(module_put) = NULL;
> > -	struct tracepoint *tpoint = NULL;
> >  
> >  	if ((argv[0][0] != 'f' && argv[0][0] != 't') || argc < 2)
> >  		return -ECANCELED;
> > @@ -1162,25 +1317,18 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  
> >  	if (is_tracepoint) {
> >  		ctx->flags |= TPARG_FL_TPOINT;
> > -		tpoint = find_tracepoint(symbol, &tp_mod);
> > -		/* lock module until register this tprobe. */
> > -		if (tp_mod && !try_module_get(tp_mod)) {
> > -			tpoint = NULL;
> > -			tp_mod = NULL;
> > -		}
> > -		if (tpoint) {
> > -			ctx->funcname = kallsyms_lookup(
> > -				(unsigned long)tpoint->probestub,
> > -				NULL, NULL, NULL, sbuf);
> > -		} else if (IS_ENABLED(CONFIG_MODULES)) {
> > -				/* This *may* be loaded afterwards */
> > -				tpoint = TRACEPOINT_STUB;
> > -				ctx->funcname = symbol;
> > -		} else {
> > +		tuser = trace_fprobe_get_tracepoint_user(symbol, &mod);
> > +		if (!tuser)
> > +			return -ENOMEM;
> > +		if (IS_ERR(tuser)) {
> >  			trace_probe_log_set_index(1);
> >  			trace_probe_log_err(0, NO_TRACEPOINT);
> > -			return -EINVAL;
> > +			return PTR_ERR(tuser);
> >  		}
> > +		ctx->funcname = tracepoint_user_lookup(tuser, sbuf);
> > +		/* If tracepoint is not loaded yet, use symbol name as funcname. */
> > +		if (!ctx->funcname)
> > +			ctx->funcname = symbol;
> >  	} else
> >  		ctx->funcname = symbol;
> >  
> > @@ -1204,13 +1352,14 @@ static int trace_fprobe_create_internal(int argc, const char *argv[],
> >  		return ret;
> >  
> >  	/* setup a probe */
> > -	tf = alloc_trace_fprobe(group, event, symbol, tpoint, argc, is_return);
> > +	tf = alloc_trace_fprobe(group, event, symbol, tuser, argc, is_return);
> >  	if (IS_ERR(tf)) {
> >  		ret = PTR_ERR(tf);
> >  		/* This must return -ENOMEM, else there is a bug */
> >  		WARN_ON_ONCE(ret != -ENOMEM);
> >  		return ret;
> >  	}
> > +	tuser = NULL; /* Move tuser to tf. */
> >  
> >  	/* parse arguments */
> >  	for (i = 0; i < argc; i++) {
> 


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

Re: [PATCH 2/4] tracing: tprobe-events: Support multiple tprobes on the same tracepoint

[Masami Hiramatsu]

On Wed, 26 Mar 2025 07:08:09 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > > +	/* tpoint can be NULL, but we don't care here. */
> > > +
> > > +	/* Search existing tracepoint_user */
> > > +	for_each_trace_fprobe(tf, dpos) {
> > > +		if (!trace_fprobe_is_tracepoint(tf))
> > > +			continue;
> > > +		if (!strcmp(tf->symbol, name)) {
> > 
> > If the try_module_get() failed, can this every be true?
> 
> Ah, that becomes true in the next patch. In this patch, it should
> not be true.

No, I was wrong. Even if the try_module_get() failed, tracepoint_user
is allocated anyway (with tpoint = NULL). Thus this can be true.

Thank you,

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

Re: [RFC PATCH] tracing: tprobe-events: Register tracepoint when enable tprobe event

[Steven Rostedt]

On Mon, 17 Mar 2025 17:03:16 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> As same as fprobe, register tracepoint stub function only when enabling
> tprobe events. The major changes are introducing a list of
> tracepoint_user and its lock, and tprobe_event_module_nb, which is
> another module notifier for module loading/unloading.  By spliting the
> lock from event_mutex and a module notifier for trace_fprobe, it
> solved AB-BA lock dependency issue between event_mutex and
> tracepoint_module_list_mutex.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_fprobe.c |  382 +++++++++++++++++++++++++------------------
>  1 file changed, 218 insertions(+), 164 deletions(-)

Is this patch still needed? It doesn't apply cleanly.

-- Steve

Re: [RFC PATCH] tracing: tprobe-events: Register tracepoint when enable tprobe event

[Masami Hiramatsu]

On Thu, 1 May 2025 11:27:12 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 17 Mar 2025 17:03:16 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > As same as fprobe, register tracepoint stub function only when enabling
> > tprobe events. The major changes are introducing a list of
> > tracepoint_user and its lock, and tprobe_event_module_nb, which is
> > another module notifier for module loading/unloading.  By spliting the
> > lock from event_mutex and a module notifier for trace_fprobe, it
> > solved AB-BA lock dependency issue between event_mutex and
> > tracepoint_module_list_mutex.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  kernel/trace/trace_fprobe.c |  382 +++++++++++++++++++++++++------------------
> >  1 file changed, 218 insertions(+), 164 deletions(-)
> 
> Is this patch still needed? It doesn't apply cleanly.

This is a kind of performance optimization. Without this patch,
tprobe always registers a stub function to the tracepoint. This
*may* introduce an overhead, and it depends on how frequently the
tracepoint is used. But I guess it is not noticable unless we
create so many tprobes because the stub function is just a 'ret'.

Thus this is not so hurry. Anyway, I will update it for for-next
because this can clean up __tracepoint_probe_module_cb() logic too.

Thank you,

> 
> -- Steve


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