Re: [PATCH 1/2] ftrace: clear module from hash of all ftrace ops

[Steven Rostedt]

On Tue, 28 Jul 2020 18:27:19 +0800
Chengming Zhou <zhouchengming@xxxxxxxxxxxxx> wrote:

> We should clear module from hash of all ops on ftrace_ops_list when
> module going, or the ops including these modules will be matched
> wrongly by new module loaded later.

This is really up to the owner of the hash and not the registered
system.

If we want, we could register some kind of callback table for all
ftrace_ops to have this updated, but the current code is incorrect.

Like:

 register_ftrace_ops_hash()

Where the hash will get updated on module removal.

> 
> Signed-off-by: Chengming Zhou <zhouchengming@xxxxxxxxxxxxx>
> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> ---
>  kernel/trace/ftrace.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 1903b80db6eb..fca01a168ae5 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6223,18 +6223,20 @@ clear_mod_from_hash(struct ftrace_page *pg, struct ftrace_hash *hash)
>  /* Clear any records from hashs */
>  static void clear_mod_from_hashes(struct ftrace_page *pg)
>  {
> -	struct trace_array *tr;
> +	struct ftrace_ops *op;
>  
> -	mutex_lock(&trace_types_lock);
> -	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> -		if (!tr->ops || !tr->ops->func_hash)

The tr->ops hashes are persistent without being registered. They match
what's in set_ftrace_filter and similar files.

Your patch just introduced a bug, because those hashes now would not
get updated if the ops were not registered.

i.e.

 # echo some_module_function > set_ftrace_filter
 # rmmod module_with_that_function
 # insmod module_with_same_address_of_function
 # echo function > current_tracer

Now the tr->ops->hash would still have the function of the original
module.


Either have all owners of ftrace_ops handle this case, or add a helper
function to handle it for them. But using ftarce_ops_list is the wrong
place to do it.

-- Steve


> +	mutex_lock(&ftrace_lock);
> +
> +	do_for_each_ftrace_op(op, ftrace_ops_list) {
> +		if (!op->func_hash)
>  			continue;
> -		mutex_lock(&tr->ops->func_hash->regex_lock);
> -		clear_mod_from_hash(pg, tr->ops->func_hash->filter_hash);
> -		clear_mod_from_hash(pg, tr->ops->func_hash->notrace_hash);
> -		mutex_unlock(&tr->ops->func_hash->regex_lock);
> -	}
> -	mutex_unlock(&trace_types_lock);
> +		mutex_lock(&op->func_hash->regex_lock);
> +		clear_mod_from_hash(pg, op->func_hash->filter_hash);
> +		clear_mod_from_hash(pg, op->func_hash->notrace_hash);
> +		mutex_unlock(&op->func_hash->regex_lock);
> +	} while_for_each_ftrace_op(op);
> +
> +	mutex_unlock(&ftrace_lock);
>  }
>  
>  static void ftrace_free_mod_map(struct rcu_head *rcu)