[PATCH bpf-next] libbpf: demote log message about unrecognised data sections back down to debug

[PATCH bpf-next] libbpf: demote log message about unrecognised data sections back down to debug

[Toke Høiland-Jørgensen]

When loading a BPF object, libbpf will output a log message when it
encounters an unrecognised data section. Since commit
50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF")
they are printed at "info" level so they will show up on the console by
default.

The rationale in the commit cited above is to "increase visibility" of such
errors, but there can be legitimate, and completely harmless, uses of extra
data sections. In particular, libxdp uses custom data sections to store
metadata and run priority configuration of XDP programs, which triggers the
log message. Ciara noticed that when porting XSK code to use libxdp instead
of libbpf as part of the deprecation, libbpf would output messages like:

libbpf: elf: skipping unrecognized data section(7) .xdp_run_config
libbpf: elf: skipping unrecognized data section(8) xdp_metadata
libbpf: elf: skipping unrecognized data section(8) xdp_metadata
libbpf: elf: skipping unrecognized data section(8) xdp_metadata

In light of this, let's demote the message severity back down to debug so
that it won't clutter up the default output of applications.

Cc: Magnus Karlsson <magnus.karlsson@intel.com>
Reported-by: Ciara Loftus <ciara.loftus@intel.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/libbpf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index a1bea1953df6..ac0eadbe1475 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -3297,8 +3297,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 				obj->efile.st_ops_data = data;
 				obj->efile.st_ops_shndx = idx;
 			} else {
-				pr_info("elf: skipping unrecognized data section(%d) %s\n",
-					idx, name);
+				pr_debug("elf: skipping unrecognized data section(%d) %s\n",
+					 idx, name);
 			}
 		} else if (sh->sh_type == SHT_REL) {
 			int targ_sec_idx = sh->sh_info; /* points to other section */
-- 
2.33.0

Re: [PATCH bpf-next] libbpf: demote log message about unrecognised data sections back down to debug

[Yonghong Song]

On 11/4/21 5:29 AM, Toke Høiland-Jørgensen wrote:
> When loading a BPF object, libbpf will output a log message when it
> encounters an unrecognised data section. Since commit
> 50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF")
> they are printed at "info" level so they will show up on the console by
> default.
> 
> The rationale in the commit cited above is to "increase visibility" of such
> errors, but there can be legitimate, and completely harmless, uses of extra
> data sections. In particular, libxdp uses custom data sections to store
> metadata and run priority configuration of XDP programs, which triggers the
> log message. Ciara noticed that when porting XSK code to use libxdp instead
> of libbpf as part of the deprecation, libbpf would output messages like:
> 
> libbpf: elf: skipping unrecognized data section(7) .xdp_run_config
> libbpf: elf: skipping unrecognized data section(8) xdp_metadata
> libbpf: elf: skipping unrecognized data section(8) xdp_metadata
> libbpf: elf: skipping unrecognized data section(8) xdp_metadata
> 
> In light of this, let's demote the message severity back down to debug so
> that it won't clutter up the default output of applications.
> 
> Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> Reported-by: Ciara Loftus <ciara.loftus@intel.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

I think it is okay to demote the visibility from "info" to "debug".
Andrii's recent patches can handle more data sections. And also,
if applications work fine, skipping unrecognized sections probably
will be ignored by user any way. If application has issues, debug
mode will reveal more information.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   tools/lib/bpf/libbpf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index a1bea1953df6..ac0eadbe1475 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -3297,8 +3297,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
>   				obj->efile.st_ops_data = data;
>   				obj->efile.st_ops_shndx = idx;
>   			} else {
> -				pr_info("elf: skipping unrecognized data section(%d) %s\n",
> -					idx, name);
> +				pr_debug("elf: skipping unrecognized data section(%d) %s\n",
> +					 idx, name);
>   			}
>   		} else if (sh->sh_type == SHT_REL) {
>   			int targ_sec_idx = sh->sh_info; /* points to other section */
> 

Re: [PATCH bpf-next] libbpf: demote log message about unrecognised data sections back down to debug

[Andrii Nakryiko]

On Thu, Nov 4, 2021 at 5:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> When loading a BPF object, libbpf will output a log message when it
> encounters an unrecognised data section. Since commit
> 50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF")
> they are printed at "info" level so they will show up on the console by
> default.
>
> The rationale in the commit cited above is to "increase visibility" of such
> errors, but there can be legitimate, and completely harmless, uses of extra
> data sections. In particular, libxdp uses custom data sections to store

What if we make those extra sections to be ".rodata.something" and
".data.something", but without ALLOC flag in ELF, so that libbpf won't
create maps for them. Libbpf also will check that program code never
references anything from those sections.

The worry I have about allowing arbitrary sections is that if in the
future we want to add other special sections, then we might run into a
conflict with some applications. So having some enforced naming
convention would help prevent this. WDYT?

> metadata and run priority configuration of XDP programs, which triggers the
> log message. Ciara noticed that when porting XSK code to use libxdp instead
> of libbpf as part of the deprecation, libbpf would output messages like:
>
> libbpf: elf: skipping unrecognized data section(7) .xdp_run_config
> libbpf: elf: skipping unrecognized data section(8) xdp_metadata
> libbpf: elf: skipping unrecognized data section(8) xdp_metadata
> libbpf: elf: skipping unrecognized data section(8) xdp_metadata
>
> In light of this, let's demote the message severity back down to debug so
> that it won't clutter up the default output of applications.
>
> Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> Reported-by: Ciara Loftus <ciara.loftus@intel.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  tools/lib/bpf/libbpf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index a1bea1953df6..ac0eadbe1475 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -3297,8 +3297,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
>                                 obj->efile.st_ops_data = data;
>                                 obj->efile.st_ops_shndx = idx;
>                         } else {
> -                               pr_info("elf: skipping unrecognized data section(%d) %s\n",
> -                                       idx, name);
> +                               pr_debug("elf: skipping unrecognized data section(%d) %s\n",
> +                                        idx, name);
>                         }
>                 } else if (sh->sh_type == SHT_REL) {
>                         int targ_sec_idx = sh->sh_info; /* points to other section */
> --
> 2.33.0
>

Re: [PATCH bpf-next] libbpf: demote log message about unrecognised data sections back down to debug

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Thu, Nov 4, 2021 at 5:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> When loading a BPF object, libbpf will output a log message when it
>> encounters an unrecognised data section. Since commit
>> 50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF")
>> they are printed at "info" level so they will show up on the console by
>> default.
>>
>> The rationale in the commit cited above is to "increase visibility" of such
>> errors, but there can be legitimate, and completely harmless, uses of extra
>> data sections. In particular, libxdp uses custom data sections to store
>
> What if we make those extra sections to be ".rodata.something" and
> ".data.something", but without ALLOC flag in ELF, so that libbpf won't
> create maps for them. Libbpf also will check that program code never
> references anything from those sections.
>
> The worry I have about allowing arbitrary sections is that if in the
> future we want to add other special sections, then we might run into a
> conflict with some applications. So having some enforced naming
> convention would help prevent this. WDYT?

Hmm, I see your point, but as the libxdp example shows, this has not
really been "disallowed" before. I.e., having these arbitrary sections
has worked just fine.

How about we do the opposite: claim a namespace for future libbpf
extensions and disallow (as in, hard fail) if anything unrecognised is
in those sections? For instance, this could be any section names
starting with .BPF?

-Toke

Re: [PATCH bpf-next] libbpf: demote log message about unrecognised data sections back down to debug

[Andrii Nakryiko]

On Fri, Nov 5, 2021 at 7:34 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Thu, Nov 4, 2021 at 5:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> When loading a BPF object, libbpf will output a log message when it
> >> encounters an unrecognised data section. Since commit
> >> 50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF")
> >> they are printed at "info" level so they will show up on the console by
> >> default.
> >>
> >> The rationale in the commit cited above is to "increase visibility" of such
> >> errors, but there can be legitimate, and completely harmless, uses of extra
> >> data sections. In particular, libxdp uses custom data sections to store
> >
> > What if we make those extra sections to be ".rodata.something" and
> > ".data.something", but without ALLOC flag in ELF, so that libbpf won't
> > create maps for them. Libbpf also will check that program code never
> > references anything from those sections.
> >
> > The worry I have about allowing arbitrary sections is that if in the
> > future we want to add other special sections, then we might run into a
> > conflict with some applications. So having some enforced naming
> > convention would help prevent this. WDYT?
>
> Hmm, I see your point, but as the libxdp example shows, this has not
> really been "disallowed" before. I.e., having these arbitrary sections
> has worked just fine.

A bunch of things were not disallowed, but that is changing for libbpf
1.0, so now is the right time :)

