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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 3 Mar 2009 09:28:12 -0500
From:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
To:	Ananth N Mavinakayanahalli <ananth@...ibm.com>
Cc:	Ingo Molnar <mingo@...e.hu>,
	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)

* Ananth N Mavinakayanahalli (ananth@...ibm.com) wrote:
> 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).
> 

Yes, I even moved all kprobe_mutexes out of arch_arm_kprobe/arch_arm_kprobe
a while ago in preparation for this patch. :) I can repost without the
white space modifications.

Mathieu

> Ananth

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
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/