On 01/12/2023 17:06, 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"). > > v1->v2: > - don't rely on assembly output in verifier log, which changes between > compiler versions (CI). > > Suggested-by: Alexei Starovoitov <ast@xxxxxxxxxx> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> one minor thing below, but Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx> > --- > .../bpf/prog_tests/global_func_dead_code.c | 60 +++++++++++++++++++ > .../bpf/progs/freplace_dead_global_func.c | 11 ++++ > .../bpf/progs/verifier_global_subprogs.c | 33 ++++++---- > 3 files changed, 92 insertions(+), 12 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 > > 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..d873eb20dd7c > --- /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"); missing "err = " assignment here? > + 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..8ddc2f354be9 100644 > --- a/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c > +++ b/tools/testing/selftests/bpf/progs/verifier_global_subprogs.c > @@ -10,50 +10,59 @@ > > 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") > __success __log_level(2) > /* main prog is validated completely first */ > __msg("('global_calls_good_only') is global and assumed valid.") > -__msg("1: (95) exit") > /* eventually global_good() is transitively validated as well */ > __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("('global_calls_bad') is global and assumed valid.") > /* 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