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

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

[Andrii Nakryiko]

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.

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

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

[Masami Hiramatsu]

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>

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

[Jiri Olsa]

On Wed, Mar 20, 2024 at 12:47:42PM +0900, Masami Hiramatsu wrote:
> 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.

I think this is nice improvement

Acked-by: Jiri Olsa <jolsa@kernel.org>

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

I like the idea of new flag saying the address was adjusted for endbr

kprobe adjust the address in arch_adjust_kprobe_addr, it could be
easily added in there and then we'd adjust the address in get_entry_ip
accordingly

jirka

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

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

[Andrii Nakryiko]

On Wed, Mar 20, 2024 at 1:34 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Mar 20, 2024 at 12:47:42PM +0900, Masami Hiramatsu wrote:
> > 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.
>
> I think this is nice improvement
>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
>

Masami, are you ok if we land this rather straightforward fix in
bpf-next tree for now, and then you or someone a bit more familiar
with ftrace/kprobe internals can generalize this in a more generic
way?

> >
> > 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.
>
> I like the idea of new flag saying the address was adjusted for endbr
>

instead of a flag, can kprobe low-level infrastructure just provide
"effective fentry ip" without any flags, so that BPF side of things
don't have to care?

> kprobe adjust the address in arch_adjust_kprobe_addr, it could be
> easily added in there and then we'd adjust the address in get_entry_ip
> accordingly
>
> jirka
>
> >
> > 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>
> >

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

[Masami Hiramatsu]

On Wed, 20 Mar 2024 10:46:54 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> On Wed, Mar 20, 2024 at 1:34 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Wed, Mar 20, 2024 at 12:47:42PM +0900, Masami Hiramatsu wrote:
> > > 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.
> >
> > I think this is nice improvement
> >
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> >
> 
> Masami, are you ok if we land this rather straightforward fix in
> bpf-next tree for now, and then you or someone a bit more familiar
> with ftrace/kprobe internals can generalize this in a more generic
> way?

I'm OK for this change for short term fix. As far as I can see, the
kprobe-side change may involve more kprobe internal changes, so

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

> 
> > >
> > > 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.
> >
> > I like the idea of new flag saying the address was adjusted for endbr
> >
> 
> instead of a flag, can kprobe low-level infrastructure just provide
> "effective fentry ip" without any flags, so that BPF side of things
> don't have to care?

It's possible. But it is a bit only for BPF and not fit to kprobe
itself. I think we can add it in trace_kprobe instead of kprobe,
which can be accessed from struct kprobe *kp.

Thank you,

> 
> > kprobe adjust the address in arch_adjust_kprobe_addr, it could be
> > easily added in there and then we'd adjust the address in get_entry_ip
> > accordingly
> >
> > jirka
> >
> > >
> > > 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>
> > >


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

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

[Andrii Nakryiko]

On Wed, Mar 20, 2024 at 4:46 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Wed, 20 Mar 2024 10:46:54 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > On Wed, Mar 20, 2024 at 1:34 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Wed, Mar 20, 2024 at 12:47:42PM +0900, Masami Hiramatsu wrote:
> > > > 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.
> > >
> > > I think this is nice improvement
> > >
> > > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > >
> >
> > Masami, are you ok if we land this rather straightforward fix in
> > bpf-next tree for now, and then you or someone a bit more familiar
> > with ftrace/kprobe internals can generalize this in a more generic
> > way?
>
> I'm OK for this change for short term fix. As far as I can see, the
> kprobe-side change may involve more kprobe internal changes, so
>
> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>

Great, thank you!

> >
> > > >
> > > > 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.
> > >
> > > I like the idea of new flag saying the address was adjusted for endbr
> > >
> >
> > instead of a flag, can kprobe low-level infrastructure just provide
> > "effective fentry ip" without any flags, so that BPF side of things
> > don't have to care?
>
> It's possible. But it is a bit only for BPF and not fit to kprobe
> itself. I think we can add it in trace_kprobe instead of kprobe,
> which can be accessed from struct kprobe *kp.

sure, if it can be just "endbr64 offset" instead of a true/false flag,
it would help to avoid extra conditionals in the hot path (which waste
LBR records in some mode, which are important in some applications)

>
> Thank you,
>
> >
> > > kprobe adjust the address in arch_adjust_kprobe_addr, it could be
> > > easily added in there and then we'd adjust the address in get_entry_ip
> > > accordingly
> > >
> > > jirka
> > >
> > > >
> > > > 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>
> > > >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Tue, 19 Mar 2024 14:20:13 -0700 you 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.
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: avoid get_kernel_nofault() to fetch kprobe entry IP
    https://git.kernel.org/bpf/bpf-next/c/a8497506cd2c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html