>
> How about we do the opposite: claim a namespace for future libbpf
> extensions and disallow (as in, hard fail) if anything unrecognised is
> in those sections? For instance, this could be any section names
> starting with .BPF?

Looking at what we added (.maps, .kconfig, .ksym), there is no common
prefix besides ".". I'd be ok to reserve anything starting with "."
for future use by libbpf. We can allow any non-dot section without a
warning (but it would have to be non-allocatable section). Would that
work?

>
> -Toke
>

Re: [PATCH bpf-next] libbpf: demote log message about unrecognised data sections back down to debug

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Fri, Nov 5, 2021 at 7:34 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Thu, Nov 4, 2021 at 5:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> When loading a BPF object, libbpf will output a log message when it
>> >> encounters an unrecognised data section. Since commit
>> >> 50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF")
>> >> they are printed at "info" level so they will show up on the console by
>> >> default.
>> >>
>> >> The rationale in the commit cited above is to "increase visibility" of such
>> >> errors, but there can be legitimate, and completely harmless, uses of extra
>> >> data sections. In particular, libxdp uses custom data sections to store
>> >
>> > What if we make those extra sections to be ".rodata.something" and
>> > ".data.something", but without ALLOC flag in ELF, so that libbpf won't
>> > create maps for them. Libbpf also will check that program code never
>> > references anything from those sections.
>> >
>> > The worry I have about allowing arbitrary sections is that if in the
>> > future we want to add other special sections, then we might run into a
>> > conflict with some applications. So having some enforced naming
>> > convention would help prevent this. WDYT?
>>
>> Hmm, I see your point, but as the libxdp example shows, this has not
>> really been "disallowed" before. I.e., having these arbitrary sections
>> has worked just fine.
>
> A bunch of things were not disallowed, but that is changing for libbpf
> 1.0, so now is the right time :)
>
>>
>> How about we do the opposite: claim a namespace for future libbpf
>> extensions and disallow (as in, hard fail) if anything unrecognised is
>> in those sections? For instance, this could be any section names
>> starting with .BPF?
>
> Looking at what we added (.maps, .kconfig, .ksym), there is no common
> prefix besides ".". I'd be ok to reserve anything starting with "."
> for future use by libbpf. We can allow any non-dot section without a
> warning (but it would have to be non-allocatable section). Would that
> work?

