On Wed, Sep 9, 2020 at 11:25 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> From: YiFei Zhu <zhuyifei@xxxxxxxxxx>
>
> The patch adds a simple wrapper bpf_prog_bind_map around the syscall.
> When the libbpf tries to load a program, it will probe the kernel for
> the support of this syscall and unconditionally bind .rodata section
btw, you subject is out of sync, still mentions .metadata
> to the program.
>
> Cc: YiFei Zhu <zhuyifei1999@xxxxxxxxx>
> Signed-off-by: YiFei Zhu <zhuyifei@xxxxxxxxxx>
Please drop zhuyifei@xxxxxxxxxx from CC list (it's unreachable), when
you submit a new version.
> Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> ---
> tools/lib/bpf/bpf.c | 13 ++++++
> tools/lib/bpf/bpf.h | 8 ++++
> tools/lib/bpf/libbpf.c | 94 ++++++++++++++++++++++++++++++++--------
> tools/lib/bpf/libbpf.map | 1 +
> 4 files changed, 98 insertions(+), 18 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 82b983ff6569..5f6c5676cc45 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -872,3 +872,16 @@ int bpf_enable_stats(enum bpf_stats_type type)
>
> return sys_bpf(BPF_ENABLE_STATS, &attr, sizeof(attr));
> }
> +
> +int bpf_prog_bind_map(int prog_fd, int map_fd,
> + const struct bpf_prog_bind_opts *opts)
> +{
> + union bpf_attr attr;
> +
you forgot OPTS_VALID check here
> + memset(&attr, 0, sizeof(attr));
> + attr.prog_bind_map.prog_fd = prog_fd;
> + attr.prog_bind_map.map_fd = map_fd;
> + attr.prog_bind_map.flags = OPTS_GET(opts, flags, 0);
> +
> + return sys_bpf(BPF_PROG_BIND_MAP, &attr, sizeof(attr));
> +}
[...]
> -static int probe_kern_global_data(void)
> +static void probe_create_global_data(int *prog, int *map,
> + struct bpf_insn *insns, size_t insns_cnt)
> {
> struct bpf_load_program_attr prg_attr;
> struct bpf_create_map_attr map_attr;
> char *cp, errmsg[STRERR_BUFSIZE];
> - struct bpf_insn insns[] = {
> - BPF_LD_MAP_VALUE(BPF_REG_1, 0, 16),
> - BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 42),
> - BPF_MOV64_IMM(BPF_REG_0, 0),
> - BPF_EXIT_INSN(),
> - };
> - int ret, map;
> + int err;
>
> memset(&map_attr, 0, sizeof(map_attr));
> map_attr.map_type = BPF_MAP_TYPE_ARRAY;
> @@ -3748,26 +3749,40 @@ static int probe_kern_global_data(void)
> map_attr.value_size = 32;
> map_attr.max_entries = 1;
>
> - map = bpf_create_map_xattr(&map_attr);
> - if (map < 0) {
> - ret = -errno;
> - cp = libbpf_strerror_r(ret, errmsg, sizeof(errmsg));
> + *map = bpf_create_map_xattr(&map_attr);
> + if (*map < 0) {
> + err = errno;
> + cp = libbpf_strerror_r(err, errmsg, sizeof(errmsg));
> pr_warn("Error in %s():%s(%d). Couldn't create simple array map.\n",
> - __func__, cp, -ret);
> - return ret;
> + __func__, cp, -err);
> + return;
> }
>
> - insns[0].imm = map;
> + insns[0].imm = *map;
I think I already complained about this? You are assuming that
insns[0] is BPF_LD_MAP_VALUE, which is true only for one case out of
two already! It's just by luck that probe_prog_bind_map works because
the verifier ignores the exit code, apparently.
If this doesn't generalize well, don't generalize. But let's not do a
blind instruction rewrite, which will cause tons of confusion later.
>
> memset(&prg_attr, 0, sizeof(prg_attr));
> prg_attr.prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> prg_attr.insns = insns;
> - prg_attr.insns_cnt = ARRAY_SIZE(insns);
> + prg_attr.insns_cnt = insns_cnt;
> prg_attr.license = "GPL";
>
> - ret = bpf_load_program_xattr(&prg_attr, NULL, 0);
> + *prog = bpf_load_program_xattr(&prg_attr, NULL, 0);
> +}
> +
> +static int probe_kern_global_data(void)
> +{
> + struct bpf_insn insns[] = {
> + BPF_LD_MAP_VALUE(BPF_REG_1, 0, 16),
> + BPF_ST_MEM(BPF_DW, BPF_REG_1, 0, 42),
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + };
> + int prog = -1, map = -1;
> +
> + probe_create_global_data(&prog, &map, insns, ARRAY_SIZE(insns));
> +
> close(map);
> - return probe_fd(ret);
> + return probe_fd(prog);
> }
>
> static int probe_kern_btf(void)
> @@ -3894,6 +3909,32 @@ static int probe_kern_probe_read_kernel(void)
> return probe_fd(bpf_load_program_xattr(&attr, NULL, 0));
> }
>
> +static int probe_prog_bind_map(void)
> +{
> + struct bpf_insn insns[] = {
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + };
> + int prog = -1, map = -1, ret = 0;
> +
> + if (!kernel_supports(FEAT_GLOBAL_DATA))
> + return 0;
> +
> + probe_create_global_data(&prog, &map, insns, ARRAY_SIZE(insns));
> +
> + if (map >= 0 && prog < 0) {
> + close(map);
> + return 0;
> + }
> +
> + if (!bpf_prog_bind_map(prog, map, NULL))
> + ret = 1;
> +
> + close(map);
> + close(prog);
> + return ret;
> +}
> +
[...]
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 92ceb48a5ca2..0b7830f4ff8b 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -308,4 +308,5 @@ LIBBPF_0.2.0 {
> perf_buffer__epoll_fd;
> perf_buffer__consume_buffer;
> xsk_socket__create_shared;
> + bpf_prog_bind_map;
please keep this list sorted
> } LIBBPF_0.1.0;
> --
> 2.28.0.526.ge36021eeef-goog
>
