Re: [PATCH bpf-next v2 2/3] bpftool: don't append / to the progtype

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Stanislav Fomichev <sdf@google.com>
Cc: Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>
Subject: Re: [PATCH bpf-next v2 2/3] bpftool: don't append / to the progtype
Date: Wed, 20 Oct 2021 11:12:16 -0700	[thread overview]
Message-ID: <CAEf4Bza3wYs7sjtp2UNDhT58yH+49C5sQonVssbnDko7kkpMaA@mail.gmail.com> (raw)
In-Reply-To: <20211012161544.660286-3-sdf@google.com>

On Tue, Oct 12, 2021 at 9:15 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> Otherwise, attaching with bpftool doesn't work with strict section names.
>
> Also, switch to libbpf strict mode to use the latest conventions
> (note, I don't think we have any cli api guarantees?).
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  tools/bpf/bpftool/main.c |  4 ++++
>  tools/bpf/bpftool/prog.c | 15 +--------------
>  2 files changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 02eaaf065f65..8223bac1e401 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -409,6 +409,10 @@ int main(int argc, char **argv)
>         block_mount = false;
>         bin_name = argv[0];
>
> +       ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL);

Quentin, any concerns about turning strict mode for bpftool? Either
way we should audit bpftool code to ensure that at least error
handling is done properly (see my comments on Dave's patch set about
== -1 checks).

> +       if (ret)
> +               p_err("failed to enable libbpf strict mode: %d", ret);
> +
>         hash_init(prog_table.table);
>         hash_init(map_table.table);
>         hash_init(link_table.table);
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 277d51c4c5d9..17505dc1243e 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -1396,8 +1396,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
>
>         while (argc) {
>                 if (is_prefix(*argv, "type")) {
> -                       char *type;
> -
>                         NEXT_ARG();
>
>                         if (common_prog_type != BPF_PROG_TYPE_UNSPEC) {
> @@ -1407,19 +1405,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
>                         if (!REQ_ARGS(1))
>                                 goto err_free_reuse_maps;
>
> -                       /* Put a '/' at the end of type to appease libbpf */
> -                       type = malloc(strlen(*argv) + 2);
> -                       if (!type) {
> -                               p_err("mem alloc failed");
> -                               goto err_free_reuse_maps;
> -                       }
> -                       *type = 0;
> -                       strcat(type, *argv);
> -                       strcat(type, "/");
> -
> -                       err = get_prog_type_by_name(type, &common_prog_type,
> +                       err = get_prog_type_by_name(*argv, &common_prog_type,
>                                                     &expected_attach_type);

Question not specifically to Stanislav, but anyone who's using bpftool
to load programs. Do we need to support program type overrides? Libbpf
has been inferring the program type for a long time now, are there
realistic use cases where this override logic is necessary? I see
there is bpf_object__for_each_program() loop down the code, it
essentially repeats what libbpf is already doing on
bpf_object__open(), why keep the duplicated logic?

libbpf_prog_type_by_name() is also a bad idea (IMO) and I'd like to
get rid of that in libbpf 1.0, so if we can stop using that from
bpftool, it would be great.

Thoughts?

> -                       free(type);
>                         if (err < 0)
>                                 goto err_free_reuse_maps;
>
> --
> 2.33.0.882.g93a45727a2-goog
>