[PATCH bpf-next 1/2] bpf: Return error for missed kprobe multi bpf program execution

[PATCH bpf-next 1/2] bpf: Return error for missed kprobe multi bpf program execution

[Jiri Olsa]

When kprobe multi bpf program can't be executed due to recursion check,
we currently return 0 (success) to fprobe layer where it's ignored for
standard kprobe multi probes.

For kprobe session the success return value will make fprobe layer to
install return probe and try to execute it as well.

But the return session probe should not get executed, because the entry
part did not run. FWIW the return probe bpf program most likely won't get
executed, because its recursion check will likely fail as well, but we
don't need to run it in the first place.. also we can make this clear
and obvious.

It also affects missed counts for kprobe session program execution, which
are now doubled (extra count for not executed return probe).

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/bpf_trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 48db147c6c7d..1f3d4b72a3f2 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2797,7 +2797,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
 
 	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
 		bpf_prog_inc_misses_counter(link->link.prog);
-		err = 0;
+		err = 1;
 		goto out;
 	}
 
-- 
2.47.0

[PATCH bpf-next 2/2] selftests/bpf: Add kprobe session recursion check test

[Jiri Olsa]

Adding kprobe.session probe to bpf_kfunc_common_test that misses bpf
program execution due to recursion check and making sure it increases
the program missed count properly.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/testing/selftests/bpf/prog_tests/missed.c             | 1 +
 tools/testing/selftests/bpf/progs/missed_kprobe_recursion.c | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/missed.c b/tools/testing/selftests/bpf/prog_tests/missed.c
index 70d90c43537c..ed8857ae914a 100644
--- a/tools/testing/selftests/bpf/prog_tests/missed.c
+++ b/tools/testing/selftests/bpf/prog_tests/missed.c
@@ -85,6 +85,7 @@ static void test_missed_kprobe_recursion(void)
 	ASSERT_GE(get_missed_count(bpf_program__fd(skel->progs.test3)), 1, "test3_recursion_misses");
 	ASSERT_GE(get_missed_count(bpf_program__fd(skel->progs.test4)), 1, "test4_recursion_misses");
 	ASSERT_GE(get_missed_count(bpf_program__fd(skel->progs.test5)), 1, "test5_recursion_misses");
+	ASSERT_EQ(get_missed_count(bpf_program__fd(skel->progs.test6)), 1, "test6_recursion_misses");
 
 cleanup:
 	missed_kprobe_recursion__destroy(skel);
diff --git a/tools/testing/selftests/bpf/progs/missed_kprobe_recursion.c b/tools/testing/selftests/bpf/progs/missed_kprobe_recursion.c
index c4bf679a9876..29c18d869ec1 100644
--- a/tools/testing/selftests/bpf/progs/missed_kprobe_recursion.c
+++ b/tools/testing/selftests/bpf/progs/missed_kprobe_recursion.c
@@ -46,3 +46,9 @@ int test5(struct pt_regs *ctx)
 {
 	return 0;
 }
+
+SEC("kprobe.session/bpf_kfunc_common_test")
+int test6(struct pt_regs *ctx)
+{
+	return 0;
+}
-- 
2.47.0

Re: [PATCH bpf-next 1/2] bpf: Return error for missed kprobe multi bpf program execution

[Song Liu]

On Mon, Jan 6, 2025 at 9:50 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> When kprobe multi bpf program can't be executed due to recursion check,
> we currently return 0 (success) to fprobe layer where it's ignored for
> standard kprobe multi probes.
>
> For kprobe session the success return value will make fprobe layer to
> install return probe and try to execute it as well.
>
> But the return session probe should not get executed, because the entry
> part did not run. FWIW the return probe bpf program most likely won't get
> executed, because its recursion check will likely fail as well, but we
> don't need to run it in the first place.. also we can make this clear
> and obvious.
>
> It also affects missed counts for kprobe session program execution, which
> are now doubled (extra count for not executed return probe).
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  kernel/trace/bpf_trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 48db147c6c7d..1f3d4b72a3f2 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2797,7 +2797,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
>
>         if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
>                 bpf_prog_inc_misses_counter(link->link.prog);
> -               err = 0;
> +               err = 1;

nit: Shall we return -EBUSY or some other error code?

>                 goto out;
>         }

>
> --
> 2.47.0
>
>

