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]

Message-ID: <874kogike9.fsf@toke.dk>
Date:   Wed, 02 Sep 2020 11:43:26 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>, sdf@...gle.com
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, davem@...emloft.net,
        ast@...nel.org, daniel@...earbox.net,
        YiFei Zhu <zhuyifei1999@...il.com>, andriin@...com
Subject: Re: [PATCH bpf-next v3 4/8] libbpf: implement bpf_prog_find_metadata

Alexei Starovoitov <alexei.starovoitov@...il.com> writes:

> On Mon, Aug 31, 2020 at 08:40:01AM -0700, sdf@...gle.com wrote:
>> On 08/28, Toke H�iland-J�rgensen wrote:
>> > 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)...
>> Good point. I think I can change the function signature to:
>> 
>> 	int bpf_prog_find_metadata(int prog_fd, int *map_id)
>> 
>> And explicitly return map_id via argument. Then the ret can be used as
>> -1/0 error and I can set errno appropriately where it makes sense.
>> This will better match the convention we have in this file.
>
> I don't feel great about this libbpf api. bpftool already does
> bpf_obj_get_info_by_fd() for progs and for maps.
> This extra step and extra set of syscalls is redundant work.
> I think it's better to be done as part of bpftool.
> It doesn't quite fit as generic api.

Why not? We are establishing a convention for how to store (and read)
metadata from a program; by having an API to get this, we make sure that
every application that wants to access this metadata agrees on how to do
so. If we don't have it, people will have to go look at bpftool code,
and we'll end up with copied code snippets, which seems less than ideal.

-Toke