2009-03-13 09:15:30

by Zhao Lei

Subject: [PATCH] ftrace: Avoid double-free of dyn_ftrace

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 <[email protected]>
---
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);
} while_for_each_ftrace_rec();
mutex_unlock(&ftrace_lock);
--
1.5.5.3

2009-03-13 09:26:23

by Ingo Molnar

Subject: Re: [PATCH] ftrace: Avoid double-free of dyn_ftrace

* Zhaolei <[email protected]> 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 <[email protected]>
> ---
> 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?

Ingo

2009-03-13 09:32:07

by Zhao Lei

Subject: Re: [PATCH] ftrace: Avoid double-free of dyn_ftrace

* From: "Ingo Molnar" <[email protected]>
>
> * Zhaolei <[email protected]> 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 <[email protected]>
>> ---
>> 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.

B.R.
Zhaolei
>
> Ingo
>
>????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2009-03-13 09:34:32

by Zhao Lei

Subject: [tip:tracing/ftrace] ftrace: avoid double-free of dyn_ftrace

Commit-ID: b00f0b6dc1773b4c8f538503247da050b5ea631b
Gitweb: http://git.kernel.org/tip/b00f0b6dc1773b4c8f538503247da050b5ea631b
Author: Zhaolei <[email protected]>
AuthorDate: Fri, 13 Mar 2009 17:14:01 +0800
Commit: Ingo Molnar <[email protected]>
CommitDate: Fri, 13 Mar 2009 10:25:06 +0100

ftrace: avoid double-free of dyn_ftrace

If dyn_ftrace is freed before ftrace_release(), ftrace_release()
will free it again and make ftrace_free_records wrong.

Signed-off-by: Zhao Lei <[email protected]>
Cc: "Steven Rostedt ;" <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>


---
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);
} while_for_each_ftrace_rec();
mutex_unlock(&ftrace_lock);

2009-03-24 15:55:08

by Steven Rostedt

Subject: Re: [PATCH] ftrace: Avoid double-free of dyn_ftrace

On Fri, 13 Mar 2009, Zhaolei wrote:

> * From: "Ingo Molnar" <[email protected]>
> >
> > * Zhaolei <[email protected]> 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 <[email protected]>
> >> ---
> >> 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.

We can add a:

WARN_ON(rec->flags & FTRACE_FL_FREE);

in ftrace_free_rec if you are worried about this happening.

-- Steve

2009-03-25 02:28:45

by Zhao Lei

Subject: Re: [PATCH] ftrace: Avoid double-free of dyn_ftrace

* From: "Steven Rostedt" <[email protected]>
> On Fri, 13 Mar 2009, Zhaolei wrote:
>
>> * From: "Ingo Molnar" <[email protected]>
>> >
>> > * Zhaolei <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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
>
>
>????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2009-03-25 02:35:42

by Steven Rostedt

Subject: Re: [PATCH] ftrace: Avoid double-free of dyn_ftrace

On Wed, Mar 25, 2009 at 10:25:47AM +0800, Zhaolei wrote:
> >> >
> >> > 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.

Hi Zhaolei,

Great! Feel free to send another patch ;-)

Note, use FTRACE_WARN_ON() macro. This way it shuts down ftrace if it is hit and
helps to avoid further damage later.

-- Steve