On Thu, Sep 10, 2020 at 12:41 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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
Ooops, will fix, thanks!
> > 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.
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@xxxxxxxxxx/zhuyifei1999@xxxxxxxxx/ to make
it quiet.
> > 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
Good point, will do!
> > @@ -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.
> [...]
>
> > 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!