Re: [PATCH bpf-next 1/2] bpf: Return error for missed kprobe multi bpf program execution

[Masami Hiramatsu]

On Mon,  6 Jan 2025 18:50:47 +0100
Jiri Olsa <jolsa@kernel.org> wrote:

> When kprobe multi bpf program can't be executed due to recursion check,
> we currently return 0 (success) to fprobe layer where it's ignored for
> standard kprobe multi probes.
> 
> For kprobe session the success return value will make fprobe layer to
> install return probe and try to execute it as well.
> 
> But the return session probe should not get executed, because the entry
> part did not run. FWIW the return probe bpf program most likely won't get
> executed, because its recursion check will likely fail as well, but we
> don't need to run it in the first place.. also we can make this clear
> and obvious.

Yeah, that's right.

> 
> It also affects missed counts for kprobe session program execution, which
> are now doubled (extra count for not executed return probe).
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Looks good to me.

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

Thanks!

> ---
>  kernel/trace/bpf_trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 48db147c6c7d..1f3d4b72a3f2 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2797,7 +2797,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
>  
>  	if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
>  		bpf_prog_inc_misses_counter(link->link.prog);
> -		err = 0;
> +		err = 1;
>  		goto out;
>  	}
>  
> -- 
> 2.47.0
> 


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

Re: [PATCH bpf-next 1/2] bpf: Return error for missed kprobe multi bpf program execution

[Jiri Olsa]

On Mon, Jan 06, 2025 at 02:26:22PM -0800, Song Liu wrote:
> On Mon, Jan 6, 2025 at 9:50 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > When kprobe multi bpf program can't be executed due to recursion check,
> > we currently return 0 (success) to fprobe layer where it's ignored for
> > standard kprobe multi probes.
> >
> > For kprobe session the success return value will make fprobe layer to
> > install return probe and try to execute it as well.
> >
> > But the return session probe should not get executed, because the entry
> > part did not run. FWIW the return probe bpf program most likely won't get
> > executed, because its recursion check will likely fail as well, but we
> > don't need to run it in the first place.. also we can make this clear
> > and obvious.
> >
> > It also affects missed counts for kprobe session program execution, which
> > are now doubled (extra count for not executed return probe).
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/trace/bpf_trace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 48db147c6c7d..1f3d4b72a3f2 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2797,7 +2797,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
> >
> >         if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
> >                 bpf_prog_inc_misses_counter(link->link.prog);
> > -               err = 0;
> > +               err = 1;
> 
> nit: Shall we return -EBUSY or some other error code?

it's processed in __fprobe_handler and it's treated as bool, so technically
it does not matter.. but I'd rather keep the 0/1 return values in here,
because it's what the session program is forced to return

thanks,
jirka

Re: [PATCH bpf-next 1/2] bpf: Return error for missed kprobe multi bpf program execution

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Mon,  6 Jan 2025 18:50:47 +0100 you wrote:
> When kprobe multi bpf program can't be executed due to recursion check,
> we currently return 0 (success) to fprobe layer where it's ignored for
> standard kprobe multi probes.
> 
> For kprobe session the success return value will make fprobe layer to
> install return probe and try to execute it as well.
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/2] bpf: Return error for missed kprobe multi bpf program execution
    https://git.kernel.org/bpf/bpf-next/c/2ebadb60cb36
  - [bpf-next,2/2] selftests/bpf: Add kprobe session recursion check test
    https://git.kernel.org/bpf/bpf-next/c/bfaac2a0b9e5

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

Re: [PATCH bpf-next 1/2] bpf: Return error for missed kprobe multi bpf program execution

[Song Liu]

On Wed, Jan 8, 2025 at 3:38 AM Jiri Olsa <olsajiri@gmail.com> wrote:
[...]
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index 48db147c6c7d..1f3d4b72a3f2 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -2797,7 +2797,7 @@ kprobe_multi_link_prog_run(struct bpf_kprobe_multi_link *link,
> > >
> > >         if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
> > >                 bpf_prog_inc_misses_counter(link->link.prog);
> > > -               err = 0;
> > > +               err = 1;
> >
> > nit: Shall we return -EBUSY or some other error code?
>
> it's processed in __fprobe_handler and it's treated as bool, so technically
> it does not matter.. but I'd rather keep the 0/1 return values in here,
> because it's what the session program is forced to return

Got it. Thanks for the explanation.

Song