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: <20200901225841.qpsugarocx523dmy@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 1 Sep 2020 15:58:41 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     sdf@...gle.com
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        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

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.