Re: [PATCH 1/3] uprobes: install_breakpoint() should fail ifis_swbp_insn() == T

[Peter Zijlstra]

On Wed, 2012-05-30 at 19:49 +0200, Peter Zijlstra wrote:
> Oh, indeed. I overlooked copy_insn() is taking the page from the
> page-cache instead of the page-tables and will thus get the original.
> 
> OK, no worries then. 

So its the -EEXIST from set_swbp() that I was thinking about. I think I
was also wrong on the reason that that can happen. register's vma
iteration is very careful not to have the same vma twice, but it can
race against mmap() because of the uprobe_hash() vs uprobe_mmap_hash()
madness, right?

Something like so?

---
 kernel/events/uprobes.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 985be4d..b4e749e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -45,6 +45,19 @@ static DEFINE_SPINLOCK(uprobes_treelock);	/* serialize rbtree access */
 
 #define UPROBES_HASH_SZ	13
 
+/*
+ * We need separate register/unregister and mmap/munmap lock hashes because of
+ * mmap_sem nesting.
+ *
+ * {,un}egister_uprobe() needs to install probes on (potentially) all processes
+ * and thus need to acquire multiple mmap_sems (consecutively, not
+ * concurrently), whereas uprobe_m{,un}map() is called while holding mmap_sem
+ * for the particular process doing the mmap.
+ *
+ * This all means that register_uprobe() can race with uprobe_mmap() and we
+ * can try and install a probe where one is already installed.
+ */
+
 /* serialize (un)register */
 static struct mutex uprobes_mutex[UPROBES_HASH_SZ];
 
@@ -356,6 +369,9 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned
 {
 	int result;
 
+	/*
+	 * See the comment near uprobes_hash().
+	 */
 	result = is_swbp_at_addr(mm, vaddr);
 	if (result == 1)
 		return -EEXIST;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/