lkml.org 
[lkml]   [2012]   [Apr]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [RFC 0/6] uprobes: kill uprobes_srcu/uprobe_srcu_id
    From
    Date
    On Sun, 2012-04-15 at 21:53 +0200, Oleg Nesterov wrote:
    > On 04/15, Peter Zijlstra wrote:
    > >
    > > On Sat, 2012-04-14 at 22:52 +0200, Oleg Nesterov wrote:
    > > > > > - can it work or I missed something "in general" ?
    > > > >
    > > > > So we insert in the rb-tree before we take mmap_sem, this means we can
    > > > > hit a non-uprobe int3 and still find a uprobe there, no?
    > > >
    > > > Yes, but unless I miss something this is "off-topic", this
    > > > can happen with or without these changes. If find_uprobe()
    > > > succeeds we assume that this bp was inserted by uprobe.
    > >
    > > OK, but then I completely missed what the point of that
    > > down_write() stuff is..
    >
    > To ensure handle_swbp() can't race with unregister + register
    > and send the wrong SIGTRAP.
    >
    > handle_swbp() roughly does under down_read(mmap_sem)
    >
    >
    > if (find_uprobe(vaddr))
    > process_uprobe();
    > else
    > if (is_swbp_at_addr_fast(vaddr)) // non-uprobe int3
    > send_sig(SIGTRAP);
    > else
    > restart_insn(vaddr); // raced with unregister
    >
    >
    > note that is_swbp_at_addr_fast() is used (currently) to detect
    > the race with upbrobe_unregister() and that is why we can remove
    > uprobes_srcu.
    >
    > But if find_uprobe() fails, there is a window before
    > is_swbp_at_addr_fast() reads the memory. Suppose that the next
    > uprobe_register() inserts the new uprobe at the same address.
    > In this case the task will be wrongly killed.

    OK, still not seeing how your proposal could work.. consider the below
    patch comment, I'm not seeing how is_swbp_at_addr_fast() deals with an
    in-progress INT3 while we remove the probe.

    By ensuring the non-race with reg/unreg it will either find the uprobe
    (no problem) or not find it and not see a breakpoint instruction either,
    even though the pending breakpoint was generated by a uprobe (which is
    now gone), causing a false positive SIGTRAP.

    Or am I still not getting it?

    ---
    kernel/events/uprobes.c | 53 +++++++++++++++++++++++++++++++++++++++-------
    1 files changed, 45 insertions(+), 8 deletions(-)

    diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
    index 29e881b..67818ff 100644
    --- a/kernel/events/uprobes.c
    +++ b/kernel/events/uprobes.c
    @@ -723,20 +723,57 @@ remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, loff_t vaddr)
    }

    /*
    - * There could be threads that have hit the breakpoint and are entering the
    - * notifier code and trying to acquire the uprobes_treelock. The thread
    - * calling delete_uprobe() that is removing the uprobe from the rb_tree can
    - * race with these threads and might acquire the uprobes_treelock compared
    - * to some of the breakpoint hit threads. In such a case, the breakpoint
    - * hit threads will not find the uprobe. The current unregistering thread
    - * waits till all other threads have hit a breakpoint, to acquire the
    - * uprobes_treelock before the uprobe is removed from the rbtree.
    + * <userspace>
    + * ...
    + * int3 ----> <IRQ>
    + * do_int3
    + * (A) DIE_INT3 -> uprobe_pre_sstep_notifier()
    + * ...
    + * set_thread_flag(TIF_UPROBE)
    + * srcu_read_lock_raw()
    + * <EOI>
    + * (B)
    + * ret_from_intr
    + * do_notify_resume()
    + * uprobe_notify_resume()
    + * handle_swbp()
    + * uprobe = find_uprobe()
    + * atomic_inc(&uprobe->ref)
    + * srcu_read_unlock_raw()
    + * ...
    + * (C)
    + * put_uprobe()
    + * <---- ret_from_intr
    + *
    + * ...
    */
    static void delete_uprobe(struct uprobe *uprobe)
    {
    unsigned long flags;

    + /*
    + * At this point all breakpoint instructions belonging to this uprobe
    + * have been removed, so no new references to this uprobe can be
    + * created, however!
    + *
    + * There could be an in-progress breakpoint from before we removed the
    + * instruction still pending (A). synchronize_sched() insures all CPUs
    + * will have scheduled at least once, therefore all such pending
    + * interrupts will hereafter have reached (B) and thus have taken their
    + * SRCU reference.
    + */
    + synchronize_sched();
    +
    + /*
    + * Wait for all in-progress breakpoint handlers to finish, ensuring all
    + * handlers passed (C) turning all references into active refcounts.
    + */
    synchronize_srcu(&uprobes_srcu);
    +
    + /*
    + * We can now safely remove the uprobe, all references are active
    + * references and the refcounting will work as expected.
    + */
    spin_lock_irqsave(&uprobes_treelock, flags);
    rb_erase(&uprobe->rb_node, &uprobes_tree);
    spin_unlock_irqrestore(&uprobes_treelock, flags);


    \
     
     \ /
      Last update: 2012-04-15 23:51    [W:0.093 / U:0.184 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site