Re: [PATCH v3] tracing: Export live module tracepoint strings in printk_formats

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 11 Aug 2026 18:15:13 +0000
Cao Ruichuang <create0818@163.com> wrote:

Hi Cao,

Sorry for the late reply, with AI bugs coming in, as well as travel and
conferences, I fell very behind in submitted patches.

> diff --git a/include/linux/module.h b/include/linux/module.h
> index 7566815fa..d9ac1fd6e 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -513,7 +513,9 @@ struct module {
>  #endif
>  #ifdef CONFIG_TRACING
>  	unsigned int num_trace_bprintk_fmt;
> +	unsigned int num_tracepoint_strings;
>  	const char **trace_bprintk_fmt_start;
> +	const char **tracepoint_strings_start;
>  #endif
>  #ifdef CONFIG_EVENT_TRACING
>  	struct trace_event_call **trace_events;
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index e0d838c9c..2b973cc72 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -508,10 +508,10 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>   * the ASCII strings they represent.
>   *
>   * The @str used must be a constant string and persistent as it would not
> - * make sense to show a string that no longer exists. But it is still fine
> - * to be used with modules, because when modules are unloaded, if they
> - * had tracepoints, the ring buffers are cleared too. As long as the string
> - * does not change during the life of the module, it is fine to use
> + * make sense to show a string that no longer exists. For modules, the string
> + * mapping is exported while the module is loaded. When the module is unloaded,
> + * its trace buffers are cleared before the mapping is removed. As long as the
> + * string does not change during the life of the module, it is fine to use
>   * tracepoint_string() within a module.
>   */
>  #define tracepoint_string(str)						\
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 46dd8d25a..cb0acb2ce 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2729,6 +2729,10 @@ static int find_module_sections(struct module *mod, struct load_info *info)
>  	mod->trace_bprintk_fmt_start = section_objs(info, "__trace_printk_fmt",
>  					 sizeof(*mod->trace_bprintk_fmt_start),
>  					 &mod->num_trace_bprintk_fmt);
> +	mod->tracepoint_strings_start =
> +		section_objs(info, "__tracepoint_str",
> +			     sizeof(*mod->tracepoint_strings_start),
> +			     &mod->num_tracepoint_strings);
>  #endif
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  	/* sechdrs[0].sh_size is always zero */
> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
> index 98171a239..2aca8d206 100644
> --- a/kernel/trace/trace_printk.c
> +++ b/kernel/trace/trace_printk.c
> @@ -13,21 +13,29 @@
>  #include <linux/string.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/rcupdate.h>

Please keep this in "upside-down x-mas tree" format. The order of headers
goes by the length of their names.

>  #include <linux/ctype.h>
>  #include <linux/list.h>
>  #include <linux/slab.h>
>  
>  #include "trace.h"
>  
> +static int trace_bprintk_fmt_cnt;
> +
>  #ifdef CONFIG_MODULES
>  
>  /*
>   * modules trace_printk()'s formats are autosaved in struct trace_bprintk_fmt
>   * which are queued on trace_bprintk_fmt_list.
> + *
> + * modules tracepoint_string() entries are kept as ranges into the owning
> + * module's __tracepoint_str section and are removed again when the module
> + * goes away.
>   */
>  static LIST_HEAD(trace_bprintk_fmt_list);
> +static LIST_HEAD(tracepoint_str_list);
>  
> -/* serialize accesses to trace_bprintk_fmt_list */
> +/* serialize updates and printk_formats iteration of the module lists */
>  static DEFINE_MUTEX(btrace_mutex);
>  
>  struct trace_bprintk_fmt {
> @@ -35,6 +43,14 @@ struct trace_bprintk_fmt {
>  	const char *fmt;
>  };
>  
> +struct tracepoint_mod_str {
> +	struct list_head list;
> +	struct rcu_head rcu;
> +	struct module *mod;
> +	const char **start;
> +	unsigned int num;
> +};
> +
>  static inline struct trace_bprintk_fmt *lookup_format(const char *fmt)
>  {
>  	struct trace_bprintk_fmt *pos;
> @@ -75,6 +91,7 @@ void hold_module_trace_bprintk_format(const char **start, const char **end)
>  			if (fmt) {
>  				list_add_tail(&tb_fmt->list, &trace_bprintk_fmt_list);
>  				tb_fmt->fmt = fmt;
> +				trace_bprintk_fmt_cnt++;
>  			} else
>  				kfree(tb_fmt);
>  		}
> @@ -84,16 +101,63 @@ void hold_module_trace_bprintk_format(const char **start, const char **end)
>  	mutex_unlock(&btrace_mutex);
>  }
>  
> +static void hold_module_tracepoint_strings(struct module *mod)
> +{
> +	struct tracepoint_mod_str *tp_str;
> +
> +	if (!mod->num_tracepoint_strings)
> +		return;
> +
> +	tp_str = kmalloc_obj(*tp_str);
> +	if (!tp_str)
> +		return;
> +
> +	tp_str->mod = mod;
> +	tp_str->start = mod->tracepoint_strings_start;
> +	tp_str->num = mod->num_tracepoint_strings;
> +
> +	guard(mutex)(&btrace_mutex);
> +	list_add_tail_rcu(&tp_str->list, &tracepoint_str_list);
> +}
> +
> +static void release_module_tracepoint_strings(struct module *mod)
> +{
> +	struct tracepoint_mod_str *tp_str;
> +
> +	if (!mod->num_tracepoint_strings)
> +		return;
> +
> +	guard(mutex)(&btrace_mutex);
> +	list_for_each_entry(tp_str, &tracepoint_str_list, list) {

Please use list_for_each_entry_safe(). You are removing tp_str from the
list. Do not rely on internals of RCU to protect this. If the lock was a
spin lock, then it may have been safe (even though I would still ask for
this change). But because it is a mutex, this could be preempted, the RCU
grace period finish, the tp_str freed and then the loop will crash.

> +		if (tp_str->mod != mod)
> +			continue;
> +
> +		list_del_rcu(&tp_str->list);
> +		/* Module teardown waits for RCU before freeing the section. */
> +		kfree_rcu(tp_str, rcu);
> +		return;
> +	}
> +}
> +
>  static int module_trace_bprintk_format_notify(struct notifier_block *self,
>  		unsigned long val, void *data)
>  {
>  	struct module *mod = data;
> -	if (mod->num_trace_bprintk_fmt) {
> -		const char **start = mod->trace_bprintk_fmt_start;
> -		const char **end = start + mod->num_trace_bprintk_fmt;
>  
> -		if (val == MODULE_STATE_COMING)
> +	switch (val) {
> +	case MODULE_STATE_COMING:
> +		if (mod->num_trace_bprintk_fmt) {
> +			const char **start = mod->trace_bprintk_fmt_start;
> +			const char **end = start + mod->num_trace_bprintk_fmt;
> +
>  			hold_module_trace_bprintk_format(start, end);
> +		}
> +		hold_module_tracepoint_strings(mod);
> +		break;
> +	case MODULE_STATE_GOING:
> +		/* Trace event teardown runs first and clears module event buffers. */
> +		release_module_tracepoint_strings(mod);
> +		break;
>  	}
>  	return NOTIFY_OK;
>  }
> @@ -119,7 +183,7 @@ static int module_trace_bprintk_format_notify(struct notifier_block *self,
>   * next format in the list.
>   */
>  static const char **
> -find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
> +find_next_mod_format(loff_t start_index, void *v, const char **fmt, loff_t *pos)
>  {
>  	struct trace_bprintk_fmt *mod_fmt;
>  
> @@ -158,6 +222,42 @@ find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
>  	return &mod_fmt->fmt;
>  }
>  
> +static const char **
> +find_next_mod_tracepoint_str(loff_t start_index, loff_t *pos)
> +{
> +	struct tracepoint_mod_str *tp_str;
> +	loff_t offset = *pos - start_index;
> +
> +	if (offset < 0)
> +		return NULL;
> +
> +	list_for_each_entry(tp_str, &tracepoint_str_list, list) {
> +		if (offset < (loff_t)tp_str->num)
> +			return tp_str->start + offset;
> +
> +		offset -= tp_str->num;
> +	}
> +
> +	return NULL;
> +}
> +
> +static bool is_module_tracepoint_string(const char *str)
> +{
> +	struct tracepoint_mod_str *tp_str;
> +	unsigned int i;
> +
> +	/* The tracepoint printk path can call this with IRQs disabled. */
> +	guard(rcu)();
> +	list_for_each_entry_rcu(tp_str, &tracepoint_str_list, list) {
> +		for (i = 0; i < tp_str->num; i++) {
> +			if (str == tp_str->start[i])
> +				return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  static void format_mod_start(void)
>  {
>  	mutex_lock(&btrace_mutex);
> @@ -176,10 +276,21 @@ module_trace_bprintk_format_notify(struct notifier_block *self,
>  	return NOTIFY_OK;
>  }
>  static inline const char **
> -find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
> +find_next_mod_format(loff_t start_index, void *v, const char **fmt, loff_t *pos)
> +{
> +	return NULL;
> +}
> +
> +static inline const char **
> +find_next_mod_tracepoint_str(loff_t start_index, loff_t *pos)
>  {
>  	return NULL;
>  }
> +
> +static inline bool is_module_tracepoint_string(const char *str)
> +{
> +	return false;
> +}
>  static inline void format_mod_start(void) { }
>  static inline void format_mod_stop(void) { }
>  #endif /* CONFIG_MODULES */
> @@ -194,6 +305,7 @@ void trace_printk_control(bool enabled)
>  __initdata_or_module static
>  struct notifier_block module_trace_bprintk_format_nb = {
>  	.notifier_call = module_trace_bprintk_format_notify,
> +	.priority = 0, /* lower than the trace event notifier */
>  };
>  
>  __printf(2, 3)
> @@ -259,14 +371,14 @@ bool trace_is_tracepoint_string(const char *str)
>  		if (str == *ptr)
>  			return true;
>  	}
> -	return false;
> +	return is_module_tracepoint_string(str);
>  }
>  
>  static const char **find_next(void *v, loff_t *pos)
>  {
>  	const char **fmt = v;
> -	int start_index;
> -	int last_index;
> +	loff_t start_index;
> +	loff_t last_index;
>  
>  	start_index = __stop___trace_bprintk_fmt - __start___trace_bprintk_fmt;
>  
> @@ -292,7 +404,11 @@ static const char **find_next(void *v, loff_t *pos)
>  		return __start___tracepoint_str + (*pos - last_index);
>  
>  	start_index += last_index;
> -	return find_next_mod_format(start_index, v, fmt, pos);
> +	if (*pos - start_index < trace_bprintk_fmt_cnt)
> +		return find_next_mod_format(start_index, v, fmt, pos);
> +
> +	start_index += trace_bprintk_fmt_cnt;
> +	return find_next_mod_tracepoint_str(start_index, pos);
>  }
>  
>  static void *
> diff --git a/samples/trace_printk/trace-printk.c b/samples/trace_printk/trace-printk.c
> index cfc159580..54c709c82 100644
> --- a/samples/trace_printk/trace-printk.c
> +++ b/samples/trace_printk/trace-printk.c
> @@ -2,6 +2,7 @@
>  #include <linux/module.h>
>  #include <linux/kthread.h>
>  #include <linux/irq_work.h>
> +#include <linux/tracepoint.h>
>  
>  /* Must not be static to force gcc to consider these non constant */
>  char *trace_printk_test_global_str =
> @@ -31,6 +32,9 @@ static int __init trace_printk_init(void)
>  {
>  	init_irq_work(&irqwork, trace_printk_irq_work);
>  
> +	(void)tracepoint_string("tracepoint_string_test_module_string_1");
> +	(void)tracepoint_string("tracepoint_string_test_module_string_2");

Actually this shouldn't be in trace_printk() as that's not how it would be
used. Please add it to samples. Something like this:

diff --git a/samples/trace_events/trace-events-sample.c b/samples/trace_events/trace-events-sample.c
index ca5d98c360cb..e00630c2d922 100644
--- a/samples/trace_events/trace-events-sample.c
+++ b/samples/trace_events/trace-events-sample.c
@@ -51,6 +51,10 @@ static void do_simple_thread_func(int cnt, const char *fmt, ...)
 	trace_foo_with_template_print("I have to be different", cnt);
 
 	trace_foo_rel_loc("Hello __rel_loc", cnt, bitmask, current->cpus_ptr);
+
+	trace_foo_tracepoint_str(tracepoint_string("tracepoint_string_test_module_string_1"));
+	trace_foo_tracepoint_str(tracepoint_string("tracepoint_string_test_module_string_2"));
+
 }
 
 static void simple_thread_func(int cnt)
diff --git a/samples/trace_events/trace-events-sample.h b/samples/trace_events/trace-events-sample.h
index 816848a456a2..48bca97086b6 100644
--- a/samples/trace_events/trace-events-sample.h
+++ b/samples/trace_events/trace-events-sample.h
@@ -623,6 +623,23 @@ TRACE_EVENT(foo_timer_fn,
 
 	TP_printk("name=%s count=%d", __get_str(name), __entry->count)
 );
+
+TRACE_EVENT(foo_tracepoint_str,
+
+	TP_PROTO(const char *str),
+
+	TP_ARGS(str),
+
+	TP_STRUCT__entry(
+		__field(	const char *,		str	)
+	),
+
+	TP_fast_assign(
+		__entry->str = str;
+	),
+
+	TP_printk("str=%s", __entry->str)
+);
 #endif
 
 /***** NOTICE! The #if protection ends here. *****/


> +
>  	trace_printk("This is a static string that will use trace_bputs\n");
>  	trace_printk(trace_printk_test_global_str);
>  
> diff --git a/tools/testing/selftests/ftrace/test.d/event/trace_printk.tc b/tools/testing/selftests/ftrace/test.d/event/trace_printk.tc
> index b02550b42..d66a8134c 100644
> --- a/tools/testing/selftests/ftrace/test.d/event/trace_printk.tc
> +++ b/tools/testing/selftests/ftrace/test.d/event/trace_printk.tc
> @@ -23,5 +23,10 @@ grep -q ": (irq) .* trace_printk" trace
>  
>  grep -q "This is a %s that will use trace_bprintk" printk_formats
>  grep -q "(irq) This is a static string that will use trace_bputs" printk_formats
> +grep -q "tracepoint_string_test_module_string_1" printk_formats
> +grep -q "tracepoint_string_test_module_string_2" printk_formats
>  
>  rmmod trace-printk ||:
> +
> +! grep -q "tracepoint_string_test_module_string_1" printk_formats
> +! grep -q "tracepoint_string_test_module_string_2" printk_formats

You would need different test that relies on the trace event sample code
and do this test there.


> 
> base-commit: d5b201ed4feb7a11da242a97669424efeb724031


Thanks,

-- Steve