Re: [PATCH] Text Edit Lock - kprobes architecture independent support (v2)

Ingo Molnar
2009-03-03T14:54:27Z

* Ananth N Mavinakayanahalli <ananth@in.ibm.com> wrote:

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

No, it's fine indeed, i got the locking order messed up ...
twice :-)

Ingo