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);
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href