[PATCH v4 bpf-next 0/3] veristat: add better support of freplace programs

[PATCH v4 bpf-next 0/3] veristat: add better support of freplace programs

[Andrii Nakryiko]

Teach veristat how to deal with freplace BPF programs. As they can't be
directly loaded by veristat without custom user-space part that sets correct
target program FD, veristat always fails freplace programs. This patch set
teaches veristat to guess target program type that will be inherited by
freplace program itself, and subtitute it for BPF_PROG_TYPE_EXT (freplace) one
for the purposes of BPF verification.

Patch #1 fixes bug in libbpf preventing overriding freplace with specific
program type.

Patch #2 adds convenient -d flag to request veristat to emit libbpf debug
logs. It help debugging why a specific BPF program fails to load, if the
problem is not due to BPF verification itself.

v3->v4:
  - fix optional kern_name check when guessing prog type (Alexei);
v2->v3:
  - fix bpf_obj_id selftest that uses legacy bpf_prog_test_load() helper,
    which always sets program type programmatically; teach the helper to do it
    only if actually necessary (Stanislav);
v1->v2:
  - fix compilation error reported by old GCC (my GCC v11 doesn't produce even
    a warning) and Clang (see CI failure at [0]):

GCC version:

  veristat.c: In function ‘fixup_obj’:
  veristat.c:908:1: error: label at end of compound statement
    908 | skip_freplace_fixup:
        | ^~~~~~~~~~~~~~~~~~~

Clang version:

  veristat.c:909:1: error: label at end of compound statement is a C2x extension [-Werror,-Wc2x-extensions]
  }
  ^
  1 error generated.

  [0] https://github.com/kernel-patches/bpf/actions/runs/4515972059/jobs/7953845335

Andrii Nakryiko (3):
  libbpf: disassociate section handler on explicit
    bpf_program__set_type() call
  veristat: add -d debug mode option to see debug libbpf log
  veristat: guess and substitue underlying program type for freplace
    (EXT) progs

 tools/lib/bpf/libbpf.c                        |   1 +
 tools/testing/selftests/bpf/testing_helpers.c |   2 +-
 tools/testing/selftests/bpf/veristat.c        | 129 +++++++++++++++++-
 3 files changed, 126 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v4 bpf-next 1/3] libbpf: disassociate section handler on explicit bpf_program__set_type() call

[Andrii Nakryiko]

If user explicitly overrides programs's type with
bpf_program__set_type() API call, we need to disassociate whatever
SEC_DEF handler libbpf determined initially based on program's SEC()
definition, as it's not goind to be valid anymore and could lead to
crashes and/or confusing failures.

Also, fix up bpf_prog_test_load() helper in selftests/bpf, which is
force-setting program type (even if that's completely unnecessary; this
is quite a legacy piece of code), and thus should expect auto-attach to
not work, yet one of the tests explicitly relies on auto-attach for
testing.

Instead, force-set program type only if it differs from the desired one.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.c                        | 1 +
 tools/testing/selftests/bpf/testing_helpers.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 15737d7b5a28..49cd304ae3bc 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -8468,6 +8468,7 @@ int bpf_program__set_type(struct bpf_program *prog, enum bpf_prog_type type)
 		return libbpf_err(-EBUSY);
 
 	prog->type = type;
+	prog->sec_def = NULL;
 	return 0;
 }
 
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 6c44153755e6..ecfea13f938b 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -195,7 +195,7 @@ int bpf_prog_test_load(const char *file, enum bpf_prog_type type,
 		goto err_out;
 	}
 
-	if (type != BPF_PROG_TYPE_UNSPEC)
+	if (type != BPF_PROG_TYPE_UNSPEC && bpf_program__type(prog) != type)
 		bpf_program__set_type(prog, type);
 
 	flags = bpf_program__flags(prog) | BPF_F_TEST_RND_HI32;
-- 
2.34.1

[PATCH v4 bpf-next 2/3] veristat: add -d debug mode option to see debug libbpf log

[Andrii Nakryiko]

