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  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]
Date:   Wed, 2 Sep 2020 19:31:33 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Stanislav Fomichev <sdf@...gle.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 <zhuyifei@...gle.com>,
        YiFei Zhu <zhuyifei1999@...il.com>
Subject: Re: [PATCH bpf-next v3 3/8] libbpf: Add BPF_PROG_BIND_MAP syscall and
 use it on .metadata section

On Fri, Aug 28, 2020 at 12:37 PM 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.
> And when using libbpf to load a program, it will probe the kernel for
> the support of this syscall, and scan for the .metadata ELF section
> and load it as an internal map like a .data section.
>
> In the case that kernel supports the BPF_PROG_BIND_MAP syscall and
> a .metadata section exists, the map will be explicitly bound to
> the program via the syscall immediately after program is loaded.
> -EEXIST is ignored for this syscall.

Here is the question I have. How important is it that all this
metadata is in a separate map? What if libbpf just PROG_BIND_MAP all
the maps inside a single BPF .o file to all BPF programs in that file?
Including ARRAY maps created for .data, .rodata and .bss, even if the
BPF program doesn't use any of the global variables? If it's too
extreme, we could do it only for global data maps, leaving explicit
map definitions in SEC(".maps") alone. Would that be terrible?
Conceptually it makes sense, because when you program in user-space,
you expect global variables to be there, even if you don't reference
it directly, right? The only downside is that you won't have a special
".metadata" map, rather it will be part of ".rodata" one.


>
> Cc: YiFei Zhu <zhuyifei1999@...il.com>
> Signed-off-by: YiFei Zhu <zhuyifei@...gle.com>
> 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   | 130 ++++++++++++++++++++++++++++++++-------
>  tools/lib/bpf/libbpf.map |   1 +
>  4 files changed, 131 insertions(+), 21 deletions(-)
>

[...]

> @@ -3592,18 +3619,13 @@ static int probe_kern_prog_name(void)
>         return probe_fd(ret);
>  }
>
> -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)

all those static functions are internal, no need for double underscore in names

>  {
>         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;
> @@ -3611,26 +3633,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;

you are making confusing and error prone assumptions about the first
instruction passed in, I really-really don't like this, super easy to
miss and super easy to screw up.

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

[...]

Powered by blists - more mailing lists