[bpf-next] libbpf: Add bpf_object__rodata getter function
diff mbox series

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
Related show

Commit Message

Toke Høiland-Jørgensen March 26, 2020, 3:17 p.m. UTC
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(+)

Comments

Andrii Nakryiko March 26, 2020, 7:20 p.m. UTC | #1
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
>
Toke Høiland-Jørgensen March 27, 2020, 12:24 p.m. UTC | #2
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
Andrii Nakryiko March 27, 2020, 5:49 p.m. UTC | #3
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
>
Toke Høiland-Jørgensen March 27, 2020, 6:39 p.m. UTC | #4
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

Patch
diff mbox series

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;