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 10:27:50 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
Cc:	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)


* Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca> wrote:

> @@ -709,7 +711,8 @@ int __kprobes register_kprobe(struct kpr
>  
>  	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)

> @@ -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.

>  	} else {
>  		if (p->break_handler && !kprobe_gone(p))
> @@ -918,7 +924,6 @@ static int __kprobes pre_handler_kretpro
>  		}
>  
>  		arch_prepare_kretprobe(ri, regs);
> -

spurious (and wrong) newline removal.

>  		/* XXX(hch): why is there no hlist_move_head? */
>  		INIT_HLIST_NODE(&ri->hlist);
>  		kretprobe_table_lock(hash, &flags);
> @@ -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.

> @@ -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.

The dependencies are totally wrong. The text lock (a low level 
lock) should nest inside the kprobes mutex (which is the higher 
level lock).

Have you reviewed the locking dependencies when writing this 
patch, at all? That's one of the first things to do when adding 
a new lock.

	Ingo
--
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/