On Mon, 3 Jun 2024 15:50:55 -0400
Mathieu Desnoyers [off-list ref] wrote:

On 2024-06-01 04:22, Masami Hiramatsu (Google) wrote:

quoted

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

Support raw tracepoint event on module by fprobe events.
Since it only uses for_each_kernel_tracepoint() to find a tracepoint,
the tracepoints on modules are not handled. Thus if user specified a
tracepoint on a module, it shows an error.
This adds new for_each_module_tracepoint() API to tracepoint subsystem,
and uses it to find tracepoints on modules.

Hi Masami,

Why prevent module unload when a fprobe tracepoint is attached to a
module ? This changes the kernel's behavior significantly just for the
sake of instrumentation.

I don't prevent module unloading all the time, just before registering
tracepoint handler (something like booking a ticket :-) ).
See the last hunk of this patch, it puts the module before exiting
__trace_fprobe_create().

As an alternative, LTTng-modules attach/detach to/from modules with the
coming/going notifiers, so the instrumentation gets removed when a
module is unloaded rather than preventing its unload by holding a module
reference count. I would recommend a similar approach for fprobe.

Yes, since tracepoint subsystem provides a notifier API to notify the
tracepoint is gone, fprobe already uses it to find unloading and
unregister the target function. (see __tracepoint_probe_module_cb)

Thank you!

Thanks,

Mathieu

quoted

Reported-by: don <redacted>
Closes: https://lore.kernel.org/all/20240530215718.aeec973a1d0bf058d39cb1e3@kernel.org/ (local)
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
  Changes in v2:
   - Fix build errors with CONFIG_MODULES=y.
---
  kernel/trace/trace_fprobe.c |   46 ++++++++++++++++++++++++++++++++++++-------
  1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c
index 62e6a8f4aae9..1d8a983e1edc 100644
--- a/kernel/trace/trace_fprobe.c
+++ b/kernel/trace/trace_fprobe.c

@@ -385,6 +385,7 @@ static struct trace_fprobe *alloc_trace_fprobe(const char *group,
  					       const char *event,
  					       const char *symbol,
  					       struct tracepoint *tpoint,
+					       struct module *mod,
  					       int maxactive,
  					       int nargs, bool is_return)
  {

@@ -405,6 +406,7 @@ static struct trace_fprobe *alloc_trace_fprobe(const char *group,
  		tf->fp.entry_handler = fentry_dispatcher;
  
  	tf->tpoint = tpoint;
+	tf->mod = mod;
  	tf->fp.nr_maxactive = maxactive;
  
  	ret = trace_probe_init(&tf->tp, event, group, false, nargs);

@@ -895,8 +897,23 @@ static struct notifier_block tracepoint_module_nb = {
  struct __find_tracepoint_cb_data {
  	const char *tp_name;
  	struct tracepoint *tpoint;
+	struct module *mod;
  };
  
+static void __find_tracepoint_module_cb(struct tracepoint *tp, void *priv)
+{
+	struct __find_tracepoint_cb_data *data = priv;
+
+	if (!data->tpoint && !strcmp(data->tp_name, tp->name)) {
+		data->tpoint = tp;
+		data->mod = __module_text_address((unsigned long)tp->probestub);
+		if (!try_module_get(data->mod)) {
+			data->tpoint = NULL;
+			data->mod = NULL;
+		}
+	}
+}
+
  static void __find_tracepoint_cb(struct tracepoint *tp, void *priv)
  {
  	struct __find_tracepoint_cb_data *data = priv;

@@ -905,14 +922,28 @@ static void __find_tracepoint_cb(struct tracepoint *tp, void *priv)
  		data->tpoint = tp;
  }
  
-static struct tracepoint *find_tracepoint(const char *tp_name)
+/*
+ * Find a tracepoint from kernel and module. If the tracepoint is in a module,
+ * this increments the module refcount to prevent unloading until the
+ * trace_fprobe is registered to the list. After registering the trace_fprobe
+ * on the trace_fprobe list, the module refcount is decremented because
+ * tracepoint_probe_module_cb will handle it.
+ */
+static struct tracepoint *find_tracepoint(const char *tp_name,
+					  struct module **tp_mod)
  {
  	struct __find_tracepoint_cb_data data = {
  		.tp_name = tp_name,
+		.mod = NULL,
  	};
  
  	for_each_kernel_tracepoint(__find_tracepoint_cb, &data);
  
+	if (!data.tpoint && IS_ENABLED(CONFIG_MODULES)) {
+		for_each_module_tracepoint(__find_tracepoint_module_cb, &data);
+		*tp_mod = data.mod;
+	}
+
  	return data.tpoint;
  }
  

@@ -996,6 +1027,7 @@ static int __trace_fprobe_create(int argc, const char *argv[])
  	char abuf[MAX_BTF_ARGS_LEN];
  	char *dbuf = NULL;
  	bool is_tracepoint = false;
+	struct module *tp_mod = NULL;
  	struct tracepoint *tpoint = NULL;
  	struct traceprobe_parse_context ctx = {
  		.flags = TPARG_FL_KERNEL | TPARG_FL_FPROBE,

@@ -1080,7 +1112,7 @@ static int __trace_fprobe_create(int argc, const char *argv[])
  
  	if (is_tracepoint) {
  		ctx.flags |= TPARG_FL_TPOINT;
-		tpoint = find_tracepoint(symbol);
+		tpoint = find_tracepoint(symbol, &tp_mod);
  		if (!tpoint) {
  			trace_probe_log_set_index(1);
  			trace_probe_log_err(0, NO_TRACEPOINT);

@@ -1110,8 +1142,8 @@ static int __trace_fprobe_create(int argc, const char *argv[])
  		goto out;
  
  	/* setup a probe */
-	tf = alloc_trace_fprobe(group, event, symbol, tpoint, maxactive,
-				argc, is_return);
+	tf = alloc_trace_fprobe(group, event, symbol, tpoint, tp_mod,
+				maxactive, argc, is_return);
  	if (IS_ERR(tf)) {
  		ret = PTR_ERR(tf);
  		/* This must return -ENOMEM, else there is a bug */

@@ -1119,10 +1151,6 @@ static int __trace_fprobe_create(int argc, const char *argv[])
  		goto out;	/* We know tf is not allocated */
  	}
  
-	if (is_tracepoint)
-		tf->mod = __module_text_address(
-				(unsigned long)tf->tpoint->probestub);
-
  	/* parse arguments */
  	for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) {
  		trace_probe_log_set_index(i + 2);

@@ -1155,6 +1183,8 @@ static int __trace_fprobe_create(int argc, const char *argv[])
  	}
  
  out:
+	if (tp_mod)
+		module_put(tp_mod);
  	traceprobe_finish_parse(&ctx);
  	trace_probe_log_clear();
  	kfree(new_argv);

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

-- 
Masami Hiramatsu (Google) [off-list ref]