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

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

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>