Add -d option to allow requesting libbpf debug logs from veristat.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/veristat.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index 83231456d3c5..263df32fbda8 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -135,6 +135,7 @@ static struct env {
 	char **filenames;
 	int filename_cnt;
 	bool verbose;
+	bool debug;
 	bool quiet;
 	int log_level;
 	enum resfmt out_fmt;
@@ -169,7 +170,7 @@ static int libbpf_print_fn(enum libbpf_print_level level, const char *format, va
 {
 	if (!env.verbose)
 		return 0;
-	if (level == LIBBPF_DEBUG /* && !env.verbose */)
+	if (level == LIBBPF_DEBUG  && !env.debug)
 		return 0;
 	return vfprintf(stderr, format, args);
 }
@@ -186,6 +187,7 @@ static const struct argp_option opts[] = {
 	{ NULL, 'h', NULL, OPTION_HIDDEN, "Show the full help" },
 	{ "verbose", 'v', NULL, 0, "Verbose mode" },
 	{ "log-level", 'l', "LEVEL", 0, "Verifier log level (default 0 for normal mode, 1 for verbose mode)" },
+	{ "debug", 'd', NULL, 0, "Debug mode (turns on libbpf debug logging)" },
 	{ "quiet", 'q', NULL, 0, "Quiet mode" },
 	{ "emit", 'e', "SPEC", 0, "Specify stats to be emitted" },
 	{ "sort", 's', "SPEC", 0, "Specify sort order" },
@@ -212,6 +214,10 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
 	case 'v':
 		env.verbose = true;
 		break;
+	case 'd':
+		env.debug = true;
+		env.verbose = true;
+		break;
 	case 'q':
 		env.quiet = true;
 		break;
-- 
2.34.1

[PATCH v4 bpf-next 3/3] veristat: guess and substitue underlying program type for freplace (EXT) progs

[Andrii Nakryiko]

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@kernel.org>
---
 tools/testing/selftests/bpf/veristat.c | 121 ++++++++++++++++++++++++-
 1 file changed, 117 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index 263df32fbda8..055df1abd7ca 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -15,6 +15,7 @@
 #include <sys/sysinfo.h>
 #include <sys/stat.h>
 #include <bpf/libbpf.h>
+#include <bpf/btf.h>
 #include <libelf.h>
 #include <gelf.h>
 #include <float.h>
@@ -778,7 +779,62 @@ static int parse_verif_log(char * const buf, size_t buf_sz, struct verif_stats *
 	return 0;
 }
 
-static void fixup_obj(struct bpf_object *obj)
+static int guess_prog_type_by_ctx_name(const char *ctx_name,
+				       enum bpf_prog_type *prog_type,
+				       enum bpf_attach_type *attach_type)
+{
+	/* We need to guess program type based on its declared context type.
+	 * This guess can't be perfect as many different program types might
+	 * share the same context type.  So we can only hope to reasonably
+	 * well guess this and get lucky.
+	 *
+	 * Just in case, we support both UAPI-side type names and
+	 * kernel-internal names.
+	 */
+	static struct {
+		const char *uapi_name;
+		const char *kern_name;
+		enum bpf_prog_type prog_type;
+		enum bpf_attach_type attach_type;
+	} ctx_map[] = {
+		/* __sk_buff is most ambiguous, for now we assume cgroup_skb */
+		{ "__sk_buff", "sk_buff", BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_INGRESS },
+		{ "bpf_sock", "sock", BPF_PROG_TYPE_CGROUP_SOCK, BPF_CGROUP_INET4_POST_BIND },
+		{ "bpf_sock_addr", "bpf_sock_addr_kern",  BPF_PROG_TYPE_CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_BIND },
+		{ "bpf_sock_ops", "bpf_sock_ops_kern", BPF_PROG_TYPE_SOCK_OPS, BPF_CGROUP_SOCK_OPS },
+		{ "sk_msg_md", "sk_msg", BPF_PROG_TYPE_SK_MSG, BPF_SK_MSG_VERDICT },
+		{ "bpf_cgroup_dev_ctx", "bpf_cgroup_dev_ctx", BPF_PROG_TYPE_CGROUP_DEVICE, BPF_CGROUP_DEVICE },
+		{ "bpf_sysctl", "bpf_sysctl_kern", BPF_PROG_TYPE_CGROUP_SYSCTL, BPF_CGROUP_SYSCTL },
+		{ "bpf_sockopt", "bpf_sockopt_kern", BPF_PROG_TYPE_CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT },
+		{ "sk_reuseport_md", "sk_reuseport_kern", BPF_PROG_TYPE_SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT_OR_MIGRATE },
+		{ "bpf_sk_lookup", "bpf_sk_lookup_kern", BPF_PROG_TYPE_SK_LOOKUP, BPF_SK_LOOKUP },
+		{ "xdp_md", "xdp_buff", BPF_PROG_TYPE_XDP, BPF_XDP },
+		/* tracing types with no expected attach type */
+		{ "bpf_user_pt_regs_t", "pt_regs", BPF_PROG_TYPE_KPROBE },
+		{ "bpf_perf_event_data", "bpf_perf_event_data_kern", BPF_PROG_TYPE_PERF_EVENT },
+		/* raw_tp programs use u64[] from kernel side, we don't want
+		 * to match on that, probably; so NULL for kern-side type
+		 */
+		{ "bpf_raw_tracepoint_args", NULL, BPF_PROG_TYPE_RAW_TRACEPOINT },
+	};
+	int i;
+
+	if (!ctx_name)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(ctx_map); i++) {
+		if (strcmp(ctx_map[i].uapi_name, ctx_name) == 0 ||
+		    (ctx_map[i].kern_name && strcmp(ctx_map[i].kern_name, ctx_name) == 0)) {
+			*prog_type = ctx_map[i].prog_type;
+			*attach_type = ctx_map[i].attach_type;
+			return 0;
+		}
+	}
+
+	return -ESRCH;
+}
+
+static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const char *filename)
 {
 	struct bpf_map *map;
 
@@ -798,18 +854,75 @@ static void fixup_obj(struct bpf_object *obj)
 				bpf_map__set_max_entries(map, 1);
 		}
 	}
