Re: [PATCH bpf-next] bpf: avoid get_kernel_nofault() to fetch kprobe entry IP

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Tue, 19 Mar 2024 14:20:13 -0700
Andrii Nakryiko <andrii@kernel.org> wrote:

> get_kernel_nofault() (or, rather, underlying copy_from_kernel_nofault())
> is not free and it does pop up in performance profiles when
> kprobes are heavily utilized with CONFIG_X86_KERNEL_IBT=y config.
> 
> Let's avoid using it if we know that fentry_ip - 4 can't cross page
> boundary. We do that by masking lowest 12 bits and checking if they are
> >= 4, in which case we can do direct memory read.
> 
> Another benefit (and actually what caused a closer look at this part of
> code) is that now LBR record is (typically) not wasted on
> copy_from_kernel_nofault() call and code, which helps tools like
> retsnoop that grab LBR records from inside BPF code in kretprobes.

Hmm, we may better to have this function in kprobe side and
store a flag which such architecture dependent offset is added.
That is more natural.

Thanks!

> 
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  kernel/trace/bpf_trace.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 0a5c4efc73c3..f81adabda38c 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1053,9 +1053,15 @@ static unsigned long get_entry_ip(unsigned long fentry_ip)
>  {
>  	u32 instr;
>  
> -	/* Being extra safe in here in case entry ip is on the page-edge. */
> -	if (get_kernel_nofault(instr, (u32 *) fentry_ip - 1))
> -		return fentry_ip;
> +	/* We want to be extra safe in case entry ip is on the page edge,
> +	 * but otherwise we need to avoid get_kernel_nofault()'s overhead.
> +	 */
> +	if ((fentry_ip & ~PAGE_MASK) < ENDBR_INSN_SIZE) {
> +		if (get_kernel_nofault(instr, (u32 *)(fentry_ip - ENDBR_INSN_SIZE)))
> +			return fentry_ip;
> +	} else {
> +		instr = *(u32 *)(fentry_ip - ENDBR_INSN_SIZE);
> +	}
>  	if (is_endbr(instr))
>  		fentry_ip -= ENDBR_INSN_SIZE;
>  	return fentry_ip;
> -- 
> 2.43.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>