Skip to contentSkip to conversation

ftrace: prevent ftrace modifications while being kprobe'd

PATCH Applied Abhishek Sagar 2 participants 6 messages Jun 21, 2008 – Jul 3, 2008

Subsystems (2)
Tracing ftrace
lore
Share & tools
lore

Abhishek Sagar

author
#0 lore raw

cover ftrace: prevent ftrace modifications while being kprobe'd

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

Applied
#1 lore raw
Quoted 11 lines · Abhishek Sagar

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

author
#2 lore raw

Hi Ingo,

Quoted 4 lines · Ingo Molnar

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:

Review static void ftrace_replace_code(int enable) · line 510

Quoted 10 lines
(quoted diff) +5 -1
@@ -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

#3 lore raw
Quoted 9 lines · Abhishek Sagar

Hi Ingo,

Quoted 6 lines · Ingo Molnar

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:

(quoted diff) +0 -0
@@ -435,8 +502,12 @@ static void ftrace_replace_code(int enable)
                               continue;
Quoted 12 lines

Review static void ftrace_replace_code(int enable) · line 510

Quoted 8 lines
(quoted diff) +5 -1
                       /* 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

author
#4 lore raw
Quoted 2 lines · Ingo Molnar

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]>

---

kernel/trace/ftrace.c +8 -1
index 85e8413..0f271c4 100644
@@ -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) */
Applied
#5 lore raw
Quoted 7 lines · Abhishek Sagar
Quoted 2 lines · Ingo Molnar

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