Re: [PATCH 2/3] ftrace: export ftrace_free_filter() to modules

[Steven Rostedt]

On Thu,  3 Nov 2022 17:09:06 +0000
Mark Rutland <mark.rutland@xxxxxxx> wrote:

FYI, the subject should start with a capital letter.

  ftrace: Export ftrace_free_filter() to modules


> Setting filters on an ftrace ops results in some memory being allocated
> for the filter hashes, which must be freed before the ops can be freed.
> This can be done by removing every individual element of the hash by
> calling ftrace_set_filter_ip() or ftrace_set_filter_ips() with `remove`
> set, but this is somewhat error prone as it's easy to forget to remove
> an element.
> 
> Make it easier to clean this up by exporting ftrace_free_filter(), which
> can be used to clean up all of the filter hashes after an ftrace_ops has
> been unregistered.
> 
> Using this, fix the ftrace-direct* samples to free hashes prior to being
> unloaded. All other code either removes individual filters explicitly or
> is built-in and already calls ftrace_free_filter().
> 
> Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Florent Revest <revest@xxxxxxxxxxxx>
> Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> ---
>  kernel/trace/ftrace.c                       | 23 ++++++++++++++++++++-
>  samples/ftrace/ftrace-direct-multi-modify.c |  1 +
>  samples/ftrace/ftrace-direct-multi.c        |  1 +
>  3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index fbf2543111c05..1ecdda1df6d47 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1248,12 +1248,17 @@ static void free_ftrace_hash_rcu(struct ftrace_hash *hash)
>  	call_rcu(&hash->rcu, __free_ftrace_hash_rcu);
>  }
>  
> +/**
> + * ftrace_free_filter - remove all filters for an ftrace_ops
> + * @ops - the ops to remove the filters from
> + */
>  void ftrace_free_filter(struct ftrace_ops *ops)
>  {
>  	ftrace_ops_init(ops);
>  	free_ftrace_hash(ops->func_hash->filter_hash);
>  	free_ftrace_hash(ops->func_hash->notrace_hash);
>  }
> +EXPORT_SYMBOL_GPL(ftrace_free_filter);
>  
>  static struct ftrace_hash *alloc_ftrace_hash(int size_bits)
>  {
> @@ -5837,6 +5842,10 @@ EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi);
>   *
>   * Filters denote which functions should be enabled when tracing is enabled
>   * If @ip is NULL, it fails to update filter.
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by remvoing eached filtered addr or by using
> + * ftrace_free_filter(@ops).
>   */
>  int ftrace_set_filter_ip(struct ftrace_ops *ops, unsigned long ip,
>  			 int remove, int reset)
> @@ -5856,7 +5865,11 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter_ip);
>   *
>   * Filters denote which functions should be enabled when tracing is enabled
>   * If @ips array or any ip specified within is NULL , it fails to update filter.
> - */
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by remvoing eached filtered addr or by using
> + * ftrace_free_filter(@ops).
> +*/
>  int ftrace_set_filter_ips(struct ftrace_ops *ops, unsigned long *ips,
>  			  unsigned int cnt, int remove, int reset)
>  {
> @@ -5898,6 +5911,10 @@ ftrace_set_regex(struct ftrace_ops *ops, unsigned char *buf, int len,
>   *
>   * Filters denote which functions should be enabled when tracing is enabled.
>   * If @buf is NULL and reset is set, all functions will be enabled for tracing.
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by remvoing eached filtered addr or by using
> + * ftrace_free_filter(@ops).
>   */
>  int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
>  		       int len, int reset)
> @@ -5917,6 +5934,10 @@ EXPORT_SYMBOL_GPL(ftrace_set_filter);
>   * Notrace Filters denote which functions should not be enabled when tracing
>   * is enabled. If @buf is NULL and reset is set, all functions will be enabled
>   * for tracing.
> + *
> + * This can allocate memory which must be freed before @ops can be freed,
> + * either by remvoing eached filtered addr or by using

I love how you cut and pasted two typos four times. ;-)

-- Steve

> + * ftrace_free_filter(@ops).
>   */
>  int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
>  			int len, int reset)
> diff --git a/samples/ftrace/ftrace-direct-multi-modify.c b/samples/ftrace/ftrace-direct-multi-modify.c
> index d52370cad0b6e..a825dbd2c9cfd 100644
> --- a/samples/ftrace/ftrace-direct-multi-modify.c
> +++ b/samples/ftrace/ftrace-direct-multi-modify.c
> @@ -152,6 +152,7 @@ static void __exit ftrace_direct_multi_exit(void)
>  {
>  	kthread_stop(simple_tsk);
>  	unregister_ftrace_direct_multi(&direct, my_tramp);
> +	ftrace_free_filter(&direct);
>  }
>  
>  module_init(ftrace_direct_multi_init);
> diff --git a/samples/ftrace/ftrace-direct-multi.c b/samples/ftrace/ftrace-direct-multi.c
> index ec1088922517d..d955a26506053 100644
> --- a/samples/ftrace/ftrace-direct-multi.c
> +++ b/samples/ftrace/ftrace-direct-multi.c
> @@ -79,6 +79,7 @@ static int __init ftrace_direct_multi_init(void)
>  static void __exit ftrace_direct_multi_exit(void)
>  {
>  	unregister_ftrace_direct_multi(&direct, (unsigned long) my_tramp);
> +	ftrace_free_filter(&direct);
>  }
>  
>  module_init(ftrace_direct_multi_init);