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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKH8qBtJoZpacMj+4sEJkt1BcQ1-oju=Z9SOVq-GxitUTJch=w@mail.gmail.com>
Date:   Fri, 11 Sep 2020 08:49:25 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        YiFei Zhu <zhuyifei1999@...il.com>
Subject: 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
<andrii.nakryiko@...il.com> wrote:
>
> On Wed, Sep 9, 2020 at 11:25 AM Stanislav Fomichev <sdf@...gle.com> wrote:
> >
> > From: YiFei Zhu <zhuyifei@...gle.com>
> >
> > 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@...il.com>
> > Signed-off-by: YiFei Zhu <zhuyifei@...gle.com>
>
> Please drop zhuyifei@...gle.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@...gle.com/zhuyifei1999@...il.com/ to make
it quiet.

> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > ---
> >  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!