Re: [PATCH bpf-next v4 3/5] libbpf: Add BPF_PROG_BIND_MAP syscall and use it on .metadata section

On Thu, Sep 10, 2020 at 12:41 PM Andrii Nakryiko
[off-list ref] wrote:

On Wed, Sep 9, 2020 at 11:25 AM Stanislav Fomichev [off-list ref] wrote:

quoted

From: YiFei Zhu <redacted>

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

Ooops, will fix, thanks!

quoted

to the program.

Cc: YiFei Zhu <redacted>
Signed-off-by: YiFei Zhu <redacted>

Please drop zhuyifei@google.com from CC list (it's unreachable), when
you submit a new version.

I think git-send-email automatically adds it because of the sign-off,
let me try to see if I can remove it. I guess I can just do
s/zhuyifei@google.com/zhuyifei1999@gmail.com/ to make
it quiet.

quoted

Signed-off-by: Stanislav Fomichev <redacted>
---
 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

Good point, will do!

quoted

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

I might have missed your previous comment, sorry about that.
Agreed, it might be easier to just copy-paste the original function
and explicitly change the insns.

[...]

quoted

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

Sure, will do!