[PATCH RFC] ftrace: Clear hashes of saved init functions

[PATCH RFC] ftrace: Clear hashes of saved init functions

[Joel Fernandes]

Filters are supposed to be saved for init functions, however module init memory
is freed after module init which also frees the ftrace dyn records. However, the
filters are still left as is, this patch uses the mod_map infrastructure
added by Steven to clear the hashes of the saved init functions when the
module is unloaded.

This fixes the following sequence of commands for a module:
================================================
void bar(void)
{
    printk(KERN_INFO "bar!\n");
}

void foo(void)
{
    printk(KERN_INFO "foo!\n");
    bar();
}

static int __init hello_init(void)
{
    printk(KERN_INFO "Hello world!\n");
    foo();
    return 0;
}

static void __exit hello_cleanup(void)
{
    printk(KERN_INFO "Cleaning up module.\n");
}

module_init(hello_init);
module_exit(hello_cleanup);
================================================

Commands:
echo '*:mod:test' > /d/tracing/set_ftrace_filter
echo function > /d/tracing/current_tracer
modprobe test
rmmod test
sleep 1
modprobe test
cat /d/tracing/set_ftrace_filter

Behavior without patch: Init function is still in the filter
Expected behavior: Shouldn't have any of the filters set

Cc: Jessica Yu <jeyu@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Joel Fernandes <joelaf@google.com>
---
 kernel/trace/ftrace.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 9e99bd55732e..e3f8372fb053 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5711,6 +5711,42 @@ static int referenced_filters(struct dyn_ftrace *rec)
 	return cnt;
 }
 
