On Wed, Mar 29, 2023 at 11:36 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2023-03-27 at 11:52 -0700, Andrii Nakryiko wrote:
> > SEC("freplace") (i.e., BPF_PROG_TYPE_EXT) programs are not loadable as
> > is through veristat, as kernel expects actual program's FD during
> > BPF_PROG_LOAD time, which veristat has no way of knowing.
> >
> > Unfortunately, freplace programs are a pretty important class of
> > programs, especially when dealing with XDP chaining solutions, which
> > rely on EXT programs.
> >
> > So let's do our best and teach veristat to try to guess the original
> > program type, based on program's context argument type. And if guessing
> > process succeeds, we manually override freplace/EXT with guessed program
> > type using bpf_program__set_type() setter to increase chances of proper
> > BPF verification.
> >
> > We rely on BTF and maintain a simple lookup table. This process is
> > obviously not 100% bulletproof, as valid program might not use context
> > and thus wouldn't have to specify correct type. Also, __sk_buff is very
> > ambiguous and is the context type across many different program types.
> > We pick BPF_PROG_TYPE_CGROUP_SKB for now, which seems to work fine in
> > practice so far. Similarly, some program types require specifying attach
> > type, and so we pick one out of possible few variants.
> >
> > Best effort at its best. But this makes veristat even more widely
> > applicable.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> I left one nitpick below, otherwise looks good.
>
> I tried in on freplace programs from selftests and only 3 out 18
> programs verified correctly, others complained about unavailable
> functions or exit code not in range [0, 1], etc.
> Not sure, if it's possible to select more permissive attachment kinds, though.
>
> Tested-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> > ---
> > tools/testing/selftests/bpf/veristat.c | 121 ++++++++++++++++++++++++-
> > 1 file changed, 117 insertions(+), 4 deletions(-)
> >
[...]
> > +
> > + /* context argument is a pointer to a struct/typedef */
> > + t = btf__type_by_id(btf, btf_params(t)[0].type);
> > + while (t && btf_is_mod(t))
> > + t = btf__type_by_id(btf, t->type);
> > + if (!t || !btf_is_ptr(t))
> > + goto skip_freplace_fixup;
> > + t = btf__type_by_id(btf, t->type);
> > + while (t && btf_is_mod(t))
> > + t = btf__type_by_id(btf, t->type);
> > + if (!t)
> > + goto skip_freplace_fixup;
>
> Nitpick:
> In case if something goes wrong with BTF there is no "Failed to guess ..." message.
I had a strong expectation that BTF is not malformed, which seems
pretty reasonable and common case (you will also get kernel errors
when you attempt to load this program with bad BTF anyways). So it
felt like extra warnings for such an unlikely scenario isn't
necessary.
>
> > +
> > + ctx_name = btf__name_by_offset(btf, t->name_off);
> > +
> > + if (guess_prog_type_by_ctx_name(ctx_name, &prog_type, &attach_type) == 0) {
> > + bpf_program__set_type(prog, prog_type);
> > + bpf_program__set_expected_attach_type(prog, attach_type);
> > +
> > + if (!env.quiet) {
> > + printf("Using guessed program type '%s' for %s/%s...\n",
> > + libbpf_bpf_prog_type_str(prog_type),
> > + filename, prog_name);
> > + }
> > + } else {
> > + if (!env.quiet) {
> > + printf("Failed to guess program type for freplace program with context type name '%s' for %s/%s. Consider using canonical type names to help veristat...\n",
> > + ctx_name, filename, prog_name);
> > + }
> > + }
> > + }
> > +skip_freplace_fixup:
> > + return;
> > }
> >
> > static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog)
> > {
> > const char *prog_name = bpf_program__name(prog);
> > + const char *base_filename = basename(filename);
> > size_t buf_sz = sizeof(verif_log_buf);
> > char *buf = verif_log_buf;
> > struct verif_stats *stats;
> > int err = 0;
> > void *tmp;
> >
> > - if (!should_process_file_prog(basename(filename), bpf_program__name(prog))) {
> > + if (!should_process_file_prog(base_filename, bpf_program__name(prog))) {
> > env.progs_skipped++;
> > return 0;
> > }
> > @@ -835,12 +948,12 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
> > verif_log_buf[0] = '\0';
> >
> > /* increase chances of successful BPF object loading */
> > - fixup_obj(obj);
> > + fixup_obj(obj, prog, base_filename);
> >
> > err = bpf_object__load(obj);
> > env.progs_processed++;
> >
> > - stats->file_name = strdup(basename(filename));
> > + stats->file_name = strdup(base_filename);
> > stats->prog_name = strdup(bpf_program__name(prog));
> > stats->stats[VERDICT] = err == 0; /* 1 - success, 0 - failure */
> > parse_verif_log(buf, buf_sz, stats);
>