+
+	/* SEC(freplace) programs can't be loaded with veristat as is,
+	 * but we can try guessing their target program's expected type by
+	 * looking at the type of program's first argument and substituting
+	 * corresponding program type
+	 */
+	if (bpf_program__type(prog) == BPF_PROG_TYPE_EXT) {
+		const struct btf *btf = bpf_object__btf(obj);
+		const char *prog_name = bpf_program__name(prog);
+		enum bpf_prog_type prog_type;
+		enum bpf_attach_type attach_type;
+		const struct btf_type *t;
+		const char *ctx_name;
+		int id;
+
+		if (!btf)
+			goto skip_freplace_fixup;
+
+		id = btf__find_by_name_kind(btf, prog_name, BTF_KIND_FUNC);
+		t = btf__type_by_id(btf, id);
+		t = btf__type_by_id(btf, t->type);
+		if (!btf_is_func_proto(t) || btf_vlen(t) != 1)
+			goto skip_freplace_fixup;
+
+		/* 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;
+
+		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);
-- 
2.34.1

Re: [PATCH v4 bpf-next 0/3] veristat: add better support of freplace programs

[Stanislav Fomichev]

On 03/27, Andrii Nakryiko wrote:
> Teach veristat how to deal with freplace BPF programs. As they can't be
> directly loaded by veristat without custom user-space part that sets  
> correct
> target program FD, veristat always fails freplace programs. This patch set
> teaches veristat to guess target program type that will be inherited by
> freplace program itself, and subtitute it for BPF_PROG_TYPE_EXT  
> (freplace) one
> for the purposes of BPF verification.

Acked-by: Stanislav Fomichev <sdf@google.com>

> Patch #1 fixes bug in libbpf preventing overriding freplace with specific
> program type.

> Patch #2 adds convenient -d flag to request veristat to emit libbpf debug
> logs. It help debugging why a specific BPF program fails to load, if the
> problem is not due to BPF verification itself.

> v3->v4:
>    - fix optional kern_name check when guessing prog type (Alexei);
> v2->v3:
>    - fix bpf_obj_id selftest that uses legacy bpf_prog_test_load() helper,
>      which always sets program type programmatically; teach the helper to  
> do it
>      only if actually necessary (Stanislav);
> v1->v2:
>    - fix compilation error reported by old GCC (my GCC v11 doesn't produce  
> even
>      a warning) and Clang (see CI failure at [0]):

> GCC version:

>    veristat.c: In function ‘fixup_obj’:
>    veristat.c:908:1: error: label at end of compound statement
>      908 | skip_freplace_fixup:
>          | ^~~~~~~~~~~~~~~~~~~

> Clang version:

>    veristat.c:909:1: error: label at end of compound statement is a C2x  
> extension [-Werror,-Wc2x-extensions]
>    }
>    ^
>    1 error generated.

>    [0]  
> https://github.com/kernel-patches/bpf/actions/runs/4515972059/jobs/7953845335

> Andrii Nakryiko (3):
>    libbpf: disassociate section handler on explicit
>      bpf_program__set_type() call
>    veristat: add -d debug mode option to see debug libbpf log
>    veristat: guess and substitue underlying program type for freplace
>      (EXT) progs

>   tools/lib/bpf/libbpf.c                        |   1 +
>   tools/testing/selftests/bpf/testing_helpers.c |   2 +-
>   tools/testing/selftests/bpf/veristat.c        | 129 +++++++++++++++++-
>   3 files changed, 126 insertions(+), 6 deletions(-)

> --
> 2.34.1

Re: [PATCH v4 bpf-next 2/3] veristat: add -d debug mode option to see debug libbpf log

[Eduard Zingerman]

On Mon, 2023-03-27 at 11:52 -0700, Andrii Nakryiko wrote:
> Add -d option to allow requesting libbpf debug logs from veristat.
> 
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/testing/selftests/bpf/veristat.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 83231456d3c5..263df32fbda8 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -135,6 +135,7 @@ static struct env {
>  	char **filenames;
>  	int filename_cnt;
>  	bool verbose;
> +	bool debug;
>  	bool quiet;
>  	int log_level;

Nitpick:
  it is now three booleans that control verbosity level, would it be
  better to use numerical level instead?

>  	enum resfmt out_fmt;
> @@ -169,7 +170,7 @@ static int libbpf_print_fn(enum libbpf_print_level level, const char *format, va
>  {
>  	if (!env.verbose)
>  		return 0;
> -	if (level == LIBBPF_DEBUG /* && !env.verbose */)
> +	if (level == LIBBPF_DEBUG  && !env.debug)
>  		return 0;
>  	return vfprintf(stderr, format, args);
>  }
> @@ -186,6 +187,7 @@ static const struct argp_option opts[] = {
>  	{ NULL, 'h', NULL, OPTION_HIDDEN, "Show the full help" },
>  	{ "verbose", 'v', NULL, 0, "Verbose mode" },
>  	{ "log-level", 'l', "LEVEL", 0, "Verifier log level (default 0 for normal mode, 1 for verbose mode)" },
> +	{ "debug", 'd', NULL, 0, "Debug mode (turns on libbpf debug logging)" },
>  	{ "quiet", 'q', NULL, 0, "Quiet mode" },
>  	{ "emit", 'e', "SPEC", 0, "Specify stats to be emitted" },
>  	{ "sort", 's', "SPEC", 0, "Specify sort order" },
> @@ -212,6 +214,10 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
>  	case 'v':
>  		env.verbose = true;
>  		break;
> +	case 'd':
> +		env.debug = true;
> +		env.verbose = true;
> +		break;
>  	case 'q':
>  		env.quiet = true;
>  		break;

Re: [PATCH v4 bpf-next 2/3] veristat: add -d debug mode option to see debug libbpf log

[Andrii Nakryiko]

On Wed, Mar 29, 2023 at 10:37 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2023-03-27 at 11:52 -0700, Andrii Nakryiko wrote:
> > Add -d option to allow requesting libbpf debug logs from veristat.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > ---
> >  tools/testing/selftests/bpf/veristat.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> > index 83231456d3c5..263df32fbda8 100644
> > --- a/tools/testing/selftests/bpf/veristat.c
> > +++ b/tools/testing/selftests/bpf/veristat.c
> > @@ -135,6 +135,7 @@ static struct env {
> >       char **filenames;
> >       int filename_cnt;
> >       bool verbose;
> > +     bool debug;
> >       bool quiet;
> >       int log_level;
>
> Nitpick:
>   it is now three booleans that control verbosity level, would it be
>   better to use numerical level instead?
>

I don't think so, because bool fields make checks cleaner in specific
places in the code.

> >       enum resfmt out_fmt;
> > @@ -169,7 +170,7 @@ static int libbpf_print_fn(enum libbpf_print_level level, const char *format, va
> >  {
> >       if (!env.verbose)
> >               return 0;
> > -     if (level == LIBBPF_DEBUG /* && !env.verbose */)
> > +     if (level == LIBBPF_DEBUG  && !env.debug)
> >               return 0;
> >       return vfprintf(stderr, format, args);
> >  }
> > @@ -186,6 +187,7 @@ static const struct argp_option opts[] = {
> >       { NULL, 'h', NULL, OPTION_HIDDEN, "Show the full help" },
> >       { "verbose", 'v', NULL, 0, "Verbose mode" },
> >       { "log-level", 'l', "LEVEL", 0, "Verifier log level (default 0 for normal mode, 1 for verbose mode)" },
> > +     { "debug", 'd', NULL, 0, "Debug mode (turns on libbpf debug logging)" },
> >       { "quiet", 'q', NULL, 0, "Quiet mode" },
> >       { "emit", 'e', "SPEC", 0, "Specify stats to be emitted" },
> >       { "sort", 's', "SPEC", 0, "Specify sort order" },
> > @@ -212,6 +214,10 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
> >       case 'v':
> >               env.verbose = true;
> >               break;
> > +     case 'd':
> > +             env.debug = true;
> > +             env.verbose = true;
> > +             break;
> >       case 'q':
> >               env.quiet = true;
> >               break;
>

Re: [PATCH v4 bpf-next 3/3] veristat: guess and substitue underlying program type for freplace (EXT) progs

[Eduard Zingerman]

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@kernel.org>

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@gmail.com>

> ---
>  tools/testing/selftests/bpf/veristat.c | 121 ++++++++++++++++++++++++-
>  1 file changed, 117 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 263df32fbda8..055df1abd7ca 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -15,6 +15,7 @@
>  #include <sys/sysinfo.h>
>  #include <sys/stat.h>
>  #include <bpf/libbpf.h>
> +#include <bpf/btf.h>
>  #include <libelf.h>
>  #include <gelf.h>
>  #include <float.h>
> @@ -778,7 +779,62 @@ static int parse_verif_log(char * const buf, size_t buf_sz, struct verif_stats *
>  	return 0;
>  }
>  
> -static void fixup_obj(struct bpf_object *obj)
> +static int guess_prog_type_by_ctx_name(const char *ctx_name,
> +				       enum bpf_prog_type *prog_type,
> +				       enum bpf_attach_type *attach_type)
> +{
> +	/* We need to guess program type based on its declared context type.
> +	 * This guess can't be perfect as many different program types might
> +	 * share the same context type.  So we can only hope to reasonably
> +	 * well guess this and get lucky.
> +	 *
> +	 * Just in case, we support both UAPI-side type names and
> +	 * kernel-internal names.
> +	 */
> +	static struct {
> +		const char *uapi_name;
> +		const char *kern_name;
> +		enum bpf_prog_type prog_type;
> +		enum bpf_attach_type attach_type;
> +	} ctx_map[] = {
> +		/* __sk_buff is most ambiguous, for now we assume cgroup_skb */
> +		{ "__sk_buff", "sk_buff", BPF_PROG_TYPE_CGROUP_SKB, BPF_CGROUP_INET_INGRESS },
> +		{ "bpf_sock", "sock", BPF_PROG_TYPE_CGROUP_SOCK, BPF_CGROUP_INET4_POST_BIND },
> +		{ "bpf_sock_addr", "bpf_sock_addr_kern",  BPF_PROG_TYPE_CGROUP_SOCK_ADDR, BPF_CGROUP_INET4_BIND },
> +		{ "bpf_sock_ops", "bpf_sock_ops_kern", BPF_PROG_TYPE_SOCK_OPS, BPF_CGROUP_SOCK_OPS },
> +		{ "sk_msg_md", "sk_msg", BPF_PROG_TYPE_SK_MSG, BPF_SK_MSG_VERDICT },
> +		{ "bpf_cgroup_dev_ctx", "bpf_cgroup_dev_ctx", BPF_PROG_TYPE_CGROUP_DEVICE, BPF_CGROUP_DEVICE },
> +		{ "bpf_sysctl", "bpf_sysctl_kern", BPF_PROG_TYPE_CGROUP_SYSCTL, BPF_CGROUP_SYSCTL },
> +		{ "bpf_sockopt", "bpf_sockopt_kern", BPF_PROG_TYPE_CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT },
> +		{ "sk_reuseport_md", "sk_reuseport_kern", BPF_PROG_TYPE_SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT_OR_MIGRATE },
> +		{ "bpf_sk_lookup", "bpf_sk_lookup_kern", BPF_PROG_TYPE_SK_LOOKUP, BPF_SK_LOOKUP },
> +		{ "xdp_md", "xdp_buff", BPF_PROG_TYPE_XDP, BPF_XDP },
> +		/* tracing types with no expected attach type */
> +		{ "bpf_user_pt_regs_t", "pt_regs", BPF_PROG_TYPE_KPROBE },
> +		{ "bpf_perf_event_data", "bpf_perf_event_data_kern", BPF_PROG_TYPE_PERF_EVENT },
> +		/* raw_tp programs use u64[] from kernel side, we don't want
> +		 * to match on that, probably; so NULL for kern-side type
> +		 */
> +		{ "bpf_raw_tracepoint_args", NULL, BPF_PROG_TYPE_RAW_TRACEPOINT },
> +	};
> +	int i;
> +
> +	if (!ctx_name)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(ctx_map); i++) {
> +		if (strcmp(ctx_map[i].uapi_name, ctx_name) == 0 ||
> +		    (ctx_map[i].kern_name && strcmp(ctx_map[i].kern_name, ctx_name) == 0)) {
> +			*prog_type = ctx_map[i].prog_type;
> +			*attach_type = ctx_map[i].attach_type;
> +			return 0;
> +		}
> +	}
> +
> +	return -ESRCH;
> +}
> +
> +static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const char *filename)
>  {
>  	struct bpf_map *map;
>  
> @@ -798,18 +854,75 @@ static void fixup_obj(struct bpf_object *obj)
>  				bpf_map__set_max_entries(map, 1);
>  		}
>  	}
> +
> +	/* SEC(freplace) programs can't be loaded with veristat as is,
> +	 * but we can try guessing their target program's expected type by
> +	 * looking at the type of program's first argument and substituting
> +	 * corresponding program type
> +	 */
> +	if (bpf_program__type(prog) == BPF_PROG_TYPE_EXT) {
> +		const struct btf *btf = bpf_object__btf(obj);
> +		const char *prog_name = bpf_program__name(prog);
> +		enum bpf_prog_type prog_type;
> +		enum bpf_attach_type attach_type;
> +		const struct btf_type *t;
> +		const char *ctx_name;
> +		int id;
> +
> +		if (!btf)
> +			goto skip_freplace_fixup;
> +
> +		id = btf__find_by_name_kind(btf, prog_name, BTF_KIND_FUNC);
> +		t = btf__type_by_id(btf, id);
> +		t = btf__type_by_id(btf, t->type);
> +		if (!btf_is_func_proto(t) || btf_vlen(t) != 1)
> +			goto skip_freplace_fixup;
> +
> +		/* 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.

> +
> +		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);

Re: [PATCH v4 bpf-next 0/3] veristat: add better support of freplace programs

[patchwork-bot+netdevbpf]

Hello:

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

On Mon, 27 Mar 2023 11:51:59 -0700 you wrote:
> Teach veristat how to deal with freplace BPF programs. As they can't be
> directly loaded by veristat without custom user-space part that sets correct
> target program FD, veristat always fails freplace programs. This patch set
> teaches veristat to guess target program type that will be inherited by
> freplace program itself, and subtitute it for BPF_PROG_TYPE_EXT (freplace) one
> for the purposes of BPF verification.
> 
> [...]

Here is the summary with links:
  - [v4,bpf-next,1/3] libbpf: disassociate section handler on explicit bpf_program__set_type() call
    https://git.kernel.org/bpf/bpf-next/c/d6e6286a12e7
  - [v4,bpf-next,2/3] veristat: add -d debug mode option to see debug libbpf log
    https://git.kernel.org/bpf/bpf-next/c/b3c63d7ad81a
  - [v4,bpf-next,3/3] veristat: guess and substitue underlying program type for freplace (EXT) progs
    https://git.kernel.org/bpf/bpf-next/c/fa7cc9062087

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

Re: [PATCH v4 bpf-next 3/3] veristat: guess and substitue underlying program type for freplace (EXT) progs

[Alexei Starovoitov]

On Wed, Mar 29, 2023 at 11:36 AM Eduard Zingerman <eddyz87@gmail.com> 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@kernel.org>
>
> 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@gmail.com>

Thanks for testing and important feedback.
I've applied the set. The nits can be addressed in the follow up.

What do you have in mind as 'more permissive attach' ?
What are those 15 out of 18 with invalid exit code?
What kind of attach_type will help?

Re: [PATCH v4 bpf-next 3/3] veristat: guess and substitue underlying program type for freplace (EXT) progs

[Eduard Zingerman]

On Wed, 2023-03-29 at 22:38 -0700, Alexei Starovoitov wrote:
> On Wed, Mar 29, 2023 at 11:36 AM Eduard Zingerman <eddyz87@gmail.com> 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@kernel.org>
> > 
> > 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@gmail.com>
> 
> Thanks for testing and important feedback.
> I've applied the set. The nits can be addressed in the follow up.
> 
> What do you have in mind as 'more permissive attach' ?
> What are those 15 out of 18 with invalid exit code?
> What kind of attach_type will help?

TLDR: I apologize, it was a bad comment.
      Should have done more analysis yesterday and withheld from commenting.

---

Inspected each program and it turned out that most of the failing ones
are either not programs but separate functions, or have __skb_buf parameter.
The summary table is at the end of the email, here is the list of those
that should load but fail:

- Have __skb_buf parameter and attach to SEC("tc")
  - fexit_bpf2bpf.bpf.o:new_get_skb_len
  - freplace_cls_redirect.bpf.o:freplace_cls_redirect_test
  - freplace_global_func.bpf.o:new_test_pkt_access
- Need ifindex to be specified prior loading:
  - xdp_metadata2.bpf.o:freplace_rx
                                                                            Should
File                            Program                            Verdict  fail?   Reason
------------------------------  ---------------------------------  -------  ------  -----------------------
fexit_bpf2bpf.bpf.o             new_get_constant                   failure      no  Function, not a program
fexit_bpf2bpf.bpf.o             new_get_skb_ifindex                failure      no  Function, not a program
fexit_bpf2bpf.bpf.o             new_get_skb_len                    failure      no  __sk_buff parameter
fexit_bpf2bpf.bpf.o             new_test_pkt_write_access_subprog  failure      no  Function, not a program
fexit_bpf2bpf.bpf.o             test_main                          failure      no  Function, not a program
fexit_bpf2bpf.bpf.o             test_subprog1                      failure      no  Function, not a program
fexit_bpf2bpf.bpf.o             test_subprog2                      failure      no  Function, not a program
fexit_bpf2bpf.bpf.o             test_subprog3                      failure      no  Function, not a program
freplace_attach_probe.bpf.o     new_handle_kprobe                  failure     yes
freplace_cls_redirect.bpf.o     freplace_cls_redirect_test         failure      no  __sk_buff parameter
freplace_connect4.bpf.o         new_do_bind                        failure     yes 
freplace_connect_v4_prog.bpf.o  new_connect_v4_prog                failure     yes
freplace_get_constant.bpf.o     security_new_get_constant          failure      no  Function, not a program
freplace_global_func.bpf.o      new_test_pkt_access                failure      no  __sk_buff parameter
freplace_progmap.bpf.o          xdp_cpumap_prog                    success
freplace_progmap.bpf.o          xdp_drop_prog                      success
test_trace_ext.bpf.o            test_pkt_md_access_new             success
xdp_metadata2.bpf.o             freplace_rx                        failure      no  needs ifindex
------------------------------  ---------------------------------  -------  ------- -----------------------

Re: [PATCH v4 bpf-next 3/3] veristat: guess and substitue underlying program type for freplace (EXT) progs

[Andrii Nakryiko]

On Wed, Mar 29, 2023 at 11:36 AM Eduard Zingerman <eddyz87@gmail.com> 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@kernel.org>
>
> 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@gmail.com>
>
> > ---
> >  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);
>

Re: [PATCH v4 bpf-next 3/3] veristat: guess and substitue underlying program type for freplace (EXT) progs

[Andrii Nakryiko]

On Thu, Mar 30, 2023 at 7:49 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Wed, 2023-03-29 at 22:38 -0700, Alexei Starovoitov wrote:
> > On Wed, Mar 29, 2023 at 11:36 AM Eduard Zingerman <eddyz87@gmail.com> 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@kernel.org>
> > >
> > > 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@gmail.com>
> >
> > Thanks for testing and important feedback.
> > I've applied the set. The nits can be addressed in the follow up.
> >
> > What do you have in mind as 'more permissive attach' ?
> > What are those 15 out of 18 with invalid exit code?
> > What kind of attach_type will help?
>
> TLDR: I apologize, it was a bad comment.
>       Should have done more analysis yesterday and withheld from commenting.
>
> ---
>
> Inspected each program and it turned out that most of the failing ones
> are either not programs but separate functions, or have __skb_buf parameter.
> The summary table is at the end of the email, here is the list of those
> that should load but fail:
>
> - Have __skb_buf parameter and attach to SEC("tc")
>   - fexit_bpf2bpf.bpf.o:new_get_skb_len
>   - freplace_cls_redirect.bpf.o:freplace_cls_redirect_test
>   - freplace_global_func.bpf.o:new_test_pkt_access
> - Need ifindex to be specified prior loading:
>   - xdp_metadata2.bpf.o:freplace_rx

Thanks for compiling the table! I went through it and left comments.
skbuff programs do work now, but xdp_metadata2 is harder case, I don't
think it will work, as it requires driver support.

>                                                                             Should
> File                            Program                            Verdict  fail?   Reason
> ------------------------------  ---------------------------------  -------  ------  -----------------------
> fexit_bpf2bpf.bpf.o             new_get_constant                   failure      no  Function, not a program

not much that can be done here, it's just `long` argument...

> fexit_bpf2bpf.bpf.o             new_get_skb_ifindex                failure      no  Function, not a program

Yeah, this heuristic of substituting the original program type won't
work when subprog uses more than one argument and/or if the first
argument is not the context type argument, unfortunately.


> fexit_bpf2bpf.bpf.o             new_get_skb_len                    failure      no  __sk_buff parameter

Right.. I'll set SCHED_CLS as a better guess for __sk_buff. It doesn't
seem to regress anything for Meta-specific programs I tried it
on.cI'll switch it over, unless others have opinions about SCHED_CLS
vs CGROUP_SKB. I'm not well versed in all the __sk_buff-using program
type differences, unfortunately.

This one now works with SCHED_CLS now.


> fexit_bpf2bpf.bpf.o             new_test_pkt_write_access_subprog  failure      no  Function, not a program

yep, subprog, can't work

> fexit_bpf2bpf.bpf.o             test_main                          failure      no  Function, not a program
> fexit_bpf2bpf.bpf.o             test_subprog1                      failure      no  Function, not a program
> fexit_bpf2bpf.bpf.o             test_subprog2                      failure      no  Function, not a program
> fexit_bpf2bpf.bpf.o             test_subprog3                      failure      no  Function, not a program

These are actually fexit programs. Currently they are assumed to be
fexit of kernel function. These programs are using BPF_PROG() macro,
they really have a u64[] context type. Maybe there is something to be
done here to improve the situation, but I'm not yet planning to look
into this.


> freplace_attach_probe.bpf.o     new_handle_kprobe                  failure     yes

guessing kprobe correctly, yep

> freplace_cls_redirect.bpf.o     freplace_cls_redirect_test         failure      no  __sk_buff parameter

Now succeeds with SCHED_CLS.

> freplace_connect4.bpf.o         new_do_bind                        failure     yes
> freplace_connect_v4_prog.bpf.o  new_connect_v4_prog                failure     yes

yep, guessed correctly CGROUP_SOCK_ADDR, but designed to fail

> freplace_get_constant.bpf.o     security_new_get_constant          failure      no  Function, not a program

same as new_get_constant, I don't think we can do better

> freplace_global_func.bpf.o      new_test_pkt_access                failure      no  __sk_buff parameter

Now works.

> freplace_progmap.bpf.o          xdp_cpumap_prog                    success
> freplace_progmap.bpf.o          xdp_drop_prog                      success
> test_trace_ext.bpf.o            test_pkt_md_access_new             success

Cool, handled.

> xdp_metadata2.bpf.o             freplace_rx                        failure      no  needs ifindex

this is a new fancy bpf_dev_bound_kfunc_check() stuff, I don't think
it can be easily solved (I tried substituting IFINDEX_LO and that
didn't work, of course).

> ------------------------------  ---------------------------------  -------  ------- -----------------------