[PATCH 1/2] uprobes: Use a dedicated mutex to serialize prepare_uprobe()

[PATCH 1/2] uprobes: Use a dedicated mutex to serialize prepare_uprobe()

[Adriano Cordova]

prepare_uprobe() serializes its callers by taking consumer_rwsem for
writing. That rwsem protects the consumer list, not the instruction
copy.

Give struct uprobe a dedicated prepare_mutex and use it instead.

No functional change.

Signed-off-by: Adriano Cordova <adrianox@gmail.com>
---
 kernel/events/uprobes.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 7709ea882477..686585539ebf 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -64,6 +64,7 @@ struct uprobe {
 	refcount_t		ref;
 	struct rw_semaphore	register_rwsem;
 	struct rw_semaphore	consumer_rwsem;
+	struct mutex		prepare_mutex;	/* serializes prepare_uprobe() */
 	struct list_head	pending_list;
 	struct list_head	consumers;
 	struct inode		*inode;		/* Also hold a ref to inode */
@@ -1007,6 +1008,7 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset,
 	INIT_LIST_HEAD(&uprobe->consumers);
 	init_rwsem(&uprobe->register_rwsem);
 	init_rwsem(&uprobe->consumer_rwsem);
+	mutex_init(&uprobe->prepare_mutex);
 	RB_CLEAR_NODE(&uprobe->rb_node);
 	refcount_set(&uprobe->ref, 1);
 
@@ -1104,8 +1106,8 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
 	if (test_bit(UPROBE_COPY_INSN, &uprobe->flags))
 		return ret;
 
-	/* TODO: move this into _register, until then we abuse this sem. */
-	down_write(&uprobe->consumer_rwsem);
+	/* Serialize concurrent prepare_uprobe() calls from uprobe_mmap(). */
+	mutex_lock(&uprobe->prepare_mutex);
 	if (test_bit(UPROBE_COPY_INSN, &uprobe->flags))
 		goto out;
 
@@ -1125,7 +1127,7 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
 	set_bit(UPROBE_COPY_INSN, &uprobe->flags);
 
  out:
-	up_write(&uprobe->consumer_rwsem);
+	mutex_unlock(&uprobe->prepare_mutex);
 
 	return ret;
 }
-- 
2.51.0

[PATCH 2/2] uprobes: Hide unprepared uprobes from the breakpoint paths

[Adriano Cordova]

find_uprobe_rcu() backs handle_swbp() and handle_syscall_uprobe(). Make
it skip a uprobe whose instruction has not been analyzed yet, so neither
path can use an uninitialized ->arch. An unprepared uprobe has no
breakpoint installed (install_breakpoint() prepares before set_swbp()),
so the trap path treats it as not registered yet. This also gives
handle_syscall_uprobe() the check it was missing.

Signed-off-by: Adriano Cordova <adrianox@gmail.com>
---
 kernel/events/uprobes.c | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 686585539ebf..bdc05fdce0a0 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -924,8 +924,20 @@ static struct uprobe *find_uprobe_rcu(struct inode *inode, loff_t offset)
 		 * try again as we might have missed the element (false
 		 * negative). If seqcount is unchanged, search truly failed.
 		 */
-		if (node)
-			return __node_2_uprobe(node);
+		if (node) {
+			struct uprobe *uprobe = __node_2_uprobe(node);
+
+			/*
+			 * A uprobe is inserted before it is prepared; do
+			 * not hand it out until ->arch is usable.
+			 */
+			if (!test_bit(UPROBE_COPY_INSN, &uprobe->flags))
+				return NULL;
+
+			/* Pairs with the smp_wmb() in prepare_uprobe(). */
+			smp_rmb();
+			return uprobe;
+		}
 	} while (read_seqcount_retry(&uprobes_seqcount, seq));
 
 	return NULL;
@@ -1123,7 +1135,7 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
 	if (ret)
 		goto out;
 
-	smp_wmb(); /* pairs with the smp_rmb() in handle_swbp() */
+	smp_wmb(); /* pairs with the smp_rmb() in find_uprobe_rcu() */
 	set_bit(UPROBE_COPY_INSN, &uprobe->flags);
 
  out:
@@ -2743,23 +2755,6 @@ static void handle_swbp(struct pt_regs *regs)
 	/* change it in advance for ->handler() and restart */
 	instruction_pointer_set(regs, bp_vaddr);
 
-	/*
-	 * TODO: move copy_insn/etc into _register and remove this hack.
-	 * After we hit the bp, _unregister + _register can install the
-	 * new and not-yet-analyzed uprobe at the same address, restart.
-	 */
-	if (unlikely(!test_bit(UPROBE_COPY_INSN, &uprobe->flags)))
-		goto out;
-
-	/*
-	 * Pairs with the smp_wmb() in prepare_uprobe().
-	 *
-	 * Guarantees that if we see the UPROBE_COPY_INSN bit set, then
-	 * we must also see the stores to &uprobe->arch performed by the
-	 * prepare_uprobe() call.
-	 */
-	smp_rmb();
-
 	/* Tracing handlers use ->utask to communicate with fetch methods */
 	if (!get_utask())
 		goto out;
-- 
2.51.0