[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 3 Mar 2009 17:36:59 +0530
From: Ananth N Mavinakayanahalli <ananth@...ibm.com>
To: Ingo Molnar <mingo@...e.hu>
Cc: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Masami Hiramatsu <mhiramat@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Nick Piggin <npiggin@...e.de>,
Steven Rostedt <rostedt@...dmis.org>,
Andi Kleen <andi@...stfloor.org>, linux-kernel@...r.kernel.org,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Frederic Weisbecker <fweisbec@...il.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Arjan van de Ven <arjan@...radead.org>,
Rusty Russell <rusty@...tcorp.com.au>,
"H. Peter Anvin" <hpa@...or.com>,
Steven Rostedt <srostedt@...hat.com>
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@...ymtl.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists