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

[Steven Rostedt]

On Sun, 16 Mar 2025 21:21:42 +0900
"Masami Hiramatsu (Google)" <mhiramat@xxxxxxxxxx> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxx>
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
> ---
>  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?

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.

-- 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);
>