[bpf-next] selftests/bpf: validate eliminated global subprog is not freplaceable

Commit Message

Andrii Nakryiko Dec. 1, 2023, 1:30 a.m. UTC

Add selftest that establishes dead code-eliminated valid global subprog
(global_dead) and makes sure that it's not possible to freplace it, as
it's effectively not there. This test will fail with unexpected success
before 2afae08c9dcb ("bpf: Validate global subprogs lazily").

Suggested-by: Alexei Starovoitov <[e-mail]>
Signed-off-by: Andrii Nakryiko <[e-mail]>
---
 .../bpf/prog_tests/global_func_dead_code.c    | 60 +++++++++++++++++++
 .../bpf/progs/freplace_dead_global_func.c     | 11 ++++
 .../bpf/progs/verifier_global_subprogs.c      | 32 ++++++----
 3 files changed, 92 insertions(+), 11 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/global_func_dead_code.c
 create mode 100644 tools/testing/selftests/bpf/progs/freplace_dead_global_func.c

Comments

Eduard Zingerman Dec. 1, 2023, 3:16 p.m. UTC | #1

On Thu, 2023-11-30 at 17:30 -0800, Andrii Nakryiko wrote:
> Add selftest that establishes dead code-eliminated valid global subprog
> (global_dead) and makes sure that it's not possible to freplace it, as
> it's effectively not there. This test will fail with unexpected success
> before 2afae08c9dcb ("bpf: Validate global subprogs lazily").
> 
> Suggested-by: Alexei Starovoitov <[e-mail]>
> Signed-off-by: Andrii Nakryiko <[e-mail]>
> ---

Acked-by: Eduard Zingerman <[e-mail]>