+static void
+clear_mod_func_from_hash(struct ftrace_mod_func *func, struct ftrace_hash *hash)
+{
+	struct ftrace_func_entry *entry;
+
+	if (ftrace_hash_empty(hash))
+		return;
+
+	entry = __ftrace_lookup_ip(hash, func->ip);
+
+	/*
+	 * Do not allow this rec to match again.
+	 * Yeah, it may waste some memory, but will be removed
+	 * if/when the hash is modified again.
+	 */
+	if (entry)
+		entry->ip = 0;
+}
+
+static void
+clear_mod_func_from_hashes(struct ftrace_mod_func *func)
+{
+	struct trace_array *tr;
+
+	mutex_lock(&trace_types_lock);
+	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
+		if (!tr->ops || !tr->ops->func_hash)
+			continue;
+		mutex_lock(&tr->ops->func_hash->regex_lock);
+		clear_mod_func_from_hash(func, tr->ops->func_hash->filter_hash);
+		clear_mod_func_from_hash(func, tr->ops->func_hash->notrace_hash);
+		mutex_unlock(&tr->ops->func_hash->regex_lock);
+	}
+	mutex_unlock(&trace_types_lock);
+}
+
 static void
 clear_mod_from_hash(struct ftrace_page *pg, struct ftrace_hash *hash)
 {
@@ -5771,12 +5807,24 @@ void ftrace_release_mod(struct module *mod)
 {
 	struct ftrace_mod_map *mod_map;
 	struct ftrace_mod_map *n;
+	struct ftrace_mod_func *mod_func;
 	struct dyn_ftrace *rec;
 	struct ftrace_page **last_pg;
 	struct ftrace_page *tmp_page = NULL;
 	struct ftrace_page *pg;
 	int order;
 
+	/* mod_map is freed via call_rcu_sched() */
+	preempt_disable();
+	list_for_each_entry_rcu(mod_map, &ftrace_mod_maps, list) {
+		if (mod_map->mod != mod)
+			continue;
+		list_for_each_entry_rcu(mod_func, &mod_map->funcs, list) {
+			clear_mod_func_from_hashes(mod_func);
+		}
+	}
+	preempt_enable();
+
 	mutex_lock(&ftrace_lock);
 
 	if (ftrace_disabled)
-- 
2.14.2.920.gcf0c67979c-goog

Re: [PATCH RFC] ftrace: Clear hashes of saved init functions

[Steven Rostedt]

On Sun,  8 Oct 2017 14:33:04 -0700
Joel Fernandes <joelaf@google.com> wrote:

>
>  static void
>  clear_mod_from_hash(struct ftrace_page *pg, struct ftrace_hash *hash)
>  {
> @@ -5771,12 +5807,24 @@ void ftrace_release_mod(struct module *mod)

This is too late. This should be done by ftrace_free_mem().

-- Steve

>  {
>  	struct ftrace_mod_map *mod_map;
>  	struct ftrace_mod_map *n;
> +	struct ftrace_mod_func *mod_func;
>  	struct dyn_ftrace *rec;
>  	struct ftrace_page **last_pg;
>  	struct ftrace_page *tmp_page = NULL;
>  	struct ftrace_page *pg;
>  	int order;
>  
> +	/* mod_map is freed via call_rcu_sched() */
> +	preempt_disable();
> +	list_for_each_entry_rcu(mod_map, &ftrace_mod_maps, list) {
> +		if (mod_map->mod != mod)
> +			continue;
> +		list_for_each_entry_rcu(mod_func, &mod_map->funcs, list) {
> +			clear_mod_func_from_hashes(mod_func);
> +		}
> +	}
> +	preempt_enable();
> +
>  	mutex_lock(&ftrace_lock);
>  
>  	if (ftrace_disabled)

Re: [PATCH RFC] ftrace: Clear hashes of saved init functions

[Joel Fernandes]

Hi Steve,

On Mon, Oct 9, 2017 at 8:09 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Sun,  8 Oct 2017 14:33:04 -0700
> Joel Fernandes <joelaf@google.com> wrote:
>
>>
>>  static void
>>  clear_mod_from_hash(struct ftrace_page *pg, struct ftrace_hash *hash)
>>  {
>> @@ -5771,12 +5807,24 @@ void ftrace_release_mod(struct module *mod)
>
> This is too late. This should be done by ftrace_free_mem().
>

Ok, I agree with you. Initially I started to do it this way but the
locking made it more complicated. I'll work on a patch that does it at
free_mem time and send it out today.

thanks,

- Joel

Re: [PATCH RFC] ftrace: Clear hashes of saved init functions

[Steven Rostedt]

On Mon, 9 Oct 2017 08:33:51 -0700
Joel Fernandes <joelaf@google.com> wrote:

> Hi Steve,
> 
> On Mon, Oct 9, 2017 at 8:09 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Sun,  8 Oct 2017 14:33:04 -0700
> > Joel Fernandes <joelaf@google.com> wrote:
> >  
> >>
> >>  static void
> >>  clear_mod_from_hash(struct ftrace_page *pg, struct ftrace_hash *hash)
> >>  {
> >> @@ -5771,12 +5807,24 @@ void ftrace_release_mod(struct module *mod)  
> >
> > This is too late. This should be done by ftrace_free_mem().
> >  
> 
> Ok, I agree with you. Initially I started to do it this way but the
> locking made it more complicated. I'll work on a patch that does it at
> free_mem time and send it out today.
> 

Yeah, I noticed that too. You may have to allocate pages to store what
is getting cleared. Of course that then begs the question, what do we
do when we fail to allocate? Perhaps just WARN_ONCE() and then let them
leak?

-- Steve

Re: [PATCH RFC] ftrace: Clear hashes of saved init functions

[Joel Fernandes]

On Mon, Oct 9, 2017 at 9:21 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 9 Oct 2017 08:33:51 -0700
> Joel Fernandes <joelaf@google.com> wrote:
>>
>> On Mon, Oct 9, 2017 at 8:09 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > On Sun,  8 Oct 2017 14:33:04 -0700
>> > Joel Fernandes <joelaf@google.com> wrote:
>> >
>> >>
>> >>  static void
>> >>  clear_mod_from_hash(struct ftrace_page *pg, struct ftrace_hash *hash)
>> >>  {
>> >> @@ -5771,12 +5807,24 @@ void ftrace_release_mod(struct module *mod)
>> >
>> > This is too late. This should be done by ftrace_free_mem().
>> >
>>
>> Ok, I agree with you. Initially I started to do it this way but the
>> locking made it more complicated. I'll work on a patch that does it at
>> free_mem time and send it out today.
>>
>
> Yeah, I noticed that too. You may have to allocate pages to store what
> is getting cleared.

Yes, this is what I am doing in the next revision.

> Of course that then begs the question, what do we
> do when we fail to allocate? Perhaps just WARN_ONCE() and then let them
> leak?

Yes in case of allocation failure, WARN_ONCE would seem reasonable to
me for this case.

thanks,

- Joel