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

[Oleg Nesterov <oleg@redhat.com>]

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
>