diff mbox series

[bpf-next,v4] bpf: adjust btf load error logging

Message ID 20250312010358.3468811-1-linux@jordanrome.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next,v4] bpf: adjust btf load error logging | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-12 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-19 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-18 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-49 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-50 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-51 success Logs for x86_64-llvm-18 / veristat-meta
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success Errors and warnings before: 26 (+0) this patch: 26 (+0)
netdev/cc_maintainers warning 9 maintainers not CCed: sdf@fomichev.me kpsingh@kernel.org eddyz87@gmail.com jolsa@kernel.org martin.lau@linux.dev haoluo@google.com john.fastabend@gmail.com yonghong.song@linux.dev song@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-47 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-48 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc

Commit Message

Jordan Rome March 12, 2025, 1:03 a.m. UTC
For kernels where btf is not mandatory
we should log loading errors with `pr_info`
and not retry where we increase the log level
as this is just added noise.

Signed-off-by: Jordan Rome <linux@jordanrome.com>
---
 tools/lib/bpf/btf.c             | 36 ++++++++++++++++++---------------
 tools/lib/bpf/libbpf.c          |  3 ++-
 tools/lib/bpf/libbpf_internal.h |  2 +-
 3 files changed, 23 insertions(+), 18 deletions(-)

--
2.47.1

Comments