Not really :(

We already use .xdp_run_config as one of the section names in libxdp, so
if libbpf errors out on any .-prefixed section, we'll no longer be able
to load old BPF files. While we can update the calling code to deal with
any compatibility issues by detecting the libbpf version at compile-time
we don't have control over the BPF files we load. So there has to be a
way to opt out of any new stricter libbpf behaviour when loading BPF
files; I believe we had a similar discussion around map section names.

Any application using libbpf to load BPF files that wants to stay
compatible with old programs will have the same issue, BTW. iproute2
comes to mind as one...

-Toke

Re: [PATCH bpf-next] libbpf: demote log message about unrecognised data sections back down to debug

[Andrii Nakryiko]

On Mon, Nov 8, 2021 at 4:16 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Fri, Nov 5, 2021 at 7:34 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> >>
> >> > On Thu, Nov 4, 2021 at 5:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >> >>
> >> >> When loading a BPF object, libbpf will output a log message when it
> >> >> encounters an unrecognised data section. Since commit
> >> >> 50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF")
> >> >> they are printed at "info" level so they will show up on the console by
> >> >> default.
> >> >>
> >> >> The rationale in the commit cited above is to "increase visibility" of such
> >> >> errors, but there can be legitimate, and completely harmless, uses of extra
> >> >> data sections. In particular, libxdp uses custom data sections to store
> >> >
> >> > What if we make those extra sections to be ".rodata.something" and
> >> > ".data.something", but without ALLOC flag in ELF, so that libbpf won't
> >> > create maps for them. Libbpf also will check that program code never
> >> > references anything from those sections.
> >> >
> >> > The worry I have about allowing arbitrary sections is that if in the
> >> > future we want to add other special sections, then we might run into a
> >> > conflict with some applications. So having some enforced naming
> >> > convention would help prevent this. WDYT?
> >>
> >> Hmm, I see your point, but as the libxdp example shows, this has not
> >> really been "disallowed" before. I.e., having these arbitrary sections
> >> has worked just fine.
> >
> > A bunch of things were not disallowed, but that is changing for libbpf
> > 1.0, so now is the right time :)
> >
> >>
> >> How about we do the opposite: claim a namespace for future libbpf
> >> extensions and disallow (as in, hard fail) if anything unrecognised is
> >> in those sections? For instance, this could be any section names
> >> starting with .BPF?
> >
> > Looking at what we added (.maps, .kconfig, .ksym), there is no common
> > prefix besides ".". I'd be ok to reserve anything starting with "."
> > for future use by libbpf. We can allow any non-dot section without a
> > warning (but it would have to be non-allocatable section). Would that
> > work?
>
> Not really :(
>
> We already use .xdp_run_config as one of the section names in libxdp, so

Are any of those sections allocatable? What if libbpf complains about
allocatable ones only?

Also, how widely libxdp is used so that it's already impossible to
change anything?

> if libbpf errors out on any .-prefixed section, we'll no longer be able
> to load old BPF files. While we can update the calling code to deal with
> any compatibility issues by detecting the libbpf version at compile-time
> we don't have control over the BPF files we load. So there has to be a
> way to opt out of any new stricter libbpf behaviour when loading BPF
> files; I believe we had a similar discussion around map section names.
>
> Any application using libbpf to load BPF files that wants to stay
> compatible with old programs will have the same issue, BTW. iproute2
> comes to mind as one...
>
> -Toke
>

Re: [PATCH bpf-next] libbpf: demote log message about unrecognised data sections back down to debug

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Mon, Nov 8, 2021 at 4:16 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>>
>> > On Fri, Nov 5, 2021 at 7:34 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >>
>> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>> >>
>> >> > On Thu, Nov 4, 2021 at 5:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >> >>
>> >> >> When loading a BPF object, libbpf will output a log message when it
>> >> >> encounters an unrecognised data section. Since commit
>> >> >> 50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF")
>> >> >> they are printed at "info" level so they will show up on the console by
>> >> >> default.
>> >> >>
>> >> >> The rationale in the commit cited above is to "increase visibility" of such
>> >> >> errors, but there can be legitimate, and completely harmless, uses of extra
>> >> >> data sections. In particular, libxdp uses custom data sections to store
>> >> >
>> >> > What if we make those extra sections to be ".rodata.something" and
>> >> > ".data.something", but without ALLOC flag in ELF, so that libbpf won't
>> >> > create maps for them. Libbpf also will check that program code never
>> >> > references anything from those sections.
>> >> >
>> >> > The worry I have about allowing arbitrary sections is that if in the
>> >> > future we want to add other special sections, then we might run into a
>> >> > conflict with some applications. So having some enforced naming
>> >> > convention would help prevent this. WDYT?
>> >>
>> >> Hmm, I see your point, but as the libxdp example shows, this has not
>> >> really been "disallowed" before. I.e., having these arbitrary sections
>> >> has worked just fine.
>> >
>> > A bunch of things were not disallowed, but that is changing for libbpf
>> > 1.0, so now is the right time :)
>> >
>> >>
>> >> How about we do the opposite: claim a namespace for future libbpf
>> >> extensions and disallow (as in, hard fail) if anything unrecognised is
>> >> in those sections? For instance, this could be any section names
>> >> starting with .BPF?
>> >
>> > Looking at what we added (.maps, .kconfig, .ksym), there is no common
>> > prefix besides ".". I'd be ok to reserve anything starting with "."
>> > for future use by libbpf. We can allow any non-dot section without a
>> > warning (but it would have to be non-allocatable section). Would that
>> > work?
>>
>> Not really :(
>>
>> We already use .xdp_run_config as one of the section names in libxdp, so
>
> Are any of those sections allocatable? What if libbpf complains about
> allocatable ones only?

They are:
  5 .xdp_run_config 00000010  0000000000000000  0000000000000000  00000e70  2**3
                  CONTENTS, ALLOC, LOAD, DATA

They are just defined using the SEC() macro, like:

#define _CONCAT(x,y) x ## y
#define XDP_RUN_CONFIG(f) _CONCAT(_,f) SEC(".xdp_run_config")
struct {
	__uint(priority, 10);
	__uint(XDP_PASS, 1);
} XDP_RUN_CONFIG(FUNCNAME);

Is there a way to avoid the sections being marked as allocatable using
such macros?

> Also, how widely libxdp is used so that it's already impossible to
> change anything?

Well, we've been shipping it in three or four RHEL point releases at
this point, but I don't think we have any actual usage data, so I
honestly don't know.

I'm not against changing it, though; the XDP_RUN_CONFIG macro above is
defined in a header file we ship with libxdp, so it's straight-forward
to redefine it. I don't mind being strict by default either, I just want
to be able to do something like:

obj = bpf_object__open_file(filename, opts);
if (!obj && errno == EINVALIDSECTION) { /* or some other way of discovering this */
  warn("found invalid section, consider recompiling program; continuing anyway\n");
  opts.allow_arbitrary_sections=true;
  obj = bpf_object__open_file(filename, opts);
}

This is similar to how iproute2 sets relaxed_maps when opening a file so
it can deal with its old map definition types, so there is some precedent...

-Toke