Messages in this thread Patch in this message | | | From | "Zhaolei" <> | Subject | Re: [PATCH] ftrace: Avoid double-free of dyn_ftrace | Date | Wed, 25 Mar 2009 10:25:47 +0800 |
| |
* From: "Steven Rostedt" <rostedt@goodmis.org> > On Fri, 13 Mar 2009, Zhaolei wrote: > >> * From: "Ingo Molnar" <mingo@elte.hu> >> > >> > * Zhaolei <zhaolei@cn.fujitsu.com> wrote: >> > >> >> If dyn_ftrace is free before ftrace_release(), >> >> ftrace_release() will free it again and make >> >> ftrace_free_records wrong. >> >> >> >> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com> >> >> --- >> >> kernel/trace/ftrace.c | 3 ++- >> >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> >> >> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c >> >> index d33d306..26c45aa 100644 >> >> --- a/kernel/trace/ftrace.c >> >> +++ b/kernel/trace/ftrace.c >> >> @@ -356,7 +356,8 @@ void ftrace_release(void *start, unsigned long size) >> >> >> >> mutex_lock(&ftrace_lock); >> >> do_for_each_ftrace_rec(pg, rec) { >> >> - if ((rec->ip >= s) && (rec->ip < e)) >> >> + if ((rec->ip >= s) && (rec->ip < e) && >> >> + !(rec->flags & FTRACE_FL_FREE)) >> >> ftrace_free_rec(rec); >> > >> > Applied to tip:tracing/ftrace, thanks! >> > >> > I'm wondering, did you trigger this in practice (if yes, how?), >> > or have you found it via code review? >> Hello, Ingo >> >> It is found via code review. > > Hmm, could you explain this more. I'm thinking that this scenario should > not happen, and if it does, it should probably be a bug. > > Because when we call ftrace_free_rec we change the rec->ip to point to the > next record in the chain. Something is very wrong if rec->ip >= s && > rec->ip < e and the record is already free. Hello, Steven
Thanks for your comment. I got your meaning, and I agree that if rec->ip >= s && rec->ip < e, this record is not freed. But IMHO, "if rec->ip >= s && rec->ip < e" is used to select records in the module, and function of ignore "freed record" is only its side-effect. So, add a special judgement to avoid "freed record" is not a bad idea. And I also agree your suggestion of add a WARN_ON, because this should not happened.
B.R. Zhaolei
> > We can add a: > > WARN_ON(rec->flags & FTRACE_FL_FREE); > > in ftrace_free_rec if you are worried about this happening. > > -- Steve > > > | |