netdev - Re: [PATCH bpf-next v3 4/8] libbpf: implement bpf_prog_find

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]

Date:   Fri, 28 Aug 2020 23:10:10 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Cc:     davem@...emloft.net, ast@...nel.org, daniel@...earbox.net,
        Stanislav Fomichev <sdf@...gle.com>,
        YiFei Zhu <zhuyifei1999@...il.com>
Subject: Re: [PATCH bpf-next v3 4/8] libbpf: implement bpf_prog_find_metadata

Stanislav Fomichev <sdf@...gle.com> writes:

> This is a low-level function (hence in bpf.c) to find out the metadata
> map id for the provided program fd.
> It will be used in the next commits from bpftool.
>
> Cc: Toke Høiland-Jørgensen <toke@...hat.com>
> Cc: YiFei Zhu <zhuyifei1999@...il.com>
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> ---
>  tools/lib/bpf/bpf.c      | 74 ++++++++++++++++++++++++++++++++++++++++
>  tools/lib/bpf/bpf.h      |  1 +
>  tools/lib/bpf/libbpf.map |  1 +
>  3 files changed, 76 insertions(+)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 5f6c5676cc45..01c0ede1625d 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -885,3 +885,77 @@ int bpf_prog_bind_map(int prog_fd, int map_fd,
>  
>  	return sys_bpf(BPF_PROG_BIND_MAP, &attr, sizeof(attr));
>  }
> +
> +int bpf_prog_find_metadata(int prog_fd)
> +{
> +	struct bpf_prog_info prog_info = {};
> +	struct bpf_map_info map_info;
> +	__u32 prog_info_len;
> +	__u32 map_info_len;
> +	int saved_errno;
> +	__u32 *map_ids;
> +	int nr_maps;
> +	int map_fd;
> +	int ret;
> +	int i;
> +
> +	prog_info_len = sizeof(prog_info);
> +
> +	ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len);
> +	if (ret)
> +		return ret;
> +
> +	if (!prog_info.nr_map_ids)
> +		return -1;
> +
> +	map_ids = calloc(prog_info.nr_map_ids, sizeof(__u32));
> +	if (!map_ids)
> +		return -1;
> +
> +	nr_maps = prog_info.nr_map_ids;
> +	memset(&prog_info, 0, sizeof(prog_info));
> +	prog_info.nr_map_ids = nr_maps;
> +	prog_info.map_ids = ptr_to_u64(map_ids);
> +	prog_info_len = sizeof(prog_info);
> +
> +	ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len);
> +	if (ret)
> +		goto free_map_ids;
> +
> +	ret = -1;
> +	for (i = 0; i < prog_info.nr_map_ids; i++) {
> +		map_fd = bpf_map_get_fd_by_id(map_ids[i]);
> +		if (map_fd < 0) {
> +			ret = -1;
> +			goto free_map_ids;
> +		}
> +
> +		memset(&map_info, 0, sizeof(map_info));
> +		map_info_len = sizeof(map_info);
> +		ret = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len);
> +		saved_errno = errno;
> +		close(map_fd);
> +		errno = saved_errno;
> +		if (ret)
> +			goto free_map_ids;

If you get to this point on the last entry in the loop, ret will be 0,
and any of the continue statements below will end the loop, causing the
whole function to return 0. While this is not technically a valid ID, it
still seems odd that the function returns -1 on all error conditions
except this one.

Also, it would be good to be able to unambiguously distinguish between
"this program has no metadata associated" and "something went wrong
while querying the kernel for metadata (e.g., permission error)". So
something that amounts to a -ENOENT return; I guess turning all return
values into negative error codes would do that (and also do away with
the need for the saved_errno dance above), but id does clash a bit with
the convention in the rest of the file (where all the other functions
just return -1 and set errno)...

-Toke