Messages in this thread |  | | | Date | Tue, 3 Mar 2009 17:36:59 +0530 | | From | Ananth N Mavinakayanahalli <> | | Subject | Re: [PATCH] Text Edit Lock - kprobes architecture independent support (v2) |
| |
On Tue, Mar 03, 2009 at 10:27:50AM +0100, Ingo Molnar wrote: > > * Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote: > > > @@ -709,7 +711,8 @@ int __kprobes register_kprobe(struct kpr
Hi Ingo,
> > if (kprobe_enabled) > > arch_arm_kprobe(p); > > hm, it's cleaner now, but there's serious locking dependency > problems visible in the patch: > > > - > > +out_unlock_text: > > + mutex_unlock(&text_mutex); > > out: > > mutex_unlock(&kprobe_mutex); > > this one creates a (text_mutex -> kprobe_mutex) dependency. > (also you removed a newline spuriously - dont do that)
That is a mutex_unlock :-) ...
> > @@ -746,8 +749,11 @@ valid_p: > > * enabled and not gone - otherwise, the breakpoint would > > * already have been removed. We save on flushing icache. > > */ > > - if (kprobe_enabled && !kprobe_gone(old_p)) > > + if (kprobe_enabled && !kprobe_gone(old_p)) { > > + mutex_lock(&text_mutex); > > arch_disarm_kprobe(p); > > + mutex_unlock(&text_mutex); > > + } > > hlist_del_rcu(&old_p->hlist); > > (kprobe_mutex -> text_mutex) dependency. AB-BA deadlock.
At this time the kprobe_mutex is already held.
...
> > @@ -1280,12 +1285,14 @@ static void __kprobes enable_all_kprobes > > if (kprobe_enabled) > > goto already_enabled; > > > > + mutex_lock(&text_mutex); > > for (i = 0; i < KPROBE_TABLE_SIZE; i++) { > > head = &kprobe_table[i]; > > hlist_for_each_entry_rcu(p, node, head, hlist) > > if (!kprobe_gone(p)) > > arch_arm_kprobe(p); > > } > > + mutex_unlock(&text_mutex); > > this one creates a (kprobe_mutex -> text_mutex) dependency > again.
kprobe_mutex his held here too...
> > @@ -1310,6 +1317,7 @@ static void __kprobes disable_all_kprobe > > > > kprobe_enabled = false; > > printk(KERN_INFO "Kprobes globally disabled\n"); > > + mutex_lock(&text_mutex); > > for (i = 0; i < KPROBE_TABLE_SIZE; i++) { > > head = &kprobe_table[i]; > > hlist_for_each_entry_rcu(p, node, head, hlist) { > > @@ -1317,7 +1325,7 @@ static void __kprobes disable_all_kprobe > > arch_disarm_kprobe(p); > > } > > } > > - > > + mutex_unlock(&text_mutex); > > mutex_unlock(&kprobe_mutex); > > And this one in the reverse direction again.
Unlock again :-)
> The dependencies are totally wrong. The text lock (a low level > lock) should nest inside the kprobes mutex (which is the higher > level lock).
From what I see, Mathieu has done just that and has gotten the order right in all cases. Or maybe I am missing something?
(I recall having tested this patch with LOCKDEP turned on and it din't throw any errors).
Ananth
|  |