lkml.org 
[lkml]   [2009]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@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


    \
     
     \ /
      Last update: 2009-03-03 13:09    [W:0.032 / U:62.832 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site