| 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
| ||
|
Message-ID: <20150713195109.GE2885@kernel.org>
Date: Mon, 13 Jul 2015 16:51:09 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: "Wangnan (F)" <wangnan0@...wei.com>
Cc: ast@...mgrid.com, linux-kernel@...r.kernel.org, lizefan@...wei.com,
hekuang@...wei.com, xiakaixu@...wei.com, pi3orama@....com
Subject: Re: [PATCH 02/39] bpf tools: Collect eBPF programs from their own
sections
Em Fri, Jul 10, 2015 at 11:07:53AM +0800, Wangnan (F) escreveu:
> On 2015/7/9 23:58, Arnaldo Carvalho de Melo wrote:
> >Em Thu, Jul 09, 2015 at 12:35:05PM +0000, Wang Nan escreveu:
> >>This patch collects all programs in an object file into an array of
> >>'struct bpf_program' for further processing. That structure is for
> >>representing each eBPF program. 'bpf_prog' should be a better name, but
> >>it has been used by linux/filter.h. Although it is a kernel space name,
> >>I still prefer to call it 'bpf_program' to prevent possible confusion.
> >>
> >>bpf_program__new() creates a new 'struct bpf_program' object. It first
> >>init a variable in stack using __bpf_program__new(), then if success,
> >>enlarges obj->programs array and copy the new object in.
> >>
> >>Signed-off-by: Wang Nan <wangnan0@...wei.com>
> >>Acked-by: Alexei Starovoitov <ast@...mgrid.com>
> >>Cc: Brendan Gregg <brendan.d.gregg@...il.com>
> >>Cc: Daniel Borkmann <daniel@...earbox.net>
> >>Cc: David Ahern <dsahern@...il.com>
> >>Cc: He Kuang <hekuang@...wei.com>
> >>Cc: Jiri Olsa <jolsa@...nel.org>
> >>Cc: Kaixu Xia <xiakaixu@...wei.com>
> >>Cc: Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
> >>Cc: Namhyung Kim <namhyung@...nel.org>
> >>Cc: Paul Mackerras <paulus@...ba.org>
> >>Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> >>Cc: Zefan Li <lizefan@...wei.com>
> >>Cc: pi3orama@....com
> >>Link: http://lkml.kernel.org/r/1435716878-189507-13-git-send-email-wangnan0@huawei.com
> >>Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>
> >>---
> >> tools/lib/bpf/libbpf.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 117 insertions(+)
> >>
> >>diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >>index 9b016c0..3b717de 100644
> >>--- a/tools/lib/bpf/libbpf.c
> >>+++ b/tools/lib/bpf/libbpf.c
> >>@@ -78,12 +78,27 @@ void libbpf_set_print(libbpf_print_fn_t warn,
> >> # define LIBBPF_ELF_C_READ_MMAP ELF_C_READ
> >> #endif
> >>+/*
> >>+ * bpf_prog should be a better name but it has been used in
> >>+ * linux/filter.h.
> >>+ */
> >>+struct bpf_program {
> >>+ /* Index in elf obj file, for relocation use. */
> >>+ int idx;
> >>+ char *section_name;
> >>+ struct bpf_insn *insns;
> >>+ size_t insns_cnt;
> >>+};
> >>+
> >> struct bpf_object {
> >> char license[64];
> >> u32 kern_version;
> >> void *maps_buf;
> >> size_t maps_buf_sz;
> >>+ struct bpf_program *programs;
> >>+ size_t nr_programs;
> >>+
> >> /*
> >> * Information when doing elf related work. Only valid if fd
> >> * is valid.
> >>@@ -100,6 +115,84 @@ struct bpf_object {
> >> };
> >> #define obj_elf_valid(o) ((o)->efile.elf)
> >>+static void bpf_program__clear(struct bpf_program *prog)
> >>+{
> >>+ if (!prog)
> >>+ return;
> >>+
> >>+ zfree(&prog->section_name);
> >>+ zfree(&prog->insns);
> >>+ prog->insns_cnt = 0;
> >>+ prog->idx = -1;
> >>+}
> >So in perf land we use 'bpf_program__exit()' as the counterpart of
> >bpf_program__init(), i.e. one just initializes fields, allocating
> >memory for 'struct bpf_program' members, but does not allocates the
> >struct bpf_program itself, because sometimes we embed it inside other
> >structs, or we have it in arrays, as you do.
> >
> >So, to keep that convention, please rename bpf_program__clear() to
> >bpf_program__exit() and the next function, __bpf_program__new() to
> >bpf_program__init(), with 'struct bpf_program *prog' as the first
> >parameter.
> >
> >To speed things up, from now on, when I see such stuff, I will do the
> >changes, put them in a branch with a commiter note, and wait for your
> >Ack (or not, if you disagree with something).
> >
> >One more comment below.
> >
> >>+
> >>+static int
> >>+__bpf_program__new(void *data, size_t size, char *name, int idx,
> >>+ struct bpf_program *prog)
> >>+{
> >>+ if (size < sizeof(struct bpf_insn)) {
> >>+ pr_warning("corrupted section '%s'\n", name);
> >>+ return -EINVAL;
> >>+ }
> >>+
> >>+ bzero(prog, sizeof(*prog));
> >>+
> >>+ prog->section_name = strdup(name);
> >>+ if (!prog->section_name) {
> >>+ pr_warning("failed to alloc name for prog %s\n",
> >>+ name);
> >>+ goto errout;
> >>+ }
> >>+
> >>+ prog->insns = malloc(size);
> >>+ if (!prog->insns) {
> >>+ pr_warning("failed to alloc insns for %s\n", name);
> >>+ goto errout;
> >>+ }
> >>+ prog->insns_cnt = size / sizeof(struct bpf_insn);
> >>+ memcpy(prog->insns, data,
> >>+ prog->insns_cnt * sizeof(struct bpf_insn));
> >>+ prog->idx = idx;
> >>+
> >>+ return 0;
> >>+errout:
> >>+ bpf_program__clear(prog);
> >>+ return -ENOMEM;
> >>+}
> >>+
> >>+static struct bpf_program *
> >>+bpf_program__new(struct bpf_object *obj, void *data, size_t size,
> >>+ char *name, int idx)
> >This, as well, is not a 'bpf_program' method, it is a 'struct
> >bpf_object' one, that will manipulate 'struct bpf_object' internal
> >state, changing its struct members to get space for an extra bpf_program
> >that was initialized on the stack, if the initialization of such
> >bpf_program went well, or bail out otherwise.
> >
> >So I suggest you rename this to:
> >
> >int bpf_object__add_program(struct bpf_object *obj, void *data, size_t size, char *name, int idx)
> >
> >And probably move that debug that uses prog->section_name to just after
> >the realloc, here in this function.
> >
> >I will look at the other patches after lunch, thanks for providing the
> >git tree, I will try and use it before looking at the patches
> >individually, to get a feel of the whole thing.
>
> I didn't find your code, so I updated my git tree. Please see:
>
> https://github.com/WangNan0/linux/commit/e5ffa4f070ee36cce5130d08622dc305ad9cdb31
Ok, so used bpf_object__add_program, but you still return a bpf_program
pointer, that you do not use for anything, i.e. the failure of
bpf_object__add_program is reported only via a NULL return and you then
assume this was because ENOMEM was the reason, when there are multiple
errors that can cause bpf_object__add_program to fail.
Noted that with a comment on that patch, checked that no later patches
use that return, etc.
- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists