linux-kernel - Re: [PATCH] tracing/function-return-tracer: don't trace kfree while it frees the return stack

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]

Message-ID: <4929964B.2000200@gmail.com>
Date:	Sun, 23 Nov 2008 18:43:39 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Ingo Molnar <mingo@...e.hu>
CC:	Steven Rostedt <rostedt@...dmis.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tracing/function-return-tracer: don't trace kfree while
 it frees the return stack

Ingo Molnar a écrit :
> note that we also need to keep gcc from reordering things here (no 
> matter how unlikely in this particular case).
> 
> (also, small detail: we prefer a newline after variable definitions.)
> 
> Full commit attached below.
> 

I guessed that since tsk->ret_stack is referenced for the last time before the call to kfree,
the registers to memory flushing would be done before the call.

But you're right, I should prevent from possible compiler strange behaviours, thanks!




Ingo Molnar a écrit :
> * Ingo Molnar <mingo@...e.hu> wrote:
> 
>>  void ftrace_retfunc_exit_task(struct task_struct *t)
>>  {
>> -	kfree(t->ret_stack);
>> +	struct ftrace_ret_stack	*ret_stack = t->ret_stack;
>> +
>>  	t->ret_stack = NULL;
>> +	/* NULL must become visible to IRQs before we free it: */
>> +	barrier();
>> +
>> +	kfree(ret_stack);
>>  }
> 
> hm, this reminds me - this is the wrong place for the callback - the 
> call stack is freed too early.
> 
> instead of freeing it in do_exit() (like it's done right now), it 
> should be freed when the task struct and thread info is freed: in 
> kernel/fork.c:free_task() - okay?
> 
> The difference is minor but could allow more complete tracing: as 
> right now we'll skip the entries that get generated when we schedule 
> away from a dead task.
> 
> 	Ingo


That's right. Even if the noret code path will not be traced, we are loosing some traces.
That should be fixed with the patch below (didn't tested widely but seems good).
--
>From a7e143e42808f66a7128c7de322ebd6733b7cd17 Mon Sep 17 00:00:00 2001
From: Frederic Weisbecker <fweisbec@...il.com>
Date: Sun, 23 Nov 2008 18:30:01 +0100
Subject: [PATCH] tracing/function-return-tracer: Free the return stack on free_task()

Impact: avoid loosing some traces when a task is freed

do_exit() is not the last function called when a task finishes.
There are still some functions which are to be called such as free_task().
So we delay the freeing of the return stack to the last moment.

Signed-off-by: Frederic Weisbecker <fweisbec@...il.com>

diff --git a/kernel/exit.c b/kernel/exit.c
index c8334ed..cb24037 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -47,7 +47,6 @@
 #include <linux/task_io_accounting_ops.h>
 #include <linux/tracehook.h>
 #include <trace/sched.h>
-#include <linux/ftrace.h>
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
@@ -1125,7 +1124,6 @@ NORET_TYPE void do_exit(long code)
 	preempt_disable();
 	/* causes final put_task_struct in finish_task_switch(). */
 	tsk->state = TASK_DEAD;
-	ftrace_retfunc_exit_task(tsk);
 	schedule();
 	BUG();
 	/* Avoid "noreturn function does return".  */
diff --git a/kernel/fork.c b/kernel/fork.c
index 354d3f0..d183739 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -141,6 +141,7 @@ void free_task(struct task_struct *tsk)
 	prop_local_destroy_single(&tsk->dirties);
 	free_thread_info(tsk->stack);
 	rt_mutex_debug_task_free(tsk);
+	ftrace_retfunc_exit_task(tsk);
 	free_task_struct(tsk);
 }
 EXPORT_SYMBOL(free_task);
-- 
1.5.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/