All of lore.kernel.org
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: chensong_2000@189.cn
Cc: mcgrof@kernel.org, petr.pavlu@suse.com, da.gomez@kernel.org,
	samitolvanen@google.com, atomlin@atomlin.com,
	mhiramat@kernel.org, mark.rutland@arm.com,
	mathieu.desnoyers@efficios.com, linux-modules@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org
Subject: Re: [PATCH] kernel/trace/ftrace: introduce ftrace module notifier
Date: Wed, 25 Feb 2026 19:27:24 -0500	[thread overview]
Message-ID: <20260225192724.48ed165e@fedora> (raw)
In-Reply-To: <20260225054639.21637-1-chensong_2000@189.cn>

On Wed, 25 Feb 2026 13:46:39 +0800
chensong_2000@189.cn wrote:

> From: Song Chen <chensong_2000@189.cn>
> 
> Like kprobe, fprobe and btf, this patch attempts to introduce
> a notifier_block for ftrace to decouple its initialization from
> load_module.
> 
> Below is the table of ftrace fucntions calls in different
> module state:
> 
> 	MODULE_STATE_UNFORMED	ftrace_module_init
> 	MODULE_STATE_COMING	ftrace_module_enable
> 	MODULE_STATE_LIVE	ftrace_free_mem
> 	MODULE_STATE_GOING	ftrace_release_mod
> 
> Unlike others, ftrace module notifier must take care of state
> MODULE_STATE_UNFORMED to ensure calling ftrace_module_init
> before complete_formation which changes module's text property.
> 
> That pretty much remains same logic with its original design,
> the only thing that changes is blocking_notifier_call_chain
> (MODULE_STATE_GOING) has to be moved from coming_cleanup to
> ddebug_cleanup in function load_module to ensure
> ftrace_release_mod is invoked in case complete_formation fails.
> 
> Signed-off-by: Song Chen <chensong_2000@189.cn>
> ---
>  kernel/module/main.c  | 14 ++++----------
>  kernel/trace/ftrace.c | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 710ee30b3bea..5dc0a980e9bd 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -45,7 +45,6 @@
>  #include <linux/license.h>
>  #include <asm/sections.h>
>  #include <linux/tracepoint.h>
> -#include <linux/ftrace.h>
>  #include <linux/livepatch.h>
>  #include <linux/async.h>
>  #include <linux/percpu.h>
> @@ -836,7 +835,6 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
>  	klp_module_going(mod);
> -	ftrace_release_mod(mod);

Is the above safe? klp uses ftrace. That means klp_module_going() may
need to be called before ftrace_release_mod(). That said, I wonder if
klp_module_going() could be moved into ftrace_release_mod()?

>  
>  	async_synchronize_full();
>  
> @@ -3067,8 +3065,6 @@ static noinline int do_init_module(struct module *mod)
>  	if (!mod->async_probe_requested)
>  		async_synchronize_full();
>  
> -	ftrace_free_mem(mod, mod->mem[MOD_INIT_TEXT].base,
> -			mod->mem[MOD_INIT_TEXT].base + mod->mem[MOD_INIT_TEXT].size);

Have you tested the case for why this is called? It has to be called
before the module frees the kallsyms. It's for tracing the module's
init functions.

  cd /sys/kernel/tracing
  echo :mod:<module> > set_ftrace_filter
  echo function > current_tracer
  modprobe <module>
  cat trace

You should see the init functions of the module loaded. If
ftrace_free_mem() is called after the module frees the kallsyms of the
module init functions, you'll just get garbage for the init function
names.



>  	mutex_lock(&module_mutex);
>  	/* Drop initial reference. */
>  	module_put(mod);
> @@ -3131,7 +3127,6 @@ static noinline int do_init_module(struct module *mod)
>  	blocking_notifier_call_chain(&module_notify_list,
>  				     MODULE_STATE_GOING, mod);
>  	klp_module_going(mod);
> -	ftrace_release_mod(mod);
>  	free_module(mod);
>  	wake_up_all(&module_wq);
>  
> @@ -3278,7 +3273,6 @@ static int prepare_coming_module(struct module *mod)
>  {
>  	int err;
>  
> -	ftrace_module_enable(mod);
>  	err = klp_module_coming(mod);

Same issue with ftrace and klp here.

>  	if (err)
>  		return err;
> @@ -3461,7 +3455,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	init_build_id(mod, info);
>  
>  	/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
> -	ftrace_module_init(mod);
> +	blocking_notifier_call_chain(&module_notify_list,
> +				MODULE_STATE_UNFORMED, mod);
>  
>  	/* Finally it's fully formed, ready to start executing. */
>  	err = complete_formation(mod, info);
> @@ -3513,8 +3508,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
>   coming_cleanup:
>  	mod->state = MODULE_STATE_GOING;
>  	destroy_params(mod->kp, mod->num_kp);
> -	blocking_notifier_call_chain(&module_notify_list,
> -				     MODULE_STATE_GOING, mod);
>  	klp_module_going(mod);

Now klp_module_going() may need to be called *after* the
MODULE_STATE_GOING callbacks and *before* ftrace_release_mod(). But
again, if that's moved into ftrace_release_mod() it may be fine.

>   bug_cleanup:
>  	mod->state = MODULE_STATE_GOING;
> @@ -3524,7 +3517,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	mutex_unlock(&module_mutex);
>  
>   ddebug_cleanup:
> -	ftrace_release_mod(mod);
> +	blocking_notifier_call_chain(&module_notify_list,
> +				     MODULE_STATE_GOING, mod);
>  	synchronize_rcu();
>  	kfree(mod->args);
>   free_arch_cleanup:
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c

-- Steve

  reply	other threads:[~2026-02-26  0:27 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-25  5:46 [PATCH] kernel/trace/ftrace: introduce ftrace module notifier chensong_2000
2026-02-26  0:27 ` Steven Rostedt [this message]
2026-02-26 10:12   ` Song Chen
2026-02-26 10:51     ` Miroslav Benes
2026-02-26 17:30       ` Steven Rostedt
2026-02-27  1:34         ` Song Chen
2026-03-06  9:57           ` Petr Mladek
2026-04-12 14:10             ` Song Chen
2026-04-21 22:40               ` Song Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260225192724.48ed165e@fedora \
    --to=rostedt@goodmis.org \
    --cc=atomlin@atomlin.com \
    --cc=chensong_2000@189.cn \
    --cc=da.gomez@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-modules@vger.kernel.org \
    --cc=linux-trace-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mcgrof@kernel.org \
    --cc=mhiramat@kernel.org \
    --cc=petr.pavlu@suse.com \
    --cc=samitolvanen@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.