| 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: <20150708140309.GA31332@kernel.org>
Date: Wed, 8 Jul 2015 11:03:09 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Wang Nan <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 v11 00/39] perf tools: filtering events using eBPF
programs - part1
Em Wed, Jul 08, 2015 at 01:13:49PM +0000, Wang Nan escreveu:
> Hi Arnaldo Carvalho de Melo,
Hi Wang (hope this shorter form is ok on your country, calling me just
"Arnaldo" is fine in mine :-))
> I rearranged the first 39 patches of this patchset according to
> your comments. After applying all of them you can use a hello world
> BPF program for testing. They are based on your 'tmp.perf/ebpf', commit
> 60cd37eb100c4880b28078a47f3062fac7572095.
> I hope I can manage a public avaliable git repository for you
> tomorrow (tomorrow means 24 hours later). What about a repository on
> github? However I have to do this out of my office because of company's
> IT policy.
Why not ask the kernel.org admins for a:
git://git.kernel.org/pub/scm/linux/kernel/git/wangnan0/linux.git
Area?
> In this v11 you can see following improvements:
>
> Commit messages improvements:
> 'bpf tools: Collect symbol table from SHT_SYMTAB section'
> 'bpf tools: Collect relocation sections from SHT_REL sections'
> 'bpf tools: Record map accessing instructions for each program'
> 'bpf tools: Relocate eBPF programs'
> 'bpf tools: Link all bpf objects onto a list'
>
> Decoupling:
> 'bpf tools: Collect eBPF programs from their own sections'
> 'bpf tools: Introduce accessors for struct bpf_program'
>
> Renaming: bpf_object__for_each -> bpf_object__for_each_safe
> 'bpf tools: Link all bpf objects onto a list'
>
> Patch ordering:
> 'perf tools: Make perf depend on libbpf'
>
> Error message improvement (refer to http://llvm.org/apt):
> 'perf tools: Call clang to compile C source to object code'
>
> In this v11 part 1 patch set, I haven't follow your comment in
> 'bpf tools: Introduce accessors for struct bpf_object' that let me
> update accessors API from returning error code to returning actual
> value and indicate error using invalid values. I prefer current API
> because I saw and fixed many bugs related to error code in perf's
> code (like commit ed30775).
> Reason of those bugs are misusing of error code: some part of code
> return negative on error, some part of code return non-zero on error,
> and developer forgot them. I don't want libbpf to introduce more bugs
> like them. But if you insist on it, I'll change it.
If you don't follow the chosen convention, bugs appear.
And the convention of returning < 0 for errors and >= 0 for success is
common, just see the libc wrappers for syscalls, see the open, read,
write man pages, etc, that is an ooooold convention :-)
And those wrappers struck me as exaggerated, see one of them:
int bpf_program__get_fd(struct bpf_program *prog, int *pfd)
{
if (!pfd)
return -EINVAL;
*pfd = prog->fd;
return 0;
}
What can go wrong with accessing a struct member? The only think I
thought about was: hey, the struct pointer needs to be checked against
NULL, but no, in this case what you thought could go wrong was for the
library user to pass a NULL pointer as the return place (pfd).
So, yes, I still think this is way exaggerated, if you insist that the
struct must be opaque and thus we need accessors, I think that having:
int bpf_program__fd(struct bpf_program *prog)
{
return prog->fd;
}
Is way more sane, yes, I would trow away those extra four characters
(get_).
Heck, in this case there is not even a possible problem where we would
want to return something negative instead of doing what was requested.
If you find any other part in tools/perf/ (or anywhere else) that
doesn't follows the convention it states it conforms to, please file a
bug or submit a patch, like you did in the case you mentioned (ed30775),
it would be a bug and has to be fixed.
- Arnaldo
> Wang Nan (39):
> bpf: Use correct #ifdef controller for trace_call_bpf()
> tracing, perf: Implement BPF programs attached to uprobes
> bpf tools: Introduce 'bpf' library and add bpf feature check
> bpf tools: Allow caller to set printing function
> bpf tools: Open eBPF object file and do basic validation
> bpf tools: Read eBPF object from buffer
> bpf tools: Check endianness and make libbpf fail early
> bpf tools: Iterate over ELF sections to collect information
> bpf tools: Collect version and license from ELF sections
> bpf tools: Collect map definitions from 'maps' section
> bpf tools: Collect symbol table from SHT_SYMTAB section
> bpf tools: Collect eBPF programs from their own sections
> bpf tools: Collect relocation sections from SHT_REL sections
> bpf tools: Record map accessing instructions for each program
> bpf tools: Add bpf.c/h for common bpf operations
> bpf tools: Create eBPF maps defined in an object file
> bpf tools: Relocate eBPF programs
> bpf tools: Introduce bpf_load_program() to bpf.c
> bpf tools: Load eBPF programs in object files into kernel
> bpf tools: Introduce accessors for struct bpf_program
> bpf tools: Introduce accessors for struct bpf_object
> bpf tools: Link all bpf objects onto a list
> perf tools: Introduce llvm config options
> perf tools: Call clang to compile C source to object code
> perf tools: Auto detecting kernel build directory
> perf tools: Auto detecting kernel include options
> perf tests: Add LLVM test for eBPF on-the-fly compiling
> perf tools: Make perf depend on libbpf
> perf record: Enable passing bpf object file to --event
> perf record: Compile scriptlets if pass '.c' to --event
> perf tools: Parse probe points of eBPF programs during preparation
> perf probe: Attach trace_probe_event with perf_probe_event
> perf record: Probe at kprobe points
> perf record: Load all eBPF object into kernel
> perf tools: Add bpf_fd field to evsel and config it
> perf tools: Attach eBPF program to perf event
> perf tools: Suppress probing messages when probing by BPF loading
> perf record: Add clang options for compiling BPF scripts
> bpf tools: Load a program with different instance using preprocessor
>
> include/linux/trace_events.h | 7 +-
> kernel/events/core.c | 4 +-
> kernel/trace/Kconfig | 2 +-
> kernel/trace/trace_uprobe.c | 5 +
> tools/build/Makefile.feature | 6 +-
> tools/build/feature/Makefile | 6 +-
> tools/build/feature/test-bpf.c | 18 +
> tools/lib/bpf/.gitignore | 2 +
> tools/lib/bpf/Build | 1 +
> tools/lib/bpf/Makefile | 195 +++++++
> tools/lib/bpf/bpf.c | 85 +++
> tools/lib/bpf/bpf.h | 23 +
> tools/lib/bpf/libbpf.c | 1184 +++++++++++++++++++++++++++++++++++++++
> tools/lib/bpf/libbpf.h | 107 ++++
> tools/perf/MANIFEST | 3 +
> tools/perf/Makefile.perf | 19 +-
> tools/perf/builtin-probe.c | 4 +-
> tools/perf/builtin-record.c | 43 +-
> tools/perf/config/Makefile | 19 +-
> tools/perf/tests/Build | 1 +
> tools/perf/tests/builtin-test.c | 4 +
> tools/perf/tests/llvm.c | 85 +++
> tools/perf/tests/make | 4 +-
> tools/perf/tests/tests.h | 1 +
> tools/perf/util/Build | 2 +
> tools/perf/util/bpf-loader.c | 310 ++++++++++
> tools/perf/util/bpf-loader.h | 46 ++
> tools/perf/util/config.c | 4 +
> tools/perf/util/debug.c | 5 +
> tools/perf/util/debug.h | 1 +
> tools/perf/util/evlist.c | 41 ++
> tools/perf/util/evlist.h | 1 +
> tools/perf/util/evsel.c | 17 +
> tools/perf/util/evsel.h | 1 +
> tools/perf/util/llvm-utils.c | 373 ++++++++++++
> tools/perf/util/llvm-utils.h | 39 ++
> tools/perf/util/parse-events.c | 16 +
> tools/perf/util/parse-events.h | 2 +
> tools/perf/util/parse-events.l | 6 +
> tools/perf/util/parse-events.y | 29 +-
> tools/perf/util/probe-event.c | 82 +--
> tools/perf/util/probe-event.h | 7 +-
> 42 files changed, 2759 insertions(+), 51 deletions(-)
> create mode 100644 tools/build/feature/test-bpf.c
> create mode 100644 tools/lib/bpf/.gitignore
> create mode 100644 tools/lib/bpf/Build
> create mode 100644 tools/lib/bpf/Makefile
> create mode 100644 tools/lib/bpf/bpf.c
> create mode 100644 tools/lib/bpf/bpf.h
> create mode 100644 tools/lib/bpf/libbpf.c
> create mode 100644 tools/lib/bpf/libbpf.h
> create mode 100644 tools/perf/tests/llvm.c
> create mode 100644 tools/perf/util/bpf-loader.c
> create mode 100644 tools/perf/util/bpf-loader.h
> create mode 100644 tools/perf/util/llvm-utils.c
> create mode 100644 tools/perf/util/llvm-utils.h
>
> --
> 1.8.3.4
--
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