Hi Steven/Ingo,
These patches address the problem of kprobe registration interfering with
dynamic ftrace. A kprobe registered on an mcount call-site can modify that
address multiple times during its duration of probing. At the same time,
ftrace may simultaneously modify these locations. This could be fatal
especially if ftrace modifies the call-site during a kprobe registration
on it (between arch_prepare_kprobe and arch_arm_kprobe in __register_kprobe
to be more precise). So as a "fix", I've disabled any updates on the mcount
call-site while it's being kprobe'd.
--
Regards,
Abhishek Sagar
* Abhishek Sagar <[email protected]> wrote:
> Hi Steven/Ingo,
>
> These patches address the problem of kprobe registration interfering
> with dynamic ftrace. A kprobe registered on an mcount call-site can
> modify that address multiple times during its duration of probing. At
> the same time, ftrace may simultaneously modify these locations. This
> could be fatal especially if ftrace modifies the call-site during a
> kprobe registration on it (between arch_prepare_kprobe and
> arch_arm_kprobe in __register_kprobe to be more precise). So as a
> "fix", I've disabled any updates on the mcount call-site while it's
> being kprobe'd.
applied to tip/tracing/ftrace, thanks Abhishek.
note that i had to do a number of fixups to themerge, and there was one
chunk which did not merge cleanly and i left it out for now:
@@ -435,8 +502,12 @@ static void ftrace_replace_code(int enable)
continue;
/* ignore updates to this record's mcount site */
- if (get_kprobe((void *)rec->ip))
+ if (get_kprobe((void *)rec->ip)) {
+ freeze_record(rec);
continue;
+ } else {
+ unfreeze_record(rec);
+ }
please send a delta patch against tip/tracing/ftrace to get that kprobes
fix. You can pick up -tip via:
http://people.redhat.com/mingo/tip.git/README
Ingo
Hi Ingo,
On Tue, Jun 24, 2008 at 1:45 AM, Ingo Molnar <[email protected]> wrote:
> note that i had to do a number of fixups to themerge, and there was one
> chunk which did not merge cleanly and i left it out for now:
>
> @@ -435,8 +502,12 @@ static void ftrace_replace_code(int enable)
> continue;
>
> /* ignore updates to this record's mcount site */
> - if (get_kprobe((void *)rec->ip))
> + if (get_kprobe((void *)rec->ip)) {
> + freeze_record(rec);
> continue;
> + } else {
> + unfreeze_record(rec);
> + }
I wonder why it conflicted during the merge. I notice that patch 4/4
has been applied before
3/4. Can you try reversing the order and see if it works? Else I'll redo them.
--
Thanks,
Abhishek Sagar
* Abhishek Sagar <[email protected]> wrote:
> Hi Ingo,
>
> On Tue, Jun 24, 2008 at 1:45 AM, Ingo Molnar <[email protected]> wrote:
> > note that i had to do a number of fixups to themerge, and there was one
> > chunk which did not merge cleanly and i left it out for now:
> >
> > @@ -435,8 +502,12 @@ static void ftrace_replace_code(int enable)
> > continue;
> >
> > /* ignore updates to this record's mcount site */
> > - if (get_kprobe((void *)rec->ip))
> > + if (get_kprobe((void *)rec->ip)) {
> > + freeze_record(rec);
> > continue;
> > + } else {
> > + unfreeze_record(rec);
> > + }
>
> I wonder why it conflicted during the merge. I notice that patch 4/4
> has been applied before 3/4. Can you try reversing the order and see
> if it works? Else I'll redo them.
best would be a delta patch against tip/master to fix up whatever is
still missing.
Ingo
Ingo Molnar wrote:
> best would be a delta patch against tip/master to fix up whatever is
> still missing.
Here is the merge fixup for tip/master (and tip/tracing/ftrace).
Signed-off-by: Abhishek Sagar <[email protected]>
---
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 85e8413..0f271c4 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -502,8 +502,12 @@ static void ftrace_replace_code(int enable)
continue;
/* ignore updates to this record's mcount site */
- if (get_kprobe((void *)rec->ip))
+ if (get_kprobe((void *)rec->ip)) {
+ freeze_record(rec);
continue;
+ } else {
+ unfreeze_record(rec);
+ }
failed = __ftrace_replace_code(rec, old, new, enable);
if (failed && (rec->flags & FTRACE_FL_CONVERTED)) {
@@ -740,7 +744,10 @@ static int __ftrace_update_code(void *ignore)
ftrace_del_hash(p);
INIT_HLIST_NODE(&p->node);
hlist_add_head(&p->node, &temp_list);
+ freeze_record(p);
continue;
+ } else {
+ unfreeze_record(p);
}
/* convert record (i.e, patch mcount-call with NOP) */
* Abhishek Sagar <[email protected]> wrote:
> Ingo Molnar wrote:
> > best would be a delta patch against tip/master to fix up whatever is
> > still missing.
>
> Here is the merge fixup for tip/master (and tip/tracing/ftrace).
>
> Signed-off-by: Abhishek Sagar <[email protected]>
applied to tip/tracing/ftrace - thanks Abhishek!
Ingo