[...]
> diff --git a/tools/testing/selftests/bpf/prog_tests/global_func_dead_code.c b/tools/testing/selftests/bpf/prog_tests/global_func_dead_code.c
[...]
> +void test_global_func_dead_code(void)
> +{
[...]
> +	ASSERT_HAS_SUBSTR(log_buf, "Subprog global_dead doesn't exist", "dead_subprog_missing_msg");

Nit: the log is not printed if verbose tests execution is requested.

[...]

> index a0a5efd1caa1..7f9b21a1c5a7 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
> @@ -10,25 +10,31 @@
>  
>  int arr[1];
>  int unkn_idx;
> +const volatile bool call_dead_subprog = false;
>  
> -__noinline long global_bad(void)
> +__noinline long global_bad(int x)
>  {
> -	return arr[unkn_idx]; /* BOOM */
> +	return arr[unkn_idx] + x; /* BOOM */
>  }

Nit/question:
  Why change prototype from (void) to (int) here and elsewhere?
  Does not seem necessary for test logic.

[...]

Andrii Nakryiko Dec. 1, 2023, 7:17 p.m. UTC | #2

On Fri, Dec 1, 2023 at 7:16 AM Eduard Zingerman <[e-mail]> wrote:
>
> On Thu, 2023-11-30 at 17:30 -0800, Andrii Nakryiko wrote:
> > Add selftest that establishes dead code-eliminated valid global subprog
> > (global_dead) and makes sure that it's not possible to freplace it, as
> > it's effectively not there. This test will fail with unexpected success
> > before 2afae08c9dcb ("bpf: Validate global subprogs lazily").
> >
> > Suggested-by: Alexei Starovoitov <[e-mail]>
> > Signed-off-by: Andrii Nakryiko <[e-mail]>
> > ---
>
> Acked-by: Eduard Zingerman <[e-mail]>

Oops, didn't see your reply before sending v2. But there will be v3 anyway :)

>
> [...]
> > diff --git a/tools/testing/selftests/bpf/prog_tests/global_func_dead_code.c b/tools/testing/selftests/bpf/prog_tests/global_func_dead_code.c
> [...]
> > +void test_global_func_dead_code(void)
> > +{
> [...]
> > +     ASSERT_HAS_SUBSTR(log_buf, "Subprog global_dead doesn't exist", "dead_subprog_missing_msg");
>
> Nit: the log is not printed if verbose tests execution is requested.

I'm not sure I understand. What do you expect to happen that's not
happening in verbose mode?

>
> [...]
>
> > index a0a5efd1caa1..7f9b21a1c5a7 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
> > @@ -10,25 +10,31 @@
> >
> >  int arr[1];
> >  int unkn_idx;
> > +const volatile bool call_dead_subprog = false;
> >
> > -__noinline long global_bad(void)
> > +__noinline long global_bad(int x)
> >  {
> > -     return arr[unkn_idx]; /* BOOM */
> > +     return arr[unkn_idx] + x; /* BOOM */
> >  }
>
> Nit/question:
>   Why change prototype from (void) to (int) here and elsewhere?
>   Does not seem necessary for test logic.

I had some troubles attaching freplace initially, but my freplace
skills were rusty :) I can try undoing this and leaving it as is.

>
> [...]

Eduard Zingerman Dec. 1, 2023, 7:20 p.m. UTC | #3

On Fri, 2023-12-01 at 11:17 -0800, Andrii Nakryiko wrote:
[...]
> > Acked-by: Eduard Zingerman <[e-mail]>
> 
> Oops, didn't see your reply before sending v2. But there will be v3 anyway :)

np

[...]
> > Nit: the log is not printed if verbose tests execution is requested.
> 
> I'm not sure I understand. What do you expect to happen that's not
> happening in verbose mode?

I tried running this test -vvv and it did not print verification log
(admittedly this is the case with many tests in prog_tests/*.c).

[...]

> > Nit/question:
> >   Why change prototype from (void) to (int) here and elsewhere?
> >   Does not seem necessary for test logic.
> 
> I had some troubles attaching freplace initially, but my freplace
> skills were rusty :) I can try undoing this and leaving it as is.

No strong opinion, just curious.

Andrii Nakryiko Dec. 1, 2023, 7:26 p.m. UTC | #4

On Fri, Dec 1, 2023 at 11:20 AM Eduard Zingerman <[e-mail]> wrote:
>
> On Fri, 2023-12-01 at 11:17 -0800, Andrii Nakryiko wrote:
> [...]
> > > Acked-by: Eduard Zingerman <[e-mail]>
> >
> > Oops, didn't see your reply before sending v2. But there will be v3 anyway :)
>
> np
>
> [...]
> > > Nit: the log is not printed if verbose tests execution is requested.
> >
> > I'm not sure I understand. What do you expect to happen that's not
> > happening in verbose mode?
>
> I tried running this test -vvv and it did not print verification log
> (admittedly this is the case with many tests in prog_tests/*.c).

I think that's the test_loader.c feature, plus maybe some other tests
support this. This is not expected to magically work for all tests.
But also in this case we explicitly intercept the log, so it would be
too much trouble to both intercept and print it at the same time, IMO.
But if this assertion fails, we'll see the log, which is the most
important part. Also one can use veristat to get the log.

>
> [...]
>
> > > Nit/question:
> > >   Why change prototype from (void) to (int) here and elsewhere?
> > >   Does not seem necessary for test logic.
> >
> > I had some troubles attaching freplace initially, but my freplace
> > skills were rusty :) I can try undoing this and leaving it as is.
>
> No strong opinion, just curious.

I undid it, it all works now. As I said, I had freplace troubles and
was poking around with different aspects.

Eduard Zingerman Dec. 1, 2023, 8:02 p.m. UTC | #5

On Fri, 2023-12-01 at 11:26 -0800, Andrii Nakryiko wrote:
> > [...]
> > > > Nit: the log is not printed if verbose tests execution is requested.
> > > 
> > > I'm not sure I understand. What do you expect to happen that's not
> > > happening in verbose mode?
> > 
> > I tried running this test -vvv and it did not print verification log
> > (admittedly this is the case with many tests in prog_tests/*.c).
> 
> I think that's the test_loader.c feature, plus maybe some other tests
> support this. This is not expected to magically work for all tests.
> But also in this case we explicitly intercept the log, so it would be
> too much trouble to both intercept and print it at the same time, IMO.
> But if this assertion fails, we'll see the log, which is the most
> important part. Also one can use veristat to get the log.

Well, yes, that was the point of my rumbling.
When it's necessary to debug some such test one needs to modify it to
use *_opts() load variant etc. Veristat makes sense, however, so not
an issue.

> > > > Nit/question:
> > > >   Why change prototype from (void) to (int) here and elsewhere?
> > > >   Does not seem necessary for test logic.
> > > 
> > > I had some troubles attaching freplace initially, but my freplace
> > > skills were rusty :) I can try undoing this and leaving it as is.
> > 
> > No strong opinion, just curious.
> 
> I undid it, it all works now. As I said, I had freplace troubles and
> was poking around with different aspects.

Thank you.

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/global_func_dead_code.c b/tools/testing/selftests/bpf/prog_tests/global_func_dead_code.c
new file mode 100644
index 000000000000..2716f6ccfe22
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/global_func_dead_code.c
@@ -0,0 +1,60 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+
+#include <test_progs.h>
+#include "verifier_global_subprogs.skel.h"
+#include "freplace_dead_global_func.skel.h"
+
+void test_global_func_dead_code(void)
+{
+	struct verifier_global_subprogs *tgt_skel = NULL;
+	struct freplace_dead_global_func *skel = NULL;
+	char log_buf[4096];
+	int err, tgt_fd;
+
+	/* first, try to load target with good global subprog */
+	tgt_skel = verifier_global_subprogs__open();
+	if (!ASSERT_OK_PTR(tgt_skel, "tgt_skel_good_open"))
+		return;
+
+	bpf_program__set_autoload(tgt_skel->progs.chained_global_func_calls_success, true);
+
+	err = verifier_global_subprogs__load(tgt_skel);
+	if (!ASSERT_OK(err, "tgt_skel_good_load"))
+		goto out;
+
+	tgt_fd = bpf_program__fd(tgt_skel->progs.chained_global_func_calls_success);
+
+	/* Attach to good non-eliminated subprog */
+	skel = freplace_dead_global_func__open();
+	if (!ASSERT_OK_PTR(skel, "skel_good_open"))
+		goto out;
+
+	bpf_program__set_attach_target(skel->progs.freplace_prog, tgt_fd, "global_good");
+	ASSERT_OK(err, "attach_target_good");
+	
+	err = freplace_dead_global_func__load(skel);
+	if (!ASSERT_OK(err, "skel_good_load"))
+		goto out;
+
+	freplace_dead_global_func__destroy(skel);
+
+	/* Try attaching to dead code-eliminated subprog */
+	skel = freplace_dead_global_func__open();
+	if (!ASSERT_OK_PTR(skel, "skel_dead_open"))
+		goto out;
+
+	bpf_program__set_log_buf(skel->progs.freplace_prog, log_buf, sizeof(log_buf));
+	err = bpf_program__set_attach_target(skel->progs.freplace_prog, tgt_fd, "global_dead");
+	ASSERT_OK(err, "attach_target_dead");
+	
+	err = freplace_dead_global_func__load(skel);
+	if (!ASSERT_ERR(err, "skel_dead_load"))
+		goto out;
+
+	ASSERT_HAS_SUBSTR(log_buf, "Subprog global_dead doesn't exist", "dead_subprog_missing_msg");
+
+out:
+	verifier_global_subprogs__destroy(tgt_skel);
+	freplace_dead_global_func__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/freplace_dead_global_func.c b/tools/testing/selftests/bpf/progs/freplace_dead_global_func.c
new file mode 100644
index 000000000000..808738eac578
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/freplace_dead_global_func.c
@@ -0,0 +1,11 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("freplace")
+int freplace_prog(int x)
+{
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
index a0a5efd1caa1..7f9b21a1c5a7 100644
--- a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
+++ b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c
@@ -10,25 +10,31 @@ 
 
 int arr[1];
 int unkn_idx;
+const volatile bool call_dead_subprog = false;
 
-__noinline long global_bad(void)
+__noinline long global_bad(int x)
 {
-	return arr[unkn_idx]; /* BOOM */
+	return arr[unkn_idx] + x; /* BOOM */
 }
 
-__noinline long global_good(void)
+__noinline long global_good(int x)
 {
-	return arr[0];
+	return arr[0] + x;
 }
 
-__noinline long global_calls_bad(void)
+__noinline long global_calls_bad(int x)
 {
-	return global_good() + global_bad() /* does BOOM indirectly */;
+	return global_good(x) + global_bad(x) /* does BOOM indirectly */;
 }
 
-__noinline long global_calls_good_only(void)
+__noinline long global_calls_good_only(int x)
 {
-	return global_good();
+	return global_good(x);
+}
+
+__noinline long global_dead(int x)
+{
+	return x * 2;
 }
 
 SEC("?raw_tp")
@@ -41,19 +47,23 @@  __msg("Validating global_good() func")
 __msg("('global_good') is safe for any args that match its prototype")
 int chained_global_func_calls_success(void)
 {
-	return global_calls_good_only();
+	int sum = 0;
+
+	if (call_dead_subprog)
+		sum += global_dead(42);
+	return global_calls_good_only(42) + sum;
 }
 
 SEC("?raw_tp")
 __failure __log_level(2)
 /* main prog validated successfully first */
-__msg("1: (95) exit")
+__msg("2: (95) exit")
 /* eventually we validate global_bad() and fail */
 __msg("Validating global_bad() func")
 __msg("math between map_value pointer and register") /* BOOM */
 int chained_global_func_calls_bad(void)
 {
-	return global_calls_bad();
+	return global_calls_bad(13);
 }
 
 /* do out of bounds access forcing verifier to fail verification if this