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

[Chengming Zhou <zhouchengming@bytedance.com>]

在 2020/7/28 下午8:53, Steven Rostedt 写道:
> On Tue, 28 Jul 2020 18:27:19 +0800
> Chengming Zhou <zhouchengming@bytedance.com> 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.

Agreed!

But ftrace is a very core and independent mechanism of kernel, it's hard to

make sure any user of ftrace, like kprobe, livepatch will handle module 
going correctly.

At least for now, kprobe does not handle that correctly...

So I think it's safer to fix it too in ftrace : )

>
> 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.

Thanks for the suggestion, so in this new function, all ftrace_ops 
func_hash on

ftrace_ops_list will get updated on module removal.

>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>> ---
>>   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.

I thought all ftrace_ops has non empty func_hash are on the ftrace 
global list...

Well, so I just leave this function unmodified.

Just call that new function register_ftrace_ops_hash() from 
ftrace_release_mod.

Thanks!

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