Re: [PATCH] tracing: separate module tracepoint strings from trace_printk formats

Petr Pavlu
2026-04-14T11:37:47Z

On 4/13/26 2:33 PM, Cao Ruichuang wrote:
> The previous module tracepoint_string() fix took the smallest
> implementation path and reused the existing module trace_printk format
> storage.
>
> That was enough to make module __tracepoint_str entries show up in
> printk_formats and be accepted by trace_is_tracepoint_string(), but it
> also made those copied mappings persist after module unload. That does
> not match the expected module lifetime semantics.
>
> Handle module tracepoint_string() mappings separately instead of mixing
> them into the module trace_printk format list. Keep copying the strings
> into tracing-managed storage while the module is loaded, but track them
> on their own list and drop them again on MODULE_STATE_GOING.
>
> Keep module trace_printk format handling unchanged.
>
> This split is intentional: module trace_printk formats and module
> tracepoint_string() mappings do not have the same lifetime requirements.
> Keeping them in one shared structure would either preserve
> tracepoint_string() mappings too long again, or require mixed
> ownership/refcount rules in a trace_printk-oriented structure.
>
> The separate module tracepoint_string() list intentionally keeps one
> copied mapping per module entry instead of trying to share copies across
> modules by string contents. printk_formats is address-based, and sharing
> those copies would add another layer of shared ownership/refcounting
> without changing the lifetime rule this fix is trying to restore.

I believe it would be more productive to first answer the question in my
previous email and give others time to comment on the noted issue and
discuss a possible solution. The previous patch hasn't been accepted
yet, so sending a new patch on top of it just hours after my comment
seems unusual.

The patch contains some unrelated changes, such as renaming last_index
to next_index in find_next(). I suspect it was generated with the help
of AI but it isn't attributed as such. Please check
Documentation/process/coding-assistants.rst.

The implementation splits the trace_bprintk_fmt_list into two lists.
While this looks sensible, it creates nearly duplicate code for handling
the two lists, for example, hold_module_trace_format() and
hold_module_trace_format(), or lookup_mod_format_ptr() and
lookup_mod_tracepoint_ptr(). An alternative would be to keep one list
and have a 'struct module *mod' member to differentiate between the
strings. 'mod == NULL' for printk fmts and 'mod != NULL' for tracepoint
strings.

I also considered whether kernel/trace/trace_printk.c could avoid
tracking tracepoint strings altogether and instead have
find_next_mod_entry() directly traverse the modules list and
module::tracepoint_strings_start. However, this would require taking
both btrace_mutex and the RCU lock, and I believe it would be more
complex.

Since the tracepoint strings are now tracked only for the duration of
a module's lifetime, kernel/trace/trace_printk.c doesn't need to copy
the strings and can reference directly the original module data.
Alternatively, the original strings could be placed in an .init section
so that once trace_printk.c copies them and the module is initialized,
they get dropped.

--
Thanks,
Petr

