| 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
| ||
|
Date: Fri, 9 Oct 2020 11:17:10 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: "Daniel T. Lee" <danieltimlee@...il.com>
Cc: Daniel Borkmann <daniel@...earbox.net>,
Alexei Starovoitov <ast@...nel.org>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Lorenzo Bianconi <lorenzo@...nel.org>,
bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
Xdp <xdp-newbies@...r.kernel.org>
Subject: Re: [PATCH bpf-next 1/3] samples: bpf: Refactor xdp_monitor with libbpf
On Fri, Oct 9, 2020 at 9:04 AM Daniel T. Lee <danieltimlee@...il.com> wrote:
>
> To avoid confusion caused by the increasing fragmentation of the BPF
> Loader program, this commit would like to change to the libbpf loader
> instead of using the bpf_load.
>
> Thanks to libbpf's bpf_link interface, managing the tracepoint BPF
> program is much easier. bpf_program__attach_tracepoint manages the
> enable of tracepoint event and attach of BPF programs to it with a
> single interface bpf_link, so there is no need to manage event_fd and
> prog_fd separately.
>
> This commit refactors xdp_monitor with using this libbpf API, and the
> bpf_load is removed and migrated to libbpf.
>
> Signed-off-by: Daniel T. Lee <danieltimlee@...il.com>
> ---
> samples/bpf/Makefile | 2 +-
> samples/bpf/xdp_monitor_user.c | 144 ++++++++++++++++++++++++---------
> 2 files changed, 108 insertions(+), 38 deletions(-)
>
[...]
> +static int tp_cnt;
> +static int map_cnt;
> static int verbose = 1;
> static bool debug = false;
> +struct bpf_map *map_data[NUM_MAP] = { 0 };
> +struct bpf_link *tp_links[NUM_TP] = { 0 };
this syntax means "initialize *only the first element* to 0
(explicitly) and the rest of elements to default (which is also 0)".
So it's just misleading, use ` = {}`.
>
> static const struct option long_options[] = {
> {"help", no_argument, NULL, 'h' },
> @@ -41,6 +65,15 @@ static const struct option long_options[] = {
> {0, 0, NULL, 0 }
> };
>
> +static void int_exit(int sig)
> +{
> + /* Detach tracepoints */
> + while (tp_cnt)
> + bpf_link__destroy(tp_links[--tp_cnt]);
> +
see below about proper cleanup
> + exit(0);
> +}
> +
> /* C standard specifies two constants, EXIT_SUCCESS(0) and EXIT_FAILURE(1) */
> #define EXIT_FAIL_MEM 5
>
[...]
>
> -static void print_bpf_prog_info(void)
> +static void print_bpf_prog_info(struct bpf_object *obj)
> {
> - int i;
> + struct bpf_program *prog;
> + struct bpf_map *map;
> + int i = 0;
>
> /* Prog info */
> - printf("Loaded BPF prog have %d bpf program(s)\n", prog_cnt);
> - for (i = 0; i < prog_cnt; i++) {
> - printf(" - prog_fd[%d] = fd(%d)\n", i, prog_fd[i]);
> + printf("Loaded BPF prog have %d bpf program(s)\n", tp_cnt);
> + bpf_object__for_each_program(prog, obj) {
> + printf(" - prog_fd[%d] = fd(%d)\n", i++, bpf_program__fd(prog));
> }
>
> + i = 0;
> /* Maps info */
> - printf("Loaded BPF prog have %d map(s)\n", map_data_count);
> - for (i = 0; i < map_data_count; i++) {
> - char *name = map_data[i].name;
> - int fd = map_data[i].fd;
> + printf("Loaded BPF prog have %d map(s)\n", map_cnt);
> + bpf_object__for_each_map(map, obj) {
> + const char *name = bpf_map__name(map);
> + int fd = bpf_map__fd(map);
>
> - printf(" - map_data[%d] = fd(%d) name:%s\n", i, fd, name);
> + printf(" - map_data[%d] = fd(%d) name:%s\n", i++, fd, name);
please move out increment into a separate statement, no need to
confuse readers unnecessarily
> }
>
> /* Event info */
> - printf("Searching for (max:%d) event file descriptor(s)\n", prog_cnt);
> - for (i = 0; i < prog_cnt; i++) {
> - if (event_fd[i] != -1)
> - printf(" - event_fd[%d] = fd(%d)\n", i, event_fd[i]);
> + printf("Searching for (max:%d) event file descriptor(s)\n", tp_cnt);
> + for (i = 0; i < tp_cnt; i++) {
> + int fd = bpf_link__fd(tp_links[i]);
> +
> + if (fd != -1)
> + printf(" - event_fd[%d] = fd(%d)\n", i, fd);
> }
> }
>
> int main(int argc, char **argv)
> {
[...]
> + obj = bpf_object__open_file(filename, NULL);
> + if (libbpf_get_error(obj)) {
> + printf("ERROR: opening BPF object file failed\n");
> + obj = NULL;
> return EXIT_FAILURE;
> }
> - if (!prog_fd[0]) {
> - printf("ERROR - load_bpf_file: %s\n", strerror(errno));
> +
> + /* load BPF program */
> + if (bpf_object__load(obj)) {
would be still good to call bpf_object__close(obj) here, this will
avoid warnings about memory leaks, if you run this program under ASAN
> + printf("ERROR: loading BPF object file failed\n");
> return EXIT_FAILURE;
> }
>
> + for (type = 0; type < NUM_MAP; type++) {
> + map_data[type] =
> + bpf_object__find_map_by_name(obj, map_type_strings[type]);
> +
> + if (libbpf_get_error(map_data[type])) {
> + printf("ERROR: finding a map in obj file failed\n");
same about cleanup, goto into single cleanup place would be
appropriate throughout this entire function, probably.
> + return EXIT_FAILURE;
> + }
> + map_cnt++;
> + }
> +
[...]
Powered by blists - more mailing lists