Skip to contentSkip to conversation

ftrace: Clear hashes of stale ips of init memory

PATCH Superseded Joel Fernandes 1 participant 3 messages Oct 9, 2017

Subsystems (2)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 ftrace: Clear hashes of stale ips of init memory

Filters should be cleared of init functions during freeing of init memory when the ftrace dyn records are released. However in current code, the filters are left as is. This patch clears the hashes of the saved init functions when the init memory is freed. This fixes the following issue reproducible with the following sequence of commands for a test 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
cat /d/tracing/set_ftrace_filter
modprobe test
cat /d/tracing/trace
cat /d/tracing/set_ftrace_filter
rmmod test
cat /d/tracing/set_ftrace_filter
sleep 1
modprobe test
cat /d/tracing/trace
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 <[email protected]>
Cc Steven Rostedt <[email protected]>
---
 kernel/trace/ftrace.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)
kernel/trace/ftrace.c +71 -0
index 9e99bd55732e..ac7e68c5e2d2 100644
@@ -6067,6 +6067,65 @@ allocate_ftrace_mod_map(struct module *mod,
 }
 #endif /* CONFIG_MODULES */
 
+struct ftrace_init_func {
+	struct list_head list;
+	unsigned long ip;
+};
+
+/* Clear any init ips from hashes */
+static void
+clear_func_from_hash(struct ftrace_init_func *func, struct ftrace_hash *hash)
+{
+	struct ftrace_func_entry *entry;
+
+	if (ftrace_hash_empty(hash))
+		return;
+
+	printk("clear func %ps from hash\n", (void *)func->ip);
+
+	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_func_from_hashes(struct ftrace_init_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_func_from_hash(func, tr->ops->func_hash->filter_hash);
+		clear_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 add_to_clear_hash_list(struct list_head *clear_list,
+				   struct dyn_ftrace *rec)
+{
+	struct ftrace_init_func *func;
+
+	func = kmalloc(sizeof(*func), GFP_KERNEL);
+	if (!func) {
+		WARN_ONCE(1, "alloc failure, ftrace filter could be stale\n");
+		return;
+	}
+
+	func->ip = rec->ip;
+	list_add(&func->list, clear_list);
+}
+
 void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
 {
 	unsigned long start = (unsigned long)(start_ptr);
@@ -6076,8 +6135,12 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
 	struct dyn_ftrace *rec;
 	struct dyn_ftrace key;
 	struct ftrace_mod_map *mod_map = NULL;
+	struct ftrace_init_func *func, *func_next;
+	struct list_head clear_hash;
 	int order;
 
+	INIT_LIST_HEAD(&clear_hash);
+
 	key.ip = start;
 	key.flags = end;	/* overload flags, as it is unsigned long */
 
@@ -6102,6 +6165,9 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
 		if (!rec)
 			continue;
 
+		/* rec will be cleared from hashes after ftrace_lock unlock */
+		add_to_clear_hash_list(&clear_hash, rec);
+
 		if (mod_map)
 			save_ftrace_mod_rec(mod_map, rec);
 
@@ -6123,6 +6189,11 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
 		goto again;
 	}
 	mutex_unlock(&ftrace_lock);
+
+	list_for_each_entry_safe(func, func_next, &clear_hash, list) {
+		clear_func_from_hashes(func);
+		kfree(func);
+	}
 }
 
 void __init ftrace_free_init_mem(void)
Signature · 1 line
2.14.2.920.gcf0c67979c-goog
#1 lore raw

On Mon, Oct 9, 2017 at 11:53 AM, Joel Fernandes <[email protected]> wrote:
[..]

Quoted 28 lines
kernel/trace/ftrace.c +8 -0
+
 void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
 {
        unsigned long start = (unsigned long)(start_ptr);
@@ -6076,8 +6135,12 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
        struct dyn_ftrace *rec;
        struct dyn_ftrace key;
        struct ftrace_mod_map *mod_map = NULL;
+       struct ftrace_init_func *func, *func_next;
+       struct list_head clear_hash;
        int order;
 
+       INIT_LIST_HEAD(&clear_hash);
+
        key.ip = start;
        key.flags = end;        /* overload flags, as it is unsigned long */
 
@@ -6102,6 +6165,9 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
                if (!rec)
                        continue;
 
+               /* rec will be cleared from hashes after ftrace_lock unlock */
+               add_to_clear_hash_list(&clear_hash, rec);
+
                if (mod_map)
                        save_ftrace_mod_rec(mod_map, rec);
 

kernel/trace/ftrace.c void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr) · line 6193

Quoted 5 lines
kernel/trace/ftrace.c +2 -0
@@ -6123,6 +6189,11 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
                goto again;
        }
        mutex_unlock(&ftrace_lock);
+
+       list_for_each_entry_safe(func, func_next, &clear_hash, list) {

I think I screwed this list_for_each_entry up! Please ignore this revision, I'll send another patch revision shortly.

Regards,
Joel

#2 lore raw
Quoted 29 lines · Joel Fernandes

On Mon, Oct 9, 2017 at 11:53 AM, Joel Fernandes <[email protected]> wrote:
[..]

Quoted 27 lines
kernel/trace/ftrace.c +8 -0
+
 void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
 {
        unsigned long start = (unsigned long)(start_ptr);
@@ -6076,8 +6135,12 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
        struct dyn_ftrace *rec;
        struct dyn_ftrace key;
        struct ftrace_mod_map *mod_map = NULL;
+       struct ftrace_init_func *func, *func_next;
+       struct list_head clear_hash;
        int order;
 
+       INIT_LIST_HEAD(&clear_hash);
+
        key.ip = start;
        key.flags = end;        /* overload flags, as it is unsigned long */
 
@@ -6102,6 +6165,9 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
                if (!rec)
                        continue;
 
+               /* rec will be cleared from hashes after ftrace_lock unlock */
+               add_to_clear_hash_list(&clear_hash, rec);
+
                if (mod_map)
                        save_ftrace_mod_rec(mod_map, rec);
 
Quoted 9 lines

kernel/trace/ftrace.c void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr) · line 6193

Quoted 6 lines
kernel/trace/ftrace.c +2 -0
@@ -6123,6 +6189,11 @@ void ftrace_free_mem(struct module *mod, void *start_ptr, void *end_ptr)
                goto again;
        }
        mutex_unlock(&ftrace_lock);
+
+       list_for_each_entry_safe(func, func_next, &clear_hash, list) {

I think I screwed this list_for_each_entry up! Please ignore this revision, I'll send another patch revision shortly.

Actually, its correct. I got confused with how list_for_each_entry_safe's terminating condition is defined:

&pos->member != (head)

I thought head itself had to be an entry's member, but here the pointer offset is calculated and will take care of the termination so its fine.

Sorry about the noise!

thanks,

- Joel