>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217196
> Signed-off-by: Cao Ruichuang <create0818@163.com>
> ---
> include/linux/tracepoint.h | 9 +-
> kernel/trace/trace_printk.c | 250 ++++++++++++++++++++++--------------
> 2 files changed, 157 insertions(+), 102 deletions(-)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index f14da542402..aec598a4017 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -479,11 +479,10 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> *
> * For built-in code, the tracing system uses the original string address.
> * For modules, the tracing code saves tracepoint strings into
> - * tracing-managed storage when the module loads, so their mappings remain
> - * available through printk_formats and trace string checks even after the
> - * module's own memory goes away. As long as the string does not change
> - * during the life of the module, it is fine to use tracepoint_string()
> - * within a module.
> + * tracing-managed storage while the module is loaded, and drops those
> + * mappings again when the module unloads. 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/trace/trace_printk.c b/kernel/trace/trace_printk.c
> index 9f67ce42ef6..0420ffcff93 100644
> --- a/kernel/trace/trace_printk.c
> +++ b/kernel/trace/trace_printk.c
> @@ -21,24 +21,24 @@
>
> #ifdef CONFIG_MODULES
>
> -/*
> - * modules trace_printk() formats and tracepoint_string() strings are
> - * autosaved in struct trace_bprintk_fmt, which are queued on
> - * trace_bprintk_fmt_list.
> - */
> +/* module trace_printk() formats are autosaved on trace_bprintk_fmt_list. */
> static LIST_HEAD(trace_bprintk_fmt_list);
> +/* module tracepoint_string() copies live on tracepoint_string_list. */
> +static LIST_HEAD(tracepoint_string_list);
>
> -/* serialize accesses to trace_bprintk_fmt_list */
> +/* serialize accesses to module format and tracepoint-string lists */
> static DEFINE_MUTEX(btrace_mutex);
>
> struct trace_bprintk_fmt {
> struct list_head list;
> const char *fmt;
> - unsigned int type;
> };
>
> -#define TRACE_BPRINTK_TYPE BIT(0)
> -#define TRACE_TRACEPOINT_TYPE BIT(1)
> +struct tracepoint_string_entry {
> + struct list_head list;
> + struct module *mod;
> + const char *str;
> +};
>
> static inline struct trace_bprintk_fmt *lookup_format(const char *fmt)
> {
> @@ -54,24 +54,21 @@ static inline struct trace_bprintk_fmt *lookup_format(const char *fmt)
> return NULL;
> }
>
> -static void hold_module_trace_format(const char **start, const char **end,
> - unsigned int type)
> +static void hold_module_trace_bprintk_format(const char **start, const char **end)
> {
> const char **iter;
> char *fmt;
>
> /* allocate the trace_printk per cpu buffers */
> - if ((type & TRACE_BPRINTK_TYPE) && start != end)
> + if (start != end)
> trace_printk_init_buffers();
>
> mutex_lock(&btrace_mutex);
> for (iter = start; iter < end; iter++) {
> struct trace_bprintk_fmt *tb_fmt = lookup_format(*iter);
> if (tb_fmt) {
> - if (!IS_ERR(tb_fmt)) {
> - tb_fmt->type |= type;
> + if (!IS_ERR(tb_fmt))
> *iter = tb_fmt->fmt;
> - }
> continue;
> }
>
> @@ -83,7 +80,6 @@ static void hold_module_trace_format(const char **start, const char **end,
> list_add_tail(&tb_fmt->list, &trace_bprintk_fmt_list);
> strcpy(fmt, *iter);
> tb_fmt->fmt = fmt;
> - tb_fmt->type = type;
> } else
> kfree(tb_fmt);
> }
> @@ -93,89 +89,156 @@ static void hold_module_trace_format(const char **start, const char **end,
> mutex_unlock(&btrace_mutex);
> }
>
> +static void hold_module_tracepoint_strings(struct module *mod,
> + const char **start,
> + const char **end)
> +{
> + const char **iter;
> +
> + mutex_lock(&btrace_mutex);
> + for (iter = start; iter < end; iter++) {
> + struct tracepoint_string_entry *tp_entry;
> + char *str;
> +
> + tp_entry = kmalloc_obj(*tp_entry);
> + if (!tp_entry)
> + continue;
> +
> + str = kstrdup(*iter, GFP_KERNEL);
> + if (!str) {
> + kfree(tp_entry);
> + continue;
> + }
> +
> + tp_entry->mod = mod;
> + tp_entry->str = str;
> + list_add_tail(&tp_entry->list, &tracepoint_string_list);
> + *iter = tp_entry->str;
> + }
> + mutex_unlock(&btrace_mutex);
> +}
> +
> +static void release_module_tracepoint_strings(struct module *mod)
> +{
> + struct tracepoint_string_entry *tp_entry, *n;
> +
> + mutex_lock(&btrace_mutex);
> + list_for_each_entry_safe(tp_entry, n, &tracepoint_string_list, list) {
> + if (tp_entry->mod != mod)
> + continue;
> + list_del(&tp_entry->list);
> + kfree(tp_entry->str);
> + kfree(tp_entry);
> + }
> + mutex_unlock(&btrace_mutex);
> +}
> +
> static int module_trace_format_notify(struct notifier_block *self,
> unsigned long val, void *data)
> {
> struct module *mod = data;
>
> - if (val != MODULE_STATE_COMING)
> - return NOTIFY_OK;
> + 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;
>
> - 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);
> + }
> +
> + if (mod->num_tracepoint_strings) {
> + const char **start = mod->tracepoint_strings_start;
> + const char **end = start + mod->num_tracepoint_strings;
>
> - hold_module_trace_format(start, end, TRACE_BPRINTK_TYPE);
> + hold_module_tracepoint_strings(mod, start, end);
> + }
> + break;
> + case MODULE_STATE_GOING:
> + release_module_tracepoint_strings(mod);
> + break;
> }
>
> - if (mod->num_tracepoint_strings) {
> - const char **start = mod->tracepoint_strings_start;
> - const char **end = start + mod->num_tracepoint_strings;
> + return NOTIFY_OK;
> +}
>
> - hold_module_trace_format(start, end, TRACE_TRACEPOINT_TYPE);
> +static const char **find_first_mod_entry(void)
> +{
> + struct trace_bprintk_fmt *tb_fmt;
> + struct tracepoint_string_entry *tp_entry;
> +
> + if (!list_empty(&trace_bprintk_fmt_list)) {
> + tb_fmt = list_first_entry(&trace_bprintk_fmt_list,
> + typeof(*tb_fmt), list);
> + return &tb_fmt->fmt;
> }
>
> - return NOTIFY_OK;
> + if (!list_empty(&tracepoint_string_list)) {
> + tp_entry = list_first_entry(&tracepoint_string_list,
> + typeof(*tp_entry), list);
> + return &tp_entry->str;
> + }
> +
> + return NULL;
> }
>
> -/*
> - * The debugfs/tracing/printk_formats file maps the addresses with
> - * the ASCII formats that are used in the bprintk events in the
> - * buffer. For userspace tools to be able to decode the events from
> - * the buffer, they need to be able to map the address with the format.
> - *
> - * The addresses of the bprintk formats are in their own section
> - * __trace_printk_fmt. But for modules we copy them into a link list.
> - * The code to print the formats and their addresses passes around the
> - * address of the fmt string. If the fmt address passed into the seq
> - * functions is within the kernel core __trace_printk_fmt section, then
> - * it simply uses the next pointer in the list.
> - *
> - * When the fmt pointer is outside the kernel core __trace_printk_fmt
> - * section, then we need to read the link list pointers. The trick is
> - * we pass the address of the string to the seq function just like
> - * we do for the kernel core formats. To get back the structure that
> - * holds the format, we simply use container_of() and then go to the
> - * next format in the list.
> - */
> -static const char **
> -find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
> +static struct trace_bprintk_fmt *lookup_mod_format_ptr(const char **fmt_ptr)
> {
> - struct trace_bprintk_fmt *mod_fmt;
> + struct trace_bprintk_fmt *tb_fmt;
>
> - if (list_empty(&trace_bprintk_fmt_list))
> - return NULL;
> + list_for_each_entry(tb_fmt, &trace_bprintk_fmt_list, list) {
> + if (fmt_ptr == &tb_fmt->fmt)
> + return tb_fmt;
> + }
>
> - /*
> - * v will point to the address of the fmt record from t_next
> - * v will be NULL from t_start.
> - * If this is the first pointer or called from start
> - * then we need to walk the list.
> - */
> - if (!v || start_index == *pos) {
> - struct trace_bprintk_fmt *p;
> -
> - /* search the module list */
> - list_for_each_entry(p, &trace_bprintk_fmt_list, list) {
> - if (start_index == *pos)
> - return &p->fmt;
> - start_index++;
> + return NULL;
> +}
> +
> +static struct tracepoint_string_entry *lookup_mod_tracepoint_ptr(const char **str_ptr)
> +{
> + struct tracepoint_string_entry *tp_entry;
> +
> + list_for_each_entry(tp_entry, &tracepoint_string_list, list) {
> + if (str_ptr == &tp_entry->str)
> + return tp_entry;
> + }
> +
> + return NULL;
> +}
> +
> +static const char **find_next_mod_entry(int start_index, void *v, loff_t *pos)
> +{
> + struct trace_bprintk_fmt *tb_fmt;
> + struct tracepoint_string_entry *tp_entry;
> +
> + if (!v || start_index == *pos)
> + return find_first_mod_entry();
> +
> + tb_fmt = lookup_mod_format_ptr(v);
> + if (tb_fmt) {
> + if (tb_fmt->list.next != &trace_bprintk_fmt_list) {
> + tb_fmt = list_next_entry(tb_fmt, list);
> + return &tb_fmt->fmt;
> }
> - /* pos > index */
> +
> + if (!list_empty(&tracepoint_string_list)) {
> + tp_entry = list_first_entry(&tracepoint_string_list,
> + typeof(*tp_entry), list);
> + return &tp_entry->str;
> + }
> +
> return NULL;
> }
>
> - /*
> - * v points to the address of the fmt field in the mod list
> - * structure that holds the module print format.
> - */
> - mod_fmt = container_of(v, typeof(*mod_fmt), fmt);
> - if (mod_fmt->list.next == &trace_bprintk_fmt_list)
> + tp_entry = lookup_mod_tracepoint_ptr(v);
> + if (!tp_entry)
> return NULL;
>
> - mod_fmt = container_of(mod_fmt->list.next, typeof(*mod_fmt), list);
> + if (tp_entry->list.next == &tracepoint_string_list)
> + return NULL;
>
> - return &mod_fmt->fmt;
> + tp_entry = list_next_entry(tp_entry, list);
> + return &tp_entry->str;
> }
>
> static void format_mod_start(void)
> @@ -195,8 +258,8 @@ module_trace_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)
> +static inline const char **find_next_mod_entry(int start_index, void *v,
> + loff_t *pos)
> {
> return NULL;
> }
> @@ -274,7 +337,7 @@ bool trace_is_tracepoint_string(const char *str)
> {
> const char **ptr = __start___tracepoint_str;
> #ifdef CONFIG_MODULES
> - struct trace_bprintk_fmt *tb_fmt;
> + struct tracepoint_string_entry *tp_entry;
> #endif
>
> for (ptr = __start___tracepoint_str; ptr < __stop___tracepoint_str; ptr++) {
> @@ -284,8 +347,8 @@ bool trace_is_tracepoint_string(const char *str)
>
> #ifdef CONFIG_MODULES
> mutex_lock(&btrace_mutex);
> - list_for_each_entry(tb_fmt, &trace_bprintk_fmt_list, list) {
> - if ((tb_fmt->type & TRACE_TRACEPOINT_TYPE) && str == tb_fmt->fmt) {
> + list_for_each_entry(tp_entry, &tracepoint_string_list, list) {
> + if (str == tp_entry->str) {
> mutex_unlock(&btrace_mutex);
> return true;
> }
> @@ -297,9 +360,8 @@ bool trace_is_tracepoint_string(const char *str)
>
> static const char **find_next(void *v, loff_t *pos)
> {
> - const char **fmt = v;
> int start_index;
> - int last_index;
> + int next_index;
>
> start_index = __stop___trace_bprintk_fmt - __start___trace_bprintk_fmt;
>
> @@ -307,25 +369,19 @@ static const char **find_next(void *v, loff_t *pos)
> return __start___trace_bprintk_fmt + *pos;
>
> /*
> - * The __tracepoint_str section is treated the same as the
> - * __trace_printk_fmt section. The difference is that the
> - * __trace_printk_fmt section should only be used by trace_printk()
> - * in a debugging environment, as if anything exists in that section
> - * the trace_prink() helper buffers are allocated, which would just
> - * waste space in a production environment.
> - *
> - * The __tracepoint_str sections on the other hand are used by
> - * tracepoints which need to map pointers to their strings to
> - * the ASCII text for userspace.
> + * Built-in __tracepoint_str entries are exported directly from the
> + * core section. Module tracepoint_string() mappings are kept on a
> + * separate tracing-managed list below, because their lifetime is tied
> + * to module load/unload and differs from module trace_printk() formats.
> */
> - last_index = start_index;
> + next_index = start_index;
> start_index = __stop___tracepoint_str - __start___tracepoint_str;
>
> - if (*pos < last_index + start_index)
> - return __start___tracepoint_str + (*pos - last_index);
> + if (*pos < next_index + start_index)
> + return __start___tracepoint_str + (*pos - next_index);
>
> - start_index += last_index;
> - return find_next_mod_format(start_index, v, fmt, pos);
> + start_index += next_index;
> + return find_next_mod_entry(start_index, v, pos);
> }
>
> static void *