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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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