| Message ID | 20200326151741.125427-1-toke@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] libbpf: Add bpf_object__rodata getter function | expand |
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. 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. But basically, why can't you use BPF skeleton? Also, application can already find that map by looking at name. > +{ > + struct bpf_map *map; > + > + bpf_object__for_each_map(map, obj) { > + if (map->libbpf_type == LIBBPF_MAP_RODATA && map->mmaped) { > + *size = map->def.value_size; > + return map->mmaped; > + } > + } > + return NULL; > +} > + > static int bpf_object__init_global_data_maps(struct bpf_object *obj) > { > int err; > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index d38d7a629417..d2a9beed7b8a 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -166,6 +166,7 @@ typedef void (*bpf_object_clear_priv_t)(struct bpf_object *, void *); > LIBBPF_API int bpf_object__set_priv(struct bpf_object *obj, void *priv, > bpf_object_clear_priv_t clear_priv); > LIBBPF_API void *bpf_object__priv(const struct bpf_object *prog); > +LIBBPF_API void *bpf_object__rodata(const struct bpf_object *obj, size_t *size); > > LIBBPF_API int > libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type, > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index 5129283c0284..a248f4ff3a40 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -243,5 +243,6 @@ LIBBPF_0.0.8 { > bpf_link__pin; > bpf_link__pin_path; > bpf_link__unpin; > + bpf_object__rodata; > bpf_program__set_attach_target; > } LIBBPF_0.0.7; > -- > 2.26.0 >
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. > 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 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 > 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. -Toke
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 >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> > 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. Yeah, there's a single: static volatile const struct xdp_dispatcher_config conf = {}; in the BPF file. Which is defined as: struct xdp_dispatcher_config { __u8 num_progs_enabled; __u32 chain_call_actions[MAX_DISPATCHER_ACTIONS]; }; >> > 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. Great, thanks! -Toke
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) +{ + struct bpf_map *map; + + bpf_object__for_each_map(map, obj) { + if (map->libbpf_type == LIBBPF_MAP_RODATA && map->mmaped) { + *size = map->def.value_size; + return map->mmaped; + } + } + return NULL; +} + static int bpf_object__init_global_data_maps(struct bpf_object *obj) { int err; diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index d38d7a629417..d2a9beed7b8a 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -166,6 +166,7 @@ typedef void (*bpf_object_clear_priv_t)(struct bpf_object *, void *); LIBBPF_API int bpf_object__set_priv(struct bpf_object *obj, void *priv, bpf_object_clear_priv_t clear_priv); LIBBPF_API void *bpf_object__priv(const struct bpf_object *prog); +LIBBPF_API void *bpf_object__rodata(const struct bpf_object *obj, size_t *size); LIBBPF_API int libbpf_prog_type_by_name(const char *name, enum bpf_prog_type *prog_type, diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 5129283c0284..a248f4ff3a40 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -243,5 +243,6 @@ LIBBPF_0.0.8 { bpf_link__pin; bpf_link__pin_path; bpf_link__unpin; + bpf_object__rodata; bpf_program__set_attach_target; } LIBBPF_0.0.7;
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(+)