2021-07-23 08:57 UTC-0700 ~ Andrii Nakryiko [off-list ref]

On Fri, Jul 23, 2021 at 2:52 AM Quentin Monnet [off-list ref] wrote:

quoted

2021-07-22 17:48 UTC-0700 ~ Andrii Nakryiko [off-list ref]

quoted

On Wed, Jul 21, 2021 at 8:38 AM Quentin Monnet [off-list ref] wrote:

quoted

Replace the calls to deprecated function btf__get_from_id() with calls
to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests).
Update the surrounding code accordingly (instead of passing a pointer to
the btf struct, get it as a return value from the function). Also make
sure that btf__free() is called on the pointer after use.

v2:
- Given that btf__load_from_kernel_by_id() has changed since v1, adapt
  the code accordingly instead of just renaming the function. Also add a
  few calls to btf__free() when necessary.

Signed-off-by: Quentin Monnet <redacted>
Acked-by: John Fastabend <john.fastabend@gmail.com>
---
 tools/bpf/bpftool/btf.c                      |  8 ++----
 tools/bpf/bpftool/btf_dumper.c               |  6 ++--
 tools/bpf/bpftool/map.c                      | 16 +++++------
 tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
 tools/perf/util/bpf-event.c                  | 11 ++++----
 tools/perf/util/bpf_counter.c                | 12 ++++++--
 tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
 7 files changed, 51 insertions(+), 35 deletions(-)

[...]

quoted

diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 09ae0381205b..12787758ce03 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c

@@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
                }
                return btf_vmlinux;
        } else if (info->btf_value_type_id) {
-               int err;
-
-               err = btf__get_from_id(info->btf_id, &btf);
-               if (err || !btf) {
+               btf = btf__load_from_kernel_by_id(info->btf_id);
+               if (libbpf_get_error(btf)) {
                        p_err("failed to get btf");
-                       btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
+                       if (!btf)
+                               btf = ERR_PTR(-ESRCH);

why not do a simpler (less conditionals)

err = libbpf_get_error(btf);
if (err) {
    btf = ERR_PTR(err);
}

?

Because if btf is NULL at this stage, this would change the return value
from -ESRCH to NULL. This would be problematic in mapdump(), since we
check this value ("if (IS_ERR(btf))") to detect a failure in
get_map_kv_btf().

see my reply on previous patch. libbpf_get_error() handles this
transparently regardless of CLEAN_PTRS mode, as long as it is called
right after API call. So the above sample will work as you'd expect,
preserving errors.

Right, it looks like I got confused on this one. I'll update it.

quoted

I could change that check in mapdump() to use libbpf_get_error()
instead, but in that case it would similarly change the return value for
mapdump() (and errno), which I think would be propagated up to main()
and would return 0 instead of -ESRCH. This does not seem suitable and
would play badly with batch mode, among other things.

So I'm considering keeping the one additional if.

quoted

quoted

                }
        }

@@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
                            void *value)
 {
        json_writer_t *btf_wtr;
-       struct btf *btf = NULL;
-       int err;
+       struct btf *btf;

-       err = btf__get_from_id(info->btf_id, &btf);
-       if (err) {
+       btf = btf__load_from_kernel_by_id(info->btf_id);
+       if (libbpf_get_error(btf)) {
                p_err("failed to get btf");
                return;
        }

[...]

quoted

        func_info = u64_to_ptr(info->func_info);

@@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
                kernel_syms_destroy(&dd);
        }

+       btf__free(btf);
+

warrants a Fixes: tag?

I don't mind adding the tags, but do they have any advantage here? My
understanding is that they tend to be neon signs for backports to stable
branches, but this patch depends on btf__load_from_kernel_by_id(),
meaning more patches to pull. I'll see if I can move the btf__free()
fixes to a separate commit, maybe.

Having Fixes: allows to keep track of where the issue originated. It
doesn't necessarily mean something has to be backported, as far as I
understand. So it's good to do regardless. Splitting fixes into a
separate patch works for me as well, but I don't care all that much
given they are small.

OK, thank you for the clarification :).
I'll keep a single patch in that case.