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

[Chengming Zhou <zhouchengming@bytedance.com>]

在 2020/7/29 上午1:26, Steven Rostedt 写道:
> On Wed, 29 Jul 2020 00:59:33 +0800
> Chengming Zhou <zhouchengming@bytedance.com> wrote:
>
>
>>> 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...
> Nope, the two are disjoint.
>
>> Well, so I just leave this function unmodified.
>>
>> Just call that new function register_ftrace_ops_hash() from 
>> ftrace_release_mod.
> I would say to have anything that uses one of the
> ftrace_set_filter/notrace* functions, to also register itself for
> module removal.
>
> 	register_ftrace_mod_removal(struct ftrace_ops *ops);
>
> and then also have a unregister_ftrace_mod_removal() as there needs to
> be a way to remove it from the list before the ops gets freed.
>
> Then these functions would add the ops to a list, and this list is
> traversed to remove modules. The trace_arrays can register their ops
> too, so you can update that function.
>
> -- Steve


I thought about this, and at last I think you are right that it's better
the owner of the hash

do the ip clean work instead of the ftrace system...  so we should fix
kprobe's handling

of module going, not in ftrace.

Thanks!

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