Re: [RFC 0/6] uprobes: kill uprobes_srcu/uprobe_srcu_id

[Peter Zijlstra <peterz@infradead.org>]

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);