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

[Ananth N Mavinakayanahalli <ananth@in.ibm.com>]

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