| 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: <20201118021043.zck246i2jvbboqlu@kafai-mbp.dhcp.thefacebook.com>
Date: Tue, 17 Nov 2020 18:10:48 -0800
From: Martin KaFai Lau <kafai@...com>
To: "Daniel T. Lee" <danieltimlee@...il.com>
CC: Daniel Borkmann <daniel@...earbox.net>,
Alexei Starovoitov <ast@...nel.org>,
Andrii Nakryiko <andrii@...nel.org>, brakmo <brakmo@...com>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Andrii Nakryiko <andrii.nakryiko@...il.com>,
Lorenzo Bianconi <lorenzo@...nel.org>,
David Ahern <dsa@...ulusnetworks.com>,
Yonghong Song <yhs@...com>,
Toke Høiland-Jørgensen <toke@...hat.com>,
Ira Weiny <ira.weiny@...el.com>, Thomas Graf <tgraf@...g.ch>,
Jakub Kicinski <kuba@...nel.org>,
John Fastabend <john.fastabend@...il.com>,
<bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
Xdp <xdp-newbies@...r.kernel.org>
Subject: Re: [PATCH bpf-next 2/9] samples: bpf: refactor hbm program with
libbpf
On Tue, Nov 17, 2020 at 02:56:37PM +0000, Daniel T. Lee wrote:
[ ... ]
> diff --git a/samples/bpf/hbm.c b/samples/bpf/hbm.c
> index b9f9f771dd81..008bc635ad9b 100644
> --- a/samples/bpf/hbm.c
> +++ b/samples/bpf/hbm.c
> @@ -46,7 +46,6 @@
> #include <bpf/bpf.h>
> #include <getopt.h>
>
> -#include "bpf_load.h"
> #include "bpf_rlimit.h"
> #include "trace_helpers.h"
> #include "cgroup_helpers.h"
> @@ -68,9 +67,10 @@ bool edt_flag;
> static void Usage(void);
> static void do_error(char *msg, bool errno_flag);
>
> +struct bpf_program *bpf_prog;
> struct bpf_object *obj;
> -int bpfprog_fd;
> int cgroup_storage_fd;
> +int queue_stats_fd;
>
> static void do_error(char *msg, bool errno_flag)
> {
> @@ -83,56 +83,54 @@ static void do_error(char *msg, bool errno_flag)
>
> static int prog_load(char *prog)
> {
> - struct bpf_prog_load_attr prog_load_attr = {
> - .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
> - .file = prog,
> - .expected_attach_type = BPF_CGROUP_INET_EGRESS,
> - };
> - int map_fd;
> - struct bpf_map *map;
> + int rc = 1;
>
> - int ret = 0;
> + obj = bpf_object__open_file(prog, NULL);
> + if (libbpf_get_error(obj)) {
> + printf("ERROR: opening BPF object file failed\n");
> + return rc;
> + }
>
> - if (access(prog, O_RDONLY) < 0) {
> - printf("Error accessing file %s: %s\n", prog, strerror(errno));
> - return 1;
> + /* load BPF program */
> + if (bpf_object__load(obj)) {
> + printf("ERROR: loading BPF object file failed\n");
> + goto cleanup;
> }
> - if (bpf_prog_load_xattr(&prog_load_attr, &obj, &bpfprog_fd))
> - ret = 1;
> - if (!ret) {
> - map = bpf_object__find_map_by_name(obj, "queue_stats");
> - map_fd = bpf_map__fd(map);
> - if (map_fd < 0) {
> - printf("Map not found: %s\n", strerror(map_fd));
> - ret = 1;
> - }
> +
> + bpf_prog = bpf_object__find_program_by_title(obj, "cgroup_skb/egress");
> + if (!bpf_prog) {
> + printf("ERROR: finding a prog in obj file failed\n");
> + goto cleanup;
> }
>
> - if (ret) {
> - printf("ERROR: bpf_prog_load_xattr failed for: %s\n", prog);
> - printf(" Output from verifier:\n%s\n------\n", bpf_log_buf);
> - ret = -1;
> - } else {
> - ret = map_fd;
> + queue_stats_fd = bpf_object__find_map_fd_by_name(obj, "queue_stats");
> + if (queue_stats_fd < 0) {
> + printf("ERROR: finding a map in obj file failed\n");
> + goto cleanup;
> }
>
> - return ret;
> + rc = 0;
Just return 0.
> +
> +cleanup:
> + if (rc != 0)
so this test can be avoided.
> + bpf_object__close(obj);
> +
> + return rc;
> }
>
> static int run_bpf_prog(char *prog, int cg_id)
> {
> - int map_fd;
> - int rc = 0;
> + struct hbm_queue_stats qstats = {0};
> + struct bpf_link *link = NULL;
> + char cg_dir[100];
> int key = 0;
> int cg1 = 0;
> - int type = BPF_CGROUP_INET_EGRESS;
> - char cg_dir[100];
> - struct hbm_queue_stats qstats = {0};
> + int rc = 0;
>
> sprintf(cg_dir, "/hbm%d", cg_id);
> - map_fd = prog_load(prog);
> - if (map_fd == -1)
> - return 1;
> + rc = prog_load(prog);
> + if (rc != 0)
> + return rc;
>
> if (setup_cgroup_environment()) {
> printf("ERROR: setting cgroup environment\n");
> @@ -152,16 +150,18 @@ static int run_bpf_prog(char *prog, int cg_id)
> qstats.stats = stats_flag ? 1 : 0;
> qstats.loopback = loopback_flag ? 1 : 0;
> qstats.no_cn = no_cn_flag ? 1 : 0;
> - if (bpf_map_update_elem(map_fd, &key, &qstats, BPF_ANY)) {
> + if (bpf_map_update_elem(queue_stats_fd, &key, &qstats, BPF_ANY)) {
> printf("ERROR: Could not update map element\n");
> goto err;
> }
>
> if (!outFlag)
> - type = BPF_CGROUP_INET_INGRESS;
> - if (bpf_prog_attach(bpfprog_fd, cg1, type, 0)) {
> - printf("ERROR: bpf_prog_attach fails!\n");
> - log_err("Attaching prog");
> + bpf_program__set_expected_attach_type(bpf_prog, BPF_CGROUP_INET_INGRESS);
> +
> + link = bpf_program__attach_cgroup(bpf_prog, cg1);
There is a difference here.
I think the bpf_prog will be detached when link is gone (e.g. process exit)
I am not sure it is what hbm is expected considering
cg is not clean-up on the success case.
> + if (libbpf_get_error(link)) {
> + fprintf(stderr, "ERROR: bpf_program__attach_cgroup failed\n");
> + link = NULL;
not needed. bpf_link__destroy() can handle err ptr.
> goto err;
> }
>
> @@ -175,7 +175,7 @@ static int run_bpf_prog(char *prog, int cg_id)
> #define DELTA_RATE_CHECK 10000 /* in us */
> #define RATE_THRESHOLD 9500000000 /* 9.5 Gbps */
>
> - bpf_map_lookup_elem(map_fd, &key, &qstats);
> + bpf_map_lookup_elem(queue_stats_fd, &key, &qstats);
> if (gettimeofday(&t0, NULL) < 0)
> do_error("gettimeofday failed", true);
> t_last = t0;
> @@ -204,7 +204,7 @@ static int run_bpf_prog(char *prog, int cg_id)
> fclose(fin);
> printf(" new_eth_tx_bytes:%llu\n",
> new_eth_tx_bytes);
> - bpf_map_lookup_elem(map_fd, &key, &qstats);
> + bpf_map_lookup_elem(queue_stats_fd, &key, &qstats);
> new_cg_tx_bytes = qstats.bytes_total;
> delta_bytes = new_eth_tx_bytes - last_eth_tx_bytes;
> last_eth_tx_bytes = new_eth_tx_bytes;
> @@ -251,14 +251,14 @@ static int run_bpf_prog(char *prog, int cg_id)
> rate = minRate;
> qstats.rate = rate;
> }
> - if (bpf_map_update_elem(map_fd, &key, &qstats, BPF_ANY))
> + if (bpf_map_update_elem(queue_stats_fd, &key, &qstats, BPF_ANY))
> do_error("update map element fails", false);
> }
> } else {
> sleep(dur);
> }
> // Get stats!
> - if (stats_flag && bpf_map_lookup_elem(map_fd, &key, &qstats)) {
> + if (stats_flag && bpf_map_lookup_elem(queue_stats_fd, &key, &qstats)) {
> char fname[100];
> FILE *fout;
>
> @@ -367,10 +367,12 @@ static int run_bpf_prog(char *prog, int cg_id)
> err:
> rc = 1;
>
> + bpf_link__destroy(link);
> +
> if (cg1)
This test looks wrong since cg1 is a fd.
> close(cg1);
> cleanup_cgroup_environment();
> -
> + bpf_object__close(obj);
> return rc;
> }
>
Powered by blists - more mailing lists