Andrii Nakryiko March 12, 2025, 6:40 p.m. UTC | #1
On Tue, Mar 11, 2025 at 6:04 PM Jordan Rome <linux@jordanrome.com> wrote:
>
> For kernels where btf is not mandatory
> we should log loading errors with `pr_info`
> and not retry where we increase the log level
> as this is just added noise.
>
> Signed-off-by: Jordan Rome <linux@jordanrome.com>
> ---
>  tools/lib/bpf/btf.c             | 36 ++++++++++++++++++---------------
>  tools/lib/bpf/libbpf.c          |  3 ++-
>  tools/lib/bpf/libbpf_internal.h |  2 +-
>  3 files changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index eea99c766a20..7da4807451bb 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -1379,9 +1379,10 @@ static void *btf_get_raw_data(const struct btf *btf, __u32 *size, bool swap_endi
>
>  int btf_load_into_kernel(struct btf *btf,
>                          char *log_buf, size_t log_sz, __u32 log_level,
> -                        int token_fd)
> +                        int token_fd, bool btf_mandatory)
>  {
>         LIBBPF_OPTS(bpf_btf_load_opts, opts);
> +       enum libbpf_print_level print_level;
>         __u32 buf_sz = 0, raw_size;
>         char *buf = NULL, *tmp;
>         void *raw_data;
> @@ -1435,22 +1436,25 @@ int btf_load_into_kernel(struct btf *btf,
>
>         btf->fd = bpf_btf_load(raw_data, raw_size, &opts);
>         if (btf->fd < 0) {
> -               /* time to turn on verbose mode and try again */
> -               if (log_level == 0) {
> -                       log_level = 1;
> -                       goto retry_load;
> +               if (btf_mandatory) {
> +                       /* time to turn on verbose mode and try again */
> +                       if (log_level == 0) {
> +                               log_level = 1;
> +                               goto retry_load;
> +                       }
> +                       /* only retry if caller didn't provide custom log_buf, but
> +                        * make sure we can never overflow buf_sz
> +                        */
> +                       if (!log_buf && errno == ENOSPC && buf_sz <= UINT_MAX / 2)

Original behavior was to go from log_level 0 to log_level 1 when the
user provided custom log_buf, which would happen even for
non-btf_mandatory case. I'd like to not change that behavior.

Did you find some problem with the code I proposed a few emails back?
If not, why not do that instead and preserve that custom log_buf and
log_level upgrade behavior?

pw-bot: cr

> +                               goto retry_load;
>                 }
> -               /* only retry if caller didn't provide custom log_buf, but
> -                * make sure we can never overflow buf_sz
> -                */
> -               if (!log_buf && errno == ENOSPC && buf_sz <= UINT_MAX / 2)
> -                       goto retry_load;
> -
>                 err = -errno;
> -               pr_warn("BTF loading error: %s\n", errstr(err));
> -               /* don't print out contents of custom log_buf */
> -               if (!log_buf && buf[0])
> -                       pr_warn("-- BEGIN BTF LOAD LOG ---\n%s\n-- END BTF LOAD LOG --\n", buf);
> +               print_level = btf_mandatory ? LIBBPF_WARN : LIBBPF_INFO;
> +               __pr(print_level, "BTF loading error: %s\n", errstr(err));
> +               if (!log_buf && log_level)
> +                       __pr(print_level,
> +                            "-- BEGIN BTF LOAD LOG ---\n%s\n-- END BTF LOAD LOG --\n",
> +                            buf);
>         }
>
>  done:
> @@ -1460,7 +1464,7 @@ int btf_load_into_kernel(struct btf *btf,
>
>  int btf__load_into_kernel(struct btf *btf)
>  {
> -       return btf_load_into_kernel(btf, NULL, 0, 0, 0);
> +       return btf_load_into_kernel(btf, NULL, 0, 0, 0, true);
>  }
>
>  int btf__fd(const struct btf *btf)
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 8e32286854ef..2cb3f067a12e 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -3604,9 +3604,10 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj)
>                  */
>                 btf__set_fd(kern_btf, 0);
>         } else {
> +               btf_mandatory = kernel_needs_btf(obj);
>                 /* currently BPF_BTF_LOAD only supports log_level 1 */
>                 err = btf_load_into_kernel(kern_btf, obj->log_buf, obj->log_size,
> -                                          obj->log_level ? 1 : 0, obj->token_fd);
> +                                          obj->log_level ? 1 : 0, obj->token_fd, btf_mandatory);
>         }
>         if (sanitize) {
>                 if (!err) {
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index de498e2dd6b0..f1de2ba462c3 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -408,7 +408,7 @@ int libbpf__load_raw_btf(const char *raw_types, size_t types_len,
>                          int token_fd);
>  int btf_load_into_kernel(struct btf *btf,
>                          char *log_buf, size_t log_sz, __u32 log_level,
> -                        int token_fd);
> +                        int token_fd, bool btf_mandatory);
>
>  struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf);
>  void btf_get_kernel_prefix_kind(enum bpf_attach_type attach_type,
> --
> 2.47.1
>
Jordan Rome March 12, 2025, 7:21 p.m. UTC | #2
On Wed, Mar 12, 2025 at 2:40 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Mar 11, 2025 at 6:04 PM Jordan Rome <linux@jordanrome.com> wrote:
> >
> > For kernels where btf is not mandatory
> > we should log loading errors with `pr_info`
> > and not retry where we increase the log level
> > as this is just added noise.
> >
> > Signed-off-by: Jordan Rome <linux@jordanrome.com>
> > ---
> >  tools/lib/bpf/btf.c             | 36 ++++++++++++++++++---------------
> >  tools/lib/bpf/libbpf.c          |  3 ++-
> >  tools/lib/bpf/libbpf_internal.h |  2 +-
> >  3 files changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index eea99c766a20..7da4807451bb 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -1379,9 +1379,10 @@ static void *btf_get_raw_data(const struct btf *btf, __u32 *size, bool swap_endi
> >
> >  int btf_load_into_kernel(struct btf *btf,
> >                          char *log_buf, size_t log_sz, __u32 log_level,
> > -                        int token_fd)
> > +                        int token_fd, bool btf_mandatory)
> >  {
> >         LIBBPF_OPTS(bpf_btf_load_opts, opts);
> > +       enum libbpf_print_level print_level;
> >         __u32 buf_sz = 0, raw_size;
> >         char *buf = NULL, *tmp;
> >         void *raw_data;
> > @@ -1435,22 +1436,25 @@ int btf_load_into_kernel(struct btf *btf,
> >
> >         btf->fd = bpf_btf_load(raw_data, raw_size, &opts);
> >         if (btf->fd < 0) {
> > -               /* time to turn on verbose mode and try again */
> > -               if (log_level == 0) {
> > -                       log_level = 1;
> > -                       goto retry_load;
> > +               if (btf_mandatory) {
> > +                       /* time to turn on verbose mode and try again */
> > +                       if (log_level == 0) {
> > +                               log_level = 1;
> > +                               goto retry_load;
> > +                       }
> > +                       /* only retry if caller didn't provide custom log_buf, but
> > +                        * make sure we can never overflow buf_sz
> > +                        */
> > +                       if (!log_buf && errno == ENOSPC && buf_sz <= UINT_MAX / 2)
>
> Original behavior was to go from log_level 0 to log_level 1 when the
> user provided custom log_buf, which would happen even for
> non-btf_mandatory case. I'd like to not change that behavior.
>

I don't quite understand why we want to increase the log level
if btf is not mandatory. Users will still get an info message that
btf failed to load and if they are still curious, they can increase
the log level themselves right? The goal of this patch is to reduce
log noise in cases where btf fails to load and is not mandatory.

> Did you find some problem with the code I proposed a few emails back?

Truth be told, I didn't like the added complexity in the conditionals.
I tried something similar in an earlier version and it led to a SEGFAULT
when trying to access `buf[0]` which had not been allocated.

> If not, why not do that instead and preserve that custom log_buf and
> log_level upgrade behavior?
>
> pw-bot: cr
>
> > +                               goto retry_load;
> >                 }
> > -               /* only retry if caller didn't provide custom log_buf, but
> > -                * make sure we can never overflow buf_sz
> > -                */
> > -               if (!log_buf && errno == ENOSPC && buf_sz <= UINT_MAX / 2)
> > -                       goto retry_load;
> > -
> >                 err = -errno;
> > -               pr_warn("BTF loading error: %s\n", errstr(err));
> > -               /* don't print out contents of custom log_buf */
> > -               if (!log_buf && buf[0])
> > -                       pr_warn("-- BEGIN BTF LOAD LOG ---\n%s\n-- END BTF LOAD LOG --\n", buf);
> > +               print_level = btf_mandatory ? LIBBPF_WARN : LIBBPF_INFO;
> > +               __pr(print_level, "BTF loading error: %s\n", errstr(err));
> > +               if (!log_buf && log_level)
> > +                       __pr(print_level,
> > +                            "-- BEGIN BTF LOAD LOG ---\n%s\n-- END BTF LOAD LOG --\n",
> > +                            buf);
> >         }
> >
> >  done:
> > @@ -1460,7 +1464,7 @@ int btf_load_into_kernel(struct btf *btf,
> >
> >  int btf__load_into_kernel(struct btf *btf)
> >  {
> > -       return btf_load_into_kernel(btf, NULL, 0, 0, 0);
> > +       return btf_load_into_kernel(btf, NULL, 0, 0, 0, true);
> >  }
> >
> >  int btf__fd(const struct btf *btf)
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 8e32286854ef..2cb3f067a12e 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -3604,9 +3604,10 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj)
> >                  */
> >                 btf__set_fd(kern_btf, 0);
> >         } else {
> > +               btf_mandatory = kernel_needs_btf(obj);
> >                 /* currently BPF_BTF_LOAD only supports log_level 1 */
> >                 err = btf_load_into_kernel(kern_btf, obj->log_buf, obj->log_size,
> > -                                          obj->log_level ? 1 : 0, obj->token_fd);
> > +                                          obj->log_level ? 1 : 0, obj->token_fd, btf_mandatory);
> >         }
> >         if (sanitize) {
> >                 if (!err) {
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index de498e2dd6b0..f1de2ba462c3 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -408,7 +408,7 @@ int libbpf__load_raw_btf(const char *raw_types, size_t types_len,
> >                          int token_fd);
> >  int btf_load_into_kernel(struct btf *btf,
> >                          char *log_buf, size_t log_sz, __u32 log_level,
> > -                        int token_fd);
> > +                        int token_fd, bool btf_mandatory);
> >
> >  struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf);
> >  void btf_get_kernel_prefix_kind(enum bpf_attach_type attach_type,
> > --
> > 2.47.1
> >
Andrii Nakryiko March 12, 2025, 7:31 p.m. UTC | #3
On Wed, Mar 12, 2025 at 12:21 PM Jordan Rome <linux@jordanrome.com> wrote:
>
> On Wed, Mar 12, 2025 at 2:40 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Mar 11, 2025 at 6:04 PM Jordan Rome <linux@jordanrome.com> wrote:
> > >
> > > For kernels where btf is not mandatory
> > > we should log loading errors with `pr_info`
> > > and not retry where we increase the log level
> > > as this is just added noise.
> > >
> > > Signed-off-by: Jordan Rome <linux@jordanrome.com>
> > > ---
> > >  tools/lib/bpf/btf.c             | 36 ++++++++++++++++++---------------
> > >  tools/lib/bpf/libbpf.c          |  3 ++-
> > >  tools/lib/bpf/libbpf_internal.h |  2 +-
> > >  3 files changed, 23 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > > index eea99c766a20..7da4807451bb 100644
> > > --- a/tools/lib/bpf/btf.c
> > > +++ b/tools/lib/bpf/btf.c
> > > @@ -1379,9 +1379,10 @@ static void *btf_get_raw_data(const struct btf *btf, __u32 *size, bool swap_endi
> > >
> > >  int btf_load_into_kernel(struct btf *btf,
> > >                          char *log_buf, size_t log_sz, __u32 log_level,
> > > -                        int token_fd)
> > > +                        int token_fd, bool btf_mandatory)
> > >  {
> > >         LIBBPF_OPTS(bpf_btf_load_opts, opts);
> > > +       enum libbpf_print_level print_level;
> > >         __u32 buf_sz = 0, raw_size;
> > >         char *buf = NULL, *tmp;
> > >         void *raw_data;
> > > @@ -1435,22 +1436,25 @@ int btf_load_into_kernel(struct btf *btf,
> > >
> > >         btf->fd = bpf_btf_load(raw_data, raw_size, &opts);
> > >         if (btf->fd < 0) {
> > > -               /* time to turn on verbose mode and try again */
> > > -               if (log_level == 0) {
> > > -                       log_level = 1;
> > > -                       goto retry_load;
> > > +               if (btf_mandatory) {
> > > +                       /* time to turn on verbose mode and try again */
> > > +                       if (log_level == 0) {
> > > +                               log_level = 1;
> > > +                               goto retry_load;
> > > +                       }
> > > +                       /* only retry if caller didn't provide custom log_buf, but
> > > +                        * make sure we can never overflow buf_sz
> > > +                        */
> > > +                       if (!log_buf && errno == ENOSPC && buf_sz <= UINT_MAX / 2)
> >
> > Original behavior was to go from log_level 0 to log_level 1 when the
> > user provided custom log_buf, which would happen even for
> > non-btf_mandatory case. I'd like to not change that behavior.
> >
>
> I don't quite understand why we want to increase the log level
> if btf is not mandatory. Users will still get an info message that
> btf failed to load and if they are still curious, they can increase
> the log level themselves right? The goal of this patch is to reduce
> log noise in cases where btf fails to load and is not mandatory.

Program's BTF is almost always not mandatory, so we'll basically never
log BTF verification failure (only for struct_ops stuff), even with
user-provided custom log buffer, which makes this whole logic much
less useful.

Perhaps we just need to keep existing logic as is? It's not expected
for BTF to fail to be loaded into the kernel, even for old kernels.
libbpf does BTF sanitization for that reason. So when this BTF loading
fails, it usually does indicate a real issue.

That's exactly what happened with the original issue that prompted
your patches. And that's a good thing that the user was bothered by
those warnings, which eventually led to fixing an issue with a missing
patch in their backported kernel, right?

Let's just keep things as is for now. I'd rather users complain about
this rather than these BTF upload failures go unnoticed forever.

>
> > Did you find some problem with the code I proposed a few emails back?
>
> Truth be told, I didn't like the added complexity in the conditionals.
> I tried something similar in an earlier version and it led to a SEGFAULT
> when trying to access `buf[0]` which had not been allocated.
>
> > If not, why not do that instead and preserve that custom log_buf and
> > log_level upgrade behavior?
> >
> > pw-bot: cr
> >

[...]
diff mbox series

Patch

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index eea99c766a20..7da4807451bb 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -1379,9 +1379,10 @@  static void *btf_get_raw_data(const struct btf *btf, __u32 *size, bool swap_endi

 int btf_load_into_kernel(struct btf *btf,
 			 char *log_buf, size_t log_sz, __u32 log_level,
-			 int token_fd)
+			 int token_fd, bool btf_mandatory)
 {
 	LIBBPF_OPTS(bpf_btf_load_opts, opts);
+	enum libbpf_print_level print_level;
 	__u32 buf_sz = 0, raw_size;
 	char *buf = NULL, *tmp;
 	void *raw_data;
@@ -1435,22 +1436,25 @@  int btf_load_into_kernel(struct btf *btf,

 	btf->fd = bpf_btf_load(raw_data, raw_size, &opts);
 	if (btf->fd < 0) {
-		/* time to turn on verbose mode and try again */
-		if (log_level == 0) {
-			log_level = 1;
-			goto retry_load;
+		if (btf_mandatory) {
+			/* time to turn on verbose mode and try again */
+			if (log_level == 0) {
+				log_level = 1;
+				goto retry_load;
+			}
+			/* only retry if caller didn't provide custom log_buf, but
+			 * make sure we can never overflow buf_sz
+			 */
+			if (!log_buf && errno == ENOSPC && buf_sz <= UINT_MAX / 2)
+				goto retry_load;
 		}
-		/* only retry if caller didn't provide custom log_buf, but
-		 * make sure we can never overflow buf_sz
-		 */
-		if (!log_buf && errno == ENOSPC && buf_sz <= UINT_MAX / 2)
-			goto retry_load;
-
 		err = -errno;
-		pr_warn("BTF loading error: %s\n", errstr(err));
-		/* don't print out contents of custom log_buf */
-		if (!log_buf && buf[0])
-			pr_warn("-- BEGIN BTF LOAD LOG ---\n%s\n-- END BTF LOAD LOG --\n", buf);
+		print_level = btf_mandatory ? LIBBPF_WARN : LIBBPF_INFO;
+		__pr(print_level, "BTF loading error: %s\n", errstr(err));
+		if (!log_buf && log_level)
+			__pr(print_level,
+			     "-- BEGIN BTF LOAD LOG ---\n%s\n-- END BTF LOAD LOG --\n",
+			     buf);
 	}

 done:
@@ -1460,7 +1464,7 @@  int btf_load_into_kernel(struct btf *btf,

 int btf__load_into_kernel(struct btf *btf)
 {
-	return btf_load_into_kernel(btf, NULL, 0, 0, 0);
+	return btf_load_into_kernel(btf, NULL, 0, 0, 0, true);
 }

 int btf__fd(const struct btf *btf)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 8e32286854ef..2cb3f067a12e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -3604,9 +3604,10 @@  static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj)
 		 */
 		btf__set_fd(kern_btf, 0);
 	} else {
+		btf_mandatory = kernel_needs_btf(obj);
 		/* currently BPF_BTF_LOAD only supports log_level 1 */
 		err = btf_load_into_kernel(kern_btf, obj->log_buf, obj->log_size,
-					   obj->log_level ? 1 : 0, obj->token_fd);
+					   obj->log_level ? 1 : 0, obj->token_fd, btf_mandatory);
 	}
 	if (sanitize) {
 		if (!err) {
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index de498e2dd6b0..f1de2ba462c3 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -408,7 +408,7 @@  int libbpf__load_raw_btf(const char *raw_types, size_t types_len,
 			 int token_fd);
 int btf_load_into_kernel(struct btf *btf,
 			 char *log_buf, size_t log_sz, __u32 log_level,
-			 int token_fd);
+			 int token_fd, bool btf_mandatory);

 struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf);
 void btf_get_kernel_prefix_kind(enum bpf_attach_type attach_type,