Re: [PATCH v3] perf,x86: avoid missing caller address in stack traces captured in uprobe

[Josh Poimboeuf <jpoimboe@kernel.org>]

On Tue, Jul 02, 2024 at 09:02:03PM -0700, Andrii Nakryiko wrote:
> @@ -2833,6 +2858,18 @@ perf_callchain_user32(struct pt_regs *regs, struct perf_callchain_entry_ctx *ent
>  
>  	fp = compat_ptr(ss_base + regs->bp);
>  	pagefault_disable();
> +
> +#ifdef CONFIG_UPROBES
> +	/* see perf_callchain_user() below for why we do this */
> +	if (current->utask) {
> +		u32 ret_addr;
> +
> +		if (is_uprobe_at_func_entry(regs, current->utask->auprobe) &&
> +		    !__get_user(ret_addr, (const u32 __user *)regs->sp))

Shouldn't the regs->sp value be checked with __access_ok() before
calling __get_user()?

Also, instead of littering functions with ifdefs it would be better to
abstract this out into a separate function which has an "always return
false" version for !CONFIG_UPROBES.  Then the above could be simplified to
something like:

	...
 	pagefault_disable();

	if (is_uprobe_at_func_entry(regs, current) &&
	    __access_ok(regs->sp, 4) &&
	    !__get_user(ret_addr, (const u32 __user *)regs->sp))
	    	perf_callchain_store(entry, ret_addr);
	...

Also it's good practice to wait at least several days before posting new
versions to avoid spamming reviewers and to give them time to digest
what you've already sent.

-- 
Josh