* [bpf-next v4] bpf: adjust btf load error logging
@ 2025-03-12 1:03 Jordan Rome
2025-03-12 18:40 ` Andrii Nakryiko
0 siblings, 1 reply; 5+ messages in thread
From: Jordan Rome @ 2025-03-12 1:03 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Kernel Team
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)
+ 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [bpf-next v4] bpf: adjust btf load error logging
2025-03-12 1:03 [bpf-next v4] bpf: adjust btf load error logging Jordan Rome
@ 2025-03-12 18:40 ` Andrii Nakryiko
2025-03-12 19:21 ` Jordan Rome
0 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2025-03-12 18:40 UTC (permalink / raw)
To: Jordan Rome
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bpf-next v4] bpf: adjust btf load error logging
2025-03-12 18:40 ` Andrii Nakryiko
@ 2025-03-12 19:21 ` Jordan Rome
2025-03-12 19:31 ` Andrii Nakryiko
0 siblings, 1 reply; 5+ messages in thread
From: Jordan Rome @ 2025-03-12 19:21 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team
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
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bpf-next v4] bpf: adjust btf load error logging
2025-03-12 19:21 ` Jordan Rome
@ 2025-03-12 19:31 ` Andrii Nakryiko
2025-03-16 22:20 ` Jordan Rome
0 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2025-03-12 19:31 UTC (permalink / raw)
To: Jordan Rome
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team
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
> >
[...]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bpf-next v4] bpf: adjust btf load error logging
2025-03-12 19:31 ` Andrii Nakryiko
@ 2025-03-16 22:20 ` Jordan Rome
0 siblings, 0 replies; 5+ messages in thread
From: Jordan Rome @ 2025-03-16 22:20 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Kernel Team
On Wed, Mar 12, 2025 at 3:31 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> 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.
>
Fair enough.
> >
> > > 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
> > >
>
> [...]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-03-16 22:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-12 1:03 [bpf-next v4] bpf: adjust btf load error logging Jordan Rome
2025-03-12 18:40 ` Andrii Nakryiko
2025-03-12 19:21 ` Jordan Rome
2025-03-12 19:31 ` Andrii Nakryiko
2025-03-16 22:20 ` Jordan Rome
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox