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

[Eduard Zingerman <eddyz87@gmail.com>]

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