netdev - Re: [PATCH bpf-next v4 4/5] bpftool: support dumping metadata

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] [day] [month] [year] [list]

Date:   Fri, 11 Sep 2020 08:56:19 -0700
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        YiFei Zhu <zhuyifei1999@...il.com>
Subject: Re: [PATCH bpf-next v4 4/5] bpftool: support dumping metadata

On Thu, Sep 10, 2020 at 12:54 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Wed, Sep 9, 2020 at 11:25 AM Stanislav Fomichev <sdf@...gle.com> wrote:
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index f7923414a052..ca264dc22434 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -29,6 +29,9 @@
> >  #include "main.h"
> >  #include "xlated_dumper.h"
> >
> > +#define BPF_METADATA_PREFIX "bpf_metadata_"
> > +#define BPF_METADATA_PREFIX_LEN strlen(BPF_METADATA_PREFIX)
>
> this is a runtime check, why not (sizeof(BPF_METADATA_PREFIX) - 1) instead?
Make sense, will fix.

> > +static int bpf_prog_find_metadata(int prog_fd, int *map_id)
> > ...
> > +free_map_ids:
> > +       saved_errno = errno;
> > +       free(map_ids);
> > +       errno = saved_errno;
>
> not clear why all this fussing with saving/restoring errno and then
> just returning 0 or -1? Just return -ENOMEM or -ENOENT as a result of
> this function?
Yeah, I just moved this function from it's original (libbpf) location as is.
I guess it makes sense to simplify the error handling now that
it's not in exported from libbpf.

> > +       if (bpf_map_lookup_elem(map_fd, &key, value))
> > +               goto out_free;
> > +
> > +       err = btf__get_from_id(map_info.btf_id, &btf);
> > +       if (err || !btf)
> > +               goto out_free;
>
> if you make bpf_prog_find_metadata() to do this value lookup and pass
> &info, it would probably make bpf_prog_find_metadata a bit more
> usable? You'll just need to ensure that callers free allocated memory.
> Then show_prog_metadata() would take care of processing BTF info.
Sounds reasonable. I can maybe keep existing
bpf_prog_find_metadata (rename to bpf_prog_find_metadata_map_id?)
and add another wrapper that does that map lookup.