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

Ingo Molnar
2009-03-03T09:28:57Z

* Mathieu Desnoyers <mathieu.desnoyers@polymtl.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