[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

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

[Oleg Nesterov]

On 09/18, Adriano Cordova wrote:
>
> 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.

So why? Is it preparation for 2/2 ? At first glance it doesn't need
this change... Please add more details.

Oleg.

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

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

[Oleg Nesterov]

On 09/18, Adriano Cordova wrote:
>
> 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.

Sorry, I am confused.

Can you explain in more details what problem(s) this series tries to fix?

handle_swbp() checks UPROBE_COPY_INSN.

handle_syscall_uprobe() is only called by sys_uprobe() which checks
in_uprobe_trampoline(). So it can only be called after the probed insn
has been analyzed and optimized.

Oleg.

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