From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx119.postini.com [74.125.245.119]) by kanga.kvack.org (Postfix) with SMTP id 7FE846B004D for ; Sun, 15 Apr 2012 17:48:57 -0400 (EDT) Message-ID: <1334526513.28150.23.camel@twins> Subject: Re: [RFC 0/6] uprobes: kill uprobes_srcu/uprobe_srcu_id From: Peter Zijlstra Date: Sun, 15 Apr 2012 23:48:33 +0200 In-Reply-To: <20120415195351.GA22095@redhat.com> References: <20120405222024.GA19154@redhat.com> <1334409396.2528.100.camel@twins> <20120414205200.GA9083@redhat.com> <1334487062.2528.113.camel@twins> <20120415195351.GA22095@redhat.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 Sender: owner-linux-mm@kvack.org List-ID: To: Oleg Nesterov Cc: Ingo Molnar , Srikar Dronamraju , Andrew Morton , Linus Torvalds , Ananth N Mavinakayanahalli , Jim Keniston , LKML , Linux-mm , Andi Kleen , Christoph Hellwig , Steven Rostedt , Arnaldo Carvalho de Melo , Masami Hiramatsu , Thomas Gleixner , Anton Arapov 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.. >=20 > To ensure handle_swbp() can't race with unregister + register > and send the wrong SIGTRAP. >=20 > handle_swbp() roughly does under down_read(mmap_sem) >=20 >=20 > 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 >=20 >=20 > 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. >=20 > 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_st= ruct *mm, loff_t vaddr) } =20 /* - * There could be threads that have hit the breakpoint and are entering th= e - * notifier code and trying to acquire the uprobes_treelock. The thread - * calling delete_uprobe() that is removing the uprobe from the rb_tree ca= n - * 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. + * + * ... + * int3 ----> + * do_int3 + * (A) DIE_INT3 -> uprobe_pre_sstep_notifier() + * ... + * set_thread_flag(TIF_UPROBE) + * srcu_read_lock_raw() + * + * (B) + * ret_from_intr + * do_notify_resume() + * uprobe_notify_resume() + * handle_swbp() + * uprobe =3D 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; =20 + /* + * 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@kvack.org. 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: email@kvack.org