netdev - Re: [PATCH bpf-next v2 1/7] samples: bpf: refactor hbm program with libbpf

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201121023405.tchtyadco4x45sf3@kafai-mbp.dhcp.thefacebook.com>
Date:   Fri, 20 Nov 2020 18:34:05 -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 v2 1/7] samples: bpf: refactor hbm program with
 libbpf

On Thu, Nov 19, 2020 at 03:06:11PM +0000, Daniel T. Lee wrote:
[ ... ]

>  static int run_bpf_prog(char *prog, int cg_id)
>  {
> -	int map_fd;
> -	int rc = 0;
> +	struct hbm_queue_stats qstats = {0};
> +	char cg_dir[100], cg_pin_path[100];
> +	struct bpf_link *link = NULL;
>  	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");
> @@ -190,16 +183,25 @@ 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);
> +	if (libbpf_get_error(link)) {
> +		fprintf(stderr, "ERROR: bpf_program__attach_cgroup failed\n");
> +		link = NULL;
Again, this is not needed.  bpf_link__destroy() can
handle both NULL and error pointer.  Please take a look
at the bpf_link__destroy() in libbpf.c

> +		goto err;
> +	}
> +
> +	sprintf(cg_pin_path, "/sys/fs/bpf/hbm%d", cg_id);
> +	rc = bpf_link__pin(link, cg_pin_path);
> +	if (rc < 0) {
> +		printf("ERROR: bpf_link__pin failed: %d\n", rc);
>  		goto err;
>  	}
>  
> @@ -213,7 +215,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;
> @@ -242,7 +244,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;
> @@ -289,14 +291,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;
>  
> @@ -398,10 +400,10 @@ static int run_bpf_prog(char *prog, int cg_id)
>  err:
>  	rc = 1;
>  
> -	if (cg1)
> -		close(cg1);
> +	bpf_link__destroy(link);
> +	close(cg1);
>  	cleanup_cgroup_environment();
> -
> +	bpf_object__close(obj);
The bpf_* cleanup condition still looks wrong.

I can understand why it does not want to cleanup_cgroup_environment()
on the success case because the sh script may want to run test under this
cgroup.

However, the bpf_link__destroy(), bpf_object__close(), and
even close(cg1) should be done in both success and error
cases.

The cg1 test still looks wrong also.  The cg1 should
be init to -1 and then test for "if (cg1 == -1)".