netdev - Re: [PATCH bpf-next 07/10] bpftool: add `gen bpfo` command to perform BPF static linking

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 for Android: free password hash cracker in your pocket

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

Message-ID: <7c78ba67-03ff-fd84-339e-08628716abdf@isovalent.com>
Date:   Fri, 12 Mar 2021 18:07:10 +0000
From:   Quentin Monnet <quentin@...valent.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 07/10] bpftool: add `gen bpfo` command to perform
 BPF static linking

2021-03-11 10:45 UTC-0800 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> On Thu, Mar 11, 2021 at 3:31 AM Quentin Monnet <quentin@...valent.com> wrote:
>>
>> 2021-03-09 20:04 UTC-0800 ~ Andrii Nakryiko <andrii@...nel.org>
>>> Add `bpftool gen bpfo <output-file> <input_file>...` command to statically
>>> link multiple BPF object files into a single output BPF object file.
>>>
>>> Similarly to existing '*.o' convention, bpftool is establishing a '*.bpfo'
>>> convention for statically-linked BPF object files. Both .o and .bpfo suffixes
>>> will be stripped out during BPF skeleton generation to infer BPF object name.
>>>
>>> Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
>>> ---
>>>  tools/bpf/bpftool/gen.c | 46 ++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 45 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
>>> index 4033c46d83e7..8b1ed6c0a62f 100644
>>> --- a/tools/bpf/bpftool/gen.c
>>> +++ b/tools/bpf/bpftool/gen.c
>>> +static int do_bpfo(int argc, char **argv)
>>
>>> +{
>>> +     struct bpf_linker *linker;
>>> +     const char *output_file, *file;
>>> +     int err;
>>> +
>>> +     if (!REQ_ARGS(2)) {
>>> +             usage();
>>> +             return -1;
>>> +     }
>>> +
>>> +     output_file = GET_ARG();
>>> +
>>> +     linker = bpf_linker__new(output_file, NULL);
>>> +     if (!linker) {
>>> +             p_err("failed to create BPF linker instance");
>>> +             return -1;
>>> +     }
>>> +
>>> +     while (argc) {
>>> +             file = GET_ARG();
>>> +
>>> +             err = bpf_linker__add_file(linker, file);
>>> +             if (err) {
>>> +                     p_err("failed to link '%s': %d", file, err);
>>
>> I think you mentioned before that your preference was for having just
>> the error code instead of using strerror(), but I think it would be more
>> user-friendly for the majority of users who don't know the error codes
>> if we had something more verbose? How about having both strerror()
>> output and the error code?
> 
> Sure, I'll add strerror(). My earlier point was that those messages
> are more often misleading (e.g., "file not found" for ENOENT or
> something similar) than helpful. I should check if bpftool is passing
> through warn-level messages from libbpf. Those are going to be very
> helpful, if anything goes wrong. --verbose should pass through all of
> libbpf messages, if it's not already the case.

Thanks. Yes, --verbose should do it, but it's worth a double-check.

>>> +                     goto err_out;
>>> +             }
>>> +     }
>>> +
>>> +     err = bpf_linker__finalize(linker);
>>> +     if (err) {
>>> +             p_err("failed to finalize ELF file: %d", err);
>>> +             goto err_out;
>>> +     }
>>> +
>>> +     return 0;
>>> +err_out:
>>> +     bpf_linker__free(linker);
>>> +     return -1;
>>
>> Should you call bpf_linker__free() even on success? I see that
>> bpf_linker__finalize() frees some of the resources, but it seems that
>> bpf_linker__free() does a more thorough job?
> 
> yep, it should really be just
> 
> err_out:
>     bpf_linker__free(linker);
>     return err;
> 
> 
>>
>>> +}
>>> +
>>>  static int do_help(int argc, char **argv)
>>>  {
>>>       if (json_output) {
>>> @@ -611,6 +654,7 @@ static int do_help(int argc, char **argv)
>>>
>>>  static const struct cmd cmds[] = {
>>>       { "skeleton",   do_skeleton },
>>> +     { "bpfo",       do_bpfo },
>>>       { "help",       do_help },
>>>       { 0 }
>>>  };
>>>
>>
>> Please update the usage help message, man page, and bash completion,
>> thanks. Especially because what "bpftool gen bpfo" does is not intuitive
>> (but I don't have a better name suggestion at the moment).
> 
> Yeah, forgot about manpage and bash completions, as usual.
> 
> re: "gen bpfo". I don't have much better naming as well. `bpftool
> link` is already taken for bpf_link-related commands. It felt like
> keeping this under "gen" command makes sense. But maybe `bpftool
> linker link <out> <in1> <in2> ...` would be a bit less confusing
> convention?

"bpftool linker" would have been nice, but having "bpftool link", I
think it would be even more confusing. We can pass commands by their
prefixes, so is "bpftool link" the command "link" or a prefix for
"linker"? (I know it would be easy to sort out from our point of view,
but for regular users I'm sure that would be confusing).

I don't mind leaving it under "bpftool gen", it's probably the most
relevant command we have. As for replacing the "bpfo" keyword, I've
thought of "combined", "static_linked", "archive", "concat". I write
them in case it's any inspiration, but I find none of them ideal :/.

Quentin