Re: [PATCH bpf-next] libbpf: Add bpf_object__rodata getter function

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Mar 27, 2020 at 5:24 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Thu, Mar 26, 2020 at 8:18 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> This adds a new getter function to libbpf to get the rodata area of a bpf
> >> object. This is useful if a program wants to modify the rodata before
> >> loading the object. Any such modification needs to be done before loading,
> >> since libbpf freezes the backing map after populating it (to allow the
> >> kernel to do dead code elimination based on its contents).
> >>
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> ---
> >>  tools/lib/bpf/libbpf.c   | 13 +++++++++++++
> >>  tools/lib/bpf/libbpf.h   |  1 +
> >>  tools/lib/bpf/libbpf.map |  1 +
> >>  3 files changed, 15 insertions(+)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index 085e41f9b68e..d3e3bbe12f78 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -1352,6 +1352,19 @@ bpf_object__init_internal_map(struct bpf_object *obj, enum libbpf_map_type type,
> >>         return 0;
> >>  }
> >>
> >> +void *bpf_object__rodata(const struct bpf_object *obj, size_t *size)
> >
> > We probably don't want to expose this API. It just doesn't scale,
> > especially if/when we add support for custom sections names for global
> > variables.
>
> Right. I was not aware of any such plans, but OK.

There are no concrete plans, but compilers do create more than one
.rodata in some circumstances (I remember seeing something like
.rodata.align16, etc). So just don't want to have accessor for .rodata
but not for all other places. Let me take a closer look at v2, but I
think that one is a better approach.

>
> > Also checking for map->mmaped is too restrictive. See how BPF skeleton
> > solves this problem and still allows .rodata initialization even on
> > kernels that don't support memory-mapping global variables.
>
> Not sure what you mean here? As far as I can tell, the map->mmaped
> pointer has nothing to do with the kernel support for mmaping the map

Right, I forgot details by now and I just briefly looked at code and
saw mmap() call. But it's actually an anonymous mmap() call, which
gets remapped later, so yeah, it's a double-purpose memory area.

> contents. It's just what libbpf does to store the data of any
> internal_maps?
>
> I mean, bpf_object__open_skeleton() just does this:
>
>                 if (mmaped && (*map)->libbpf_type != LIBBPF_MAP_KCONFIG)
>                         *mmaped = (*map)->mmaped;
>
> which amounts to the same as I'm doing in this patch?
>
> > But basically, why can't you use BPF skeleton?
>
> Couple of reasons:
>
> - I don't need any of the other features of the skeleton
> - I don't want to depend on bpftool in the build process
> - I don't want to embed the BPF bytecode into the C object

Just curious, how are you intending to use global variables. Are you
restricting to a single global var (a struct probably), so it's easier
to work with it? Or are you resolving all the variables' offsets
manually? It's really inconvenient to work with global variables
without skeleton, which is why I'm curious.

>
> > Also, application can already find that map by looking at name.
>
> Yes, it can find the map, but it can't access the data. But I guess I
> could just add a getter for that. Just figured this was easier to
> consume; but I can see why it might impose restrictions on future
> changes, so I'll send a v2 with such a map-level getter instead.

Sounds good, I'll go review v2 now.
>
> -Toke
>