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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 31 Jul 2020 12:00:11 +0800
From:   Chengming Zhou <zhouchengming@...edance.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     mingo@...hat.com, linux-kernel@...r.kernel.org,
        songmuchun@...edance.com
Subject: Re: [External] Re: [PATCH 1/2] ftrace: clear module from hash of all
 ftrace ops


在 2020/7/29 上午1:26, Steven Rostedt 写道:
> On Wed, 29 Jul 2020 00:59:33 +0800
> Chengming Zhou <zhouchengming@...edance.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)