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

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

[Toke Høiland-Jørgensen]

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)
+{
+	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

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

[Andrii Nakryiko]

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
>

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

[Toke Høiland-Jørgensen]

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

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

[Andrii Nakryiko]

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
>

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

[Toke Høiland-Jørgensen]

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 bpf-next v2] libbpf: Add getter for pointer to data area for internal maps

[Toke Høiland-Jørgensen]

For internal maps (most notably the maps backing global variables), libbpf
uses an internal mmaped area to store the data after opening the object.
This data is subsequently copied into the kernel map when the object is
loaded.

This adds a getter for the pointer to that internal data store. This can be
used to modify the data before it is loaded into the kernel, which is
especially relevant for RODATA, which is frozen on load. This same pointer
is already exposed to the auto-generated skeletons, so access to it is
already API; this just adds a way to get at it without pulling in the full
skeleton infrastructure.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
v2:
  - Add per-map getter for data area instead of a global rodata getter for bpf_obj

tools/lib/bpf/libbpf.c   | 9 +++++++++
 tools/lib/bpf/libbpf.h   | 1 +
 tools/lib/bpf/libbpf.map | 1 +
 3 files changed, 11 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 085e41f9b68e..a0055f8908fd 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6756,6 +6756,15 @@ void *bpf_map__priv(const struct bpf_map *map)
 	return map ? map->priv : ERR_PTR(-EINVAL);
 }
 
+void *bpf_map__data_area(const struct bpf_map *map, size_t *size)
+{
+	if (map->mmaped && map->libbpf_type != LIBBPF_MAP_KCONFIG) {
+		*size = map->def.value_size;
+		return map->mmaped;
+	}
+	return NULL;
+}
+
 bool bpf_map__is_offload_neutral(const struct bpf_map *map)
 {
 	return map->def.type == BPF_MAP_TYPE_PERF_EVENT_ARRAY;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index d38d7a629417..baef0d2f3205 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -407,6 +407,7 @@ typedef void (*bpf_map_clear_priv_t)(struct bpf_map *, void *);
 LIBBPF_API int bpf_map__set_priv(struct bpf_map *map, void *priv,
 				 bpf_map_clear_priv_t clear_priv);
 LIBBPF_API void *bpf_map__priv(const struct bpf_map *map);
+LIBBPF_API void *bpf_map__data_area(const struct bpf_map *map, size_t *size);
 LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd);
 LIBBPF_API int bpf_map__resize(struct bpf_map *map, __u32 max_entries);
 LIBBPF_API bool bpf_map__is_offload_neutral(const struct bpf_map *map);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 5129283c0284..258528045a85 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_map__data_area;
 		bpf_program__set_attach_target;
 } LIBBPF_0.0.7;
-- 
2.26.0

Re: [PATCH bpf-next v2] libbpf: Add getter for pointer to data area for internal maps

[Andrii Nakryiko]

On Fri, Mar 27, 2020 at 5:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> For internal maps (most notably the maps backing global variables), libbpf
> uses an internal mmaped area to store the data after opening the object.
> This data is subsequently copied into the kernel map when the object is
> loaded.
>
> This adds a getter for the pointer to that internal data store. This can be
> used to modify the data before it is loaded into the kernel, which is
> especially relevant for RODATA, which is frozen on load. This same pointer
> is already exposed to the auto-generated skeletons, so access to it is
> already API; this just adds a way to get at it without pulling in the full
> skeleton infrastructure.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
> v2:
>   - Add per-map getter for data area instead of a global rodata getter for bpf_obj
>
> tools/lib/bpf/libbpf.c   | 9 +++++++++
>  tools/lib/bpf/libbpf.h   | 1 +
>  tools/lib/bpf/libbpf.map | 1 +
>  3 files changed, 11 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 085e41f9b68e..a0055f8908fd 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -6756,6 +6756,15 @@ void *bpf_map__priv(const struct bpf_map *map)
>         return map ? map->priv : ERR_PTR(-EINVAL);
>  }
>
> +void *bpf_map__data_area(const struct bpf_map *map, size_t *size)

I'm not entirely thrilled about "data_area" name. This is entirely for
providing initial value for maps, so maybe something like
bpf_map__init_value() or something along those lines?

Actually, how about a different API altogether:

bpf_map__set_init_value(struct bpf_map *map, void *data, size_t size)?

Application will have to prepare data of correct size, which will be
copied to libbpf's internal storage. It also doesn't expose any of
internal pointer. I don't think extra memcopy is a big deal here.
Thoughts?


> +{
> +       if (map->mmaped && map->libbpf_type != LIBBPF_MAP_KCONFIG) {
> +               *size = map->def.value_size;
> +               return map->mmaped;
> +       }
> +       return NULL;
> +}
> +
>  bool bpf_map__is_offload_neutral(const struct bpf_map *map)
>  {
>         return map->def.type == BPF_MAP_TYPE_PERF_EVENT_ARRAY;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index d38d7a629417..baef0d2f3205 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -407,6 +407,7 @@ typedef void (*bpf_map_clear_priv_t)(struct bpf_map *, void *);
>  LIBBPF_API int bpf_map__set_priv(struct bpf_map *map, void *priv,
>                                  bpf_map_clear_priv_t clear_priv);
>  LIBBPF_API void *bpf_map__priv(const struct bpf_map *map);
> +LIBBPF_API void *bpf_map__data_area(const struct bpf_map *map, size_t *size);
>  LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd);
>  LIBBPF_API int bpf_map__resize(struct bpf_map *map, __u32 max_entries);
>  LIBBPF_API bool bpf_map__is_offload_neutral(const struct bpf_map *map);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 5129283c0284..258528045a85 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_map__data_area;
>                 bpf_program__set_attach_target;
>  } LIBBPF_0.0.7;
> --
> 2.26.0
>

Re: [PATCH bpf-next v2] libbpf: Add getter for pointer to data area for internal maps

[Toke Høiland-Jørgensen]

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

> On Fri, Mar 27, 2020 at 5:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> For internal maps (most notably the maps backing global variables), libbpf
>> uses an internal mmaped area to store the data after opening the object.
>> This data is subsequently copied into the kernel map when the object is
>> loaded.
>>
>> This adds a getter for the pointer to that internal data store. This can be
>> used to modify the data before it is loaded into the kernel, which is
>> especially relevant for RODATA, which is frozen on load. This same pointer
>> is already exposed to the auto-generated skeletons, so access to it is
>> already API; this just adds a way to get at it without pulling in the full
>> skeleton infrastructure.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>> v2:
>>   - Add per-map getter for data area instead of a global rodata getter for bpf_obj
>>
>> tools/lib/bpf/libbpf.c   | 9 +++++++++
>>  tools/lib/bpf/libbpf.h   | 1 +
>>  tools/lib/bpf/libbpf.map | 1 +
>>  3 files changed, 11 insertions(+)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 085e41f9b68e..a0055f8908fd 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -6756,6 +6756,15 @@ void *bpf_map__priv(const struct bpf_map *map)
>>         return map ? map->priv : ERR_PTR(-EINVAL);
>>  }
>>
>> +void *bpf_map__data_area(const struct bpf_map *map, size_t *size)
>
> I'm not entirely thrilled about "data_area" name. This is entirely for
> providing initial value for maps, so maybe something like
> bpf_map__init_value() or something along those lines?
>
> Actually, how about a different API altogether:
>
> bpf_map__set_init_value(struct bpf_map *map, void *data, size_t size)?
>
> Application will have to prepare data of correct size, which will be
> copied to libbpf's internal storage. It also doesn't expose any of
> internal pointer. I don't think extra memcopy is a big deal here.
> Thoughts?

Huh, yeah, that's way better. Why didn't I think of that? Think maybe I
was too focused on doing this the same way the skeleton code is. I'll
send a v3 :)

-Toke

Re: [PATCH bpf-next v2] libbpf: Add getter for pointer to data area for internal maps

[Alexei Starovoitov]

On 3/27/20 3:26 PM, Toke Høiland-Jørgensen wrote:
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
> 
>> On Fri, Mar 27, 2020 at 5:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>>
>>> For internal maps (most notably the maps backing global variables), libbpf
>>> uses an internal mmaped area to store the data after opening the object.
>>> This data is subsequently copied into the kernel map when the object is
>>> loaded.
>>>
>>> This adds a getter for the pointer to that internal data store. This can be
>>> used to modify the data before it is loaded into the kernel, which is
>>> especially relevant for RODATA, which is frozen on load. This same pointer
>>> is already exposed to the auto-generated skeletons, so access to it is
>>> already API; this just adds a way to get at it without pulling in the full
>>> skeleton infrastructure.
>>>
>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>> ---
>>> v2:
>>>    - Add per-map getter for data area instead of a global rodata getter for bpf_obj
>>>
>>> tools/lib/bpf/libbpf.c   | 9 +++++++++
>>>   tools/lib/bpf/libbpf.h   | 1 +
>>>   tools/lib/bpf/libbpf.map | 1 +
>>>   3 files changed, 11 insertions(+)
>>>
>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>> index 085e41f9b68e..a0055f8908fd 100644
>>> --- a/tools/lib/bpf/libbpf.c
>>> +++ b/tools/lib/bpf/libbpf.c
>>> @@ -6756,6 +6756,15 @@ void *bpf_map__priv(const struct bpf_map *map)
>>>          return map ? map->priv : ERR_PTR(-EINVAL);
>>>   }
>>>
>>> +void *bpf_map__data_area(const struct bpf_map *map, size_t *size)
>>
>> I'm not entirely thrilled about "data_area" name. This is entirely for
>> providing initial value for maps, so maybe something like
>> bpf_map__init_value() or something along those lines?
>>
>> Actually, how about a different API altogether:
>>
>> bpf_map__set_init_value(struct bpf_map *map, void *data, size_t size)?
>>
>> Application will have to prepare data of correct size, which will be
>> copied to libbpf's internal storage. It also doesn't expose any of
>> internal pointer. I don't think extra memcopy is a big deal here.
>> Thoughts?
> 
> Huh, yeah, that's way better. Why didn't I think of that? Think maybe I
> was too focused on doing this the same way the skeleton code is. I'll
> send a v3 :)

Could you please add a selftest as well?
I'm not excited about new features without tests.

Re: [PATCH bpf-next v2] libbpf: Add getter for pointer to data area for internal maps

[Toke Høiland-Jørgensen]

Alexei Starovoitov <ast@fb.com> writes:

> On 3/27/20 3:26 PM, Toke Høiland-Jørgensen wrote:
>> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>> 
>>> On Fri, Mar 27, 2020 at 5:58 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>>>
>>>> For internal maps (most notably the maps backing global variables), libbpf
>>>> uses an internal mmaped area to store the data after opening the object.
>>>> This data is subsequently copied into the kernel map when the object is
>>>> loaded.
>>>>
>>>> This adds a getter for the pointer to that internal data store. This can be
>>>> used to modify the data before it is loaded into the kernel, which is
>>>> especially relevant for RODATA, which is frozen on load. This same pointer
>>>> is already exposed to the auto-generated skeletons, so access to it is
>>>> already API; this just adds a way to get at it without pulling in the full
>>>> skeleton infrastructure.
>>>>
>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>> ---
>>>> v2:
>>>>    - Add per-map getter for data area instead of a global rodata getter for bpf_obj
>>>>
>>>> tools/lib/bpf/libbpf.c   | 9 +++++++++
>>>>   tools/lib/bpf/libbpf.h   | 1 +
>>>>   tools/lib/bpf/libbpf.map | 1 +
>>>>   3 files changed, 11 insertions(+)
>>>>
>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>> index 085e41f9b68e..a0055f8908fd 100644
>>>> --- a/tools/lib/bpf/libbpf.c
>>>> +++ b/tools/lib/bpf/libbpf.c
>>>> @@ -6756,6 +6756,15 @@ void *bpf_map__priv(const struct bpf_map *map)
>>>>          return map ? map->priv : ERR_PTR(-EINVAL);
>>>>   }
>>>>
>>>> +void *bpf_map__data_area(const struct bpf_map *map, size_t *size)
>>>
>>> I'm not entirely thrilled about "data_area" name. This is entirely for
>>> providing initial value for maps, so maybe something like
>>> bpf_map__init_value() or something along those lines?
>>>
>>> Actually, how about a different API altogether:
>>>
>>> bpf_map__set_init_value(struct bpf_map *map, void *data, size_t size)?
>>>
>>> Application will have to prepare data of correct size, which will be
>>> copied to libbpf's internal storage. It also doesn't expose any of
>>> internal pointer. I don't think extra memcopy is a big deal here.
>>> Thoughts?
>> 
>> Huh, yeah, that's way better. Why didn't I think of that? Think maybe I
>> was too focused on doing this the same way the skeleton code is. I'll
>> send a v3 :)
>
> Could you please add a selftest as well?
> I'm not excited about new features without tests.

Sure, will do.

-Toke

[PATCH v3 1/2] libbpf: Add setter for initial value for internal maps

[Toke Høiland-Jørgensen]

For internal maps (most notably the maps backing global variables), libbpf
uses an internal mmaped area to store the data after opening the object.
This data is subsequently copied into the kernel map when the object is
loaded.

This adds a function to set a new value for that data, which can be used to
before it is loaded into the kernel. This is especially relevant for RODATA
maps, since those are frozen on load.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
v3:
  - Add a setter for the initial value instead of a getter for the pointer to it
  - Add selftest
v2:
  - Add per-map getter for data area instead of a global rodata getter for bpf_obj

 tools/lib/bpf/libbpf.c   | 11 +++++++++++
 tools/lib/bpf/libbpf.h   |  2 ++
 tools/lib/bpf/libbpf.map |  1 +
 3 files changed, 14 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 085e41f9b68e..f9953a8ffcfa 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6756,6 +6756,17 @@ void *bpf_map__priv(const struct bpf_map *map)
 	return map ? map->priv : ERR_PTR(-EINVAL);
 }
 
+int bpf_map__set_initial_value(struct bpf_map *map,
+			       void *data, size_t size)
+{
+	if (!map->mmaped || map->libbpf_type == LIBBPF_MAP_KCONFIG ||
+	    size != map->def.value_size)
+		return -EINVAL;
+
+	memcpy(map->mmaped, data, size);
+	return 0;
+}
+
 bool bpf_map__is_offload_neutral(const struct bpf_map *map)
 {
 	return map->def.type == BPF_MAP_TYPE_PERF_EVENT_ARRAY;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index d38d7a629417..ee30ed487221 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -407,6 +407,8 @@ typedef void (*bpf_map_clear_priv_t)(struct bpf_map *, void *);
 LIBBPF_API int bpf_map__set_priv(struct bpf_map *map, void *priv,
 				 bpf_map_clear_priv_t clear_priv);
 LIBBPF_API void *bpf_map__priv(const struct bpf_map *map);
+LIBBPF_API int bpf_map__set_initial_value(struct bpf_map *map,
+					  void *data, size_t size);
 LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd);
 LIBBPF_API int bpf_map__resize(struct bpf_map *map, __u32 max_entries);
 LIBBPF_API bool bpf_map__is_offload_neutral(const struct bpf_map *map);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 5129283c0284..f46873b9fe5e 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_map__set_initial_value;
 		bpf_program__set_attach_target;
 } LIBBPF_0.0.7;
-- 
2.26.0

Re: [PATCH v3 1/2] libbpf: Add setter for initial value for internal maps

[Andrii Nakryiko]

On Sat, Mar 28, 2020 at 11:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> For internal maps (most notably the maps backing global variables), libbpf
> uses an internal mmaped area to store the data after opening the object.
> This data is subsequently copied into the kernel map when the object is
> loaded.
>
> This adds a function to set a new value for that data, which can be used to
> before it is loaded into the kernel. This is especially relevant for RODATA
> maps, since those are frozen on load.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
> v3:
>   - Add a setter for the initial value instead of a getter for the pointer to it
>   - Add selftest
> v2:
>   - Add per-map getter for data area instead of a global rodata getter for bpf_obj
>
>  tools/lib/bpf/libbpf.c   | 11 +++++++++++
>  tools/lib/bpf/libbpf.h   |  2 ++
>  tools/lib/bpf/libbpf.map |  1 +
>  3 files changed, 14 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 085e41f9b68e..f9953a8ffcfa 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -6756,6 +6756,17 @@ void *bpf_map__priv(const struct bpf_map *map)
>         return map ? map->priv : ERR_PTR(-EINVAL);
>  }
>
> +int bpf_map__set_initial_value(struct bpf_map *map,
> +                              void *data, size_t size)

nit: const void *


> +{
> +       if (!map->mmaped || map->libbpf_type == LIBBPF_MAP_KCONFIG ||
> +           size != map->def.value_size)
> +               return -EINVAL;
> +

How about also checking that bpf_map wasn't yet created? Checking
map->fd >= 0 should be enough.

> +       memcpy(map->mmaped, data, size);
> +       return 0;
> +}
> +
>  bool bpf_map__is_offload_neutral(const struct bpf_map *map)
>  {
>         return map->def.type == BPF_MAP_TYPE_PERF_EVENT_ARRAY;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index d38d7a629417..ee30ed487221 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -407,6 +407,8 @@ typedef void (*bpf_map_clear_priv_t)(struct bpf_map *, void *);
>  LIBBPF_API int bpf_map__set_priv(struct bpf_map *map, void *priv,
>                                  bpf_map_clear_priv_t clear_priv);
>  LIBBPF_API void *bpf_map__priv(const struct bpf_map *map);
> +LIBBPF_API int bpf_map__set_initial_value(struct bpf_map *map,
> +                                         void *data, size_t size);
>  LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd);
>  LIBBPF_API int bpf_map__resize(struct bpf_map *map, __u32 max_entries);
>  LIBBPF_API bool bpf_map__is_offload_neutral(const struct bpf_map *map);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 5129283c0284..f46873b9fe5e 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_map__set_initial_value;
>                 bpf_program__set_attach_target;
>  } LIBBPF_0.0.7;
> --
> 2.26.0
>

Re: [PATCH v3 1/2] libbpf: Add setter for initial value for internal maps

[Toke Høiland-Jørgensen]

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

> On Sat, Mar 28, 2020 at 11:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> For internal maps (most notably the maps backing global variables), libbpf
>> uses an internal mmaped area to store the data after opening the object.
>> This data is subsequently copied into the kernel map when the object is
>> loaded.
>>
>> This adds a function to set a new value for that data, which can be used to
>> before it is loaded into the kernel. This is especially relevant for RODATA
>> maps, since those are frozen on load.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>> v3:
>>   - Add a setter for the initial value instead of a getter for the pointer to it
>>   - Add selftest
>> v2:
>>   - Add per-map getter for data area instead of a global rodata getter for bpf_obj
>>
>>  tools/lib/bpf/libbpf.c   | 11 +++++++++++
>>  tools/lib/bpf/libbpf.h   |  2 ++
>>  tools/lib/bpf/libbpf.map |  1 +
>>  3 files changed, 14 insertions(+)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 085e41f9b68e..f9953a8ffcfa 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -6756,6 +6756,17 @@ void *bpf_map__priv(const struct bpf_map *map)
>>         return map ? map->priv : ERR_PTR(-EINVAL);
>>  }
>>
>> +int bpf_map__set_initial_value(struct bpf_map *map,
>> +                              void *data, size_t size)
>
> nit: const void *

ACK.

>> +{
>> +       if (!map->mmaped || map->libbpf_type == LIBBPF_MAP_KCONFIG ||
>> +           size != map->def.value_size)
>> +               return -EINVAL;
>> +
>
> How about also checking that bpf_map wasn't yet created? Checking
> map->fd >= 0 should be enough.


Yup, makes sense.

>> +       memcpy(map->mmaped, data, size);
>> +       return 0;
>> +}
>> +
>>  bool bpf_map__is_offload_neutral(const struct bpf_map *map)
>>  {
>>         return map->def.type == BPF_MAP_TYPE_PERF_EVENT_ARRAY;
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index d38d7a629417..ee30ed487221 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -407,6 +407,8 @@ typedef void (*bpf_map_clear_priv_t)(struct bpf_map *, void *);
>>  LIBBPF_API int bpf_map__set_priv(struct bpf_map *map, void *priv,
>>                                  bpf_map_clear_priv_t clear_priv);
>>  LIBBPF_API void *bpf_map__priv(const struct bpf_map *map);
>> +LIBBPF_API int bpf_map__set_initial_value(struct bpf_map *map,
>> +                                         void *data, size_t size);
>>  LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd);
>>  LIBBPF_API int bpf_map__resize(struct bpf_map *map, __u32 max_entries);
>>  LIBBPF_API bool bpf_map__is_offload_neutral(const struct bpf_map *map);
>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>> index 5129283c0284..f46873b9fe5e 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_map__set_initial_value;
>>                 bpf_program__set_attach_target;
>>  } LIBBPF_0.0.7;
>> --
>> 2.26.0
>>

[PATCH v4 1/2] libbpf: Add setter for initial value for internal maps

[Toke Høiland-Jørgensen]

For internal maps (most notably the maps backing global variables), libbpf
uses an internal mmaped area to store the data after opening the object.
This data is subsequently copied into the kernel map when the object is
loaded.

This adds a function to set a new value for that data, which can be used to
before it is loaded into the kernel. This is especially relevant for RODATA
maps, since those are frozen on load.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
v4:
  - Make void pointer const, check if map was loaded
  - Reject set if map was already loaded
  - Split test into its own file
v3:
  - Add a setter for the initial value instead of a getter for the pointer to it
  - Add selftest
v2:
  - Add per-map getter for data area instead of a global rodata getter for bpf_obj

 tools/lib/bpf/libbpf.c   | 11 +++++++++++
 tools/lib/bpf/libbpf.h   |  2 ++
 tools/lib/bpf/libbpf.map |  1 +
 3 files changed, 14 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 62903302935e..7deab98720ee 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6758,6 +6758,17 @@ void *bpf_map__priv(const struct bpf_map *map)
 	return map ? map->priv : ERR_PTR(-EINVAL);
 }
 
+int bpf_map__set_initial_value(struct bpf_map *map,
+			       const void *data, size_t size)
+{
+	if (!map->mmaped || map->libbpf_type == LIBBPF_MAP_KCONFIG ||
+	    size != map->def.value_size || map->fd >= 0)
+		return -EINVAL;
+
+	memcpy(map->mmaped, data, size);
+	return 0;
+}
+
 bool bpf_map__is_offload_neutral(const struct bpf_map *map)
 {
 	return map->def.type == BPF_MAP_TYPE_PERF_EVENT_ARRAY;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index bf7a35a9556d..958ae71c116e 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -407,6 +407,8 @@ typedef void (*bpf_map_clear_priv_t)(struct bpf_map *, void *);
 LIBBPF_API int bpf_map__set_priv(struct bpf_map *map, void *priv,
 				 bpf_map_clear_priv_t clear_priv);
 LIBBPF_API void *bpf_map__priv(const struct bpf_map *map);
+LIBBPF_API int bpf_map__set_initial_value(struct bpf_map *map,
+					  const void *data, size_t size);
 LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd);
 LIBBPF_API int bpf_map__resize(struct bpf_map *map, __u32 max_entries);
 LIBBPF_API bool bpf_map__is_offload_neutral(const struct bpf_map *map);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index dcc87db3ca8a..159826b36b38 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -243,6 +243,7 @@ LIBBPF_0.0.8 {
 		bpf_link__pin;
 		bpf_link__pin_path;
 		bpf_link__unpin;
+		bpf_map__set_initial_value;
 		bpf_program__set_attach_target;
 		bpf_set_link_xdp_fd_opts;
 } LIBBPF_0.0.7;
-- 
2.26.0

Re: [PATCH v4 1/2] libbpf: Add setter for initial value for internal maps

[Andrii Nakryiko]

On Sun, Mar 29, 2020 at 6:23 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> For internal maps (most notably the maps backing global variables), libbpf
> uses an internal mmaped area to store the data after opening the object.
> This data is subsequently copied into the kernel map when the object is
> loaded.
>
> This adds a function to set a new value for that data, which can be used to
> before it is loaded into the kernel. This is especially relevant for RODATA
> maps, since those are frozen on load.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

LGTM.

Acked-by: Andrii Nakryiko <andriin@fb.com>

[...]

Re: [PATCH v4 1/2] libbpf: Add setter for initial value for internal maps

[Daniel Borkmann]

On 3/29/20 3:22 PM, Toke Høiland-Jørgensen wrote:
> For internal maps (most notably the maps backing global variables), libbpf
> uses an internal mmaped area to store the data after opening the object.
> This data is subsequently copied into the kernel map when the object is
> loaded.
> 
> This adds a function to set a new value for that data, which can be used to
> before it is loaded into the kernel. This is especially relevant for RODATA
> maps, since those are frozen on load.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Both applied, thanks!

[PATCH v4 2/2] selftests: Add test for overriding global data value before load

[Toke Høiland-Jørgensen]

This adds a test to exercise the new bpf_map__set_initial_value() function.
The test simply overrides the global data section with all zeroes, and
checks that the new value makes it into the kernel map on load.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 .../bpf/prog_tests/global_data_init.c         | 61 +++++++++++++++++++
 .../selftests/bpf/progs/test_global_data.c    |  2 +-
 2 files changed, 62 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/global_data_init.c

diff --git a/tools/testing/selftests/bpf/prog_tests/global_data_init.c b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
new file mode 100644
index 000000000000..3bdaa5a40744
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/global_data_init.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+void test_global_data_init(void)
+{
+	const char *file = "./test_global_data.o";
+	int err = -ENOMEM, map_fd, zero = 0;
+	__u8 *buff = NULL, *newval = NULL;
+	struct bpf_object *obj;
+	struct bpf_map *map;
+        __u32 duration = 0;
+	size_t sz;
+
+	obj = bpf_object__open_file(file, NULL);
+	if (CHECK_FAIL(!obj))
+		return;
+
+	map = bpf_object__find_map_by_name(obj, "test_glo.rodata");
+	if (CHECK_FAIL(!map || !bpf_map__is_internal(map)))
+		goto out;
+
+	sz = bpf_map__def(map)->value_size;
+	newval = malloc(sz);
+	if (CHECK_FAIL(!newval))
+		goto out;
+
+	memset(newval, 0, sz);
+	/* wrong size, should fail */
+	err = bpf_map__set_initial_value(map, newval, sz - 1);
+	if (CHECK(!err, "reject set initial value wrong size", "err %d\n", err))
+		goto out;
+
+	err = bpf_map__set_initial_value(map, newval, sz);
+	if (CHECK(err, "set initial value", "err %d\n", err))
+		goto out;
+
+	err = bpf_object__load(obj);
+	if (CHECK_FAIL(err))
+		goto out;
+
+	map_fd = bpf_map__fd(map);
+	if (CHECK_FAIL(map_fd < 0))
+		goto out;
+
+	buff = malloc(sz);
+	if (buff)
+		err = bpf_map_lookup_elem(map_fd, &zero, buff);
+	if (CHECK(!buff || err || memcmp(buff, newval, sz),
+		  "compare .rodata map data override",
+		  "err %d errno %d\n", err, errno))
+		goto out;
+
+	memset(newval, 1, sz);
+	/* object loaded - should fail */
+	err = bpf_map__set_initial_value(map, newval, sz);
+	CHECK(!err, "reject set initial value after load", "err %d\n", err);
+out:
+	free(buff);
+	free(newval);
+	bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_global_data.c b/tools/testing/selftests/bpf/progs/test_global_data.c
index dd7a4d3dbc0d..1319be1c54ba 100644
--- a/tools/testing/selftests/bpf/progs/test_global_data.c
+++ b/tools/testing/selftests/bpf/progs/test_global_data.c
@@ -68,7 +68,7 @@ static struct foo struct3 = {
 		bpf_map_update_elem(&result_##map, &key, var, 0);	\
 	} while (0)
 
-SEC("static_data_load")
+SEC("classifier/static_data_load")
 int load_static_data(struct __sk_buff *skb)
 {
 	static const __u64 bar = ~0;
-- 
2.26.0

Re: [PATCH v4 2/2] selftests: Add test for overriding global data value before load

[Andrii Nakryiko]

On Sun, Mar 29, 2020 at 6:23 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> This adds a test to exercise the new bpf_map__set_initial_value() function.
> The test simply overrides the global data section with all zeroes, and
> checks that the new value makes it into the kernel map on load.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

I guess given you don't attach or run any BPF program, it's fine to
reuse test_global_data.o for this.

Acked-by: Andrii Nakryiko <andriin@fb.com>


>  .../bpf/prog_tests/global_data_init.c         | 61 +++++++++++++++++++
>  .../selftests/bpf/progs/test_global_data.c    |  2 +-
>  2 files changed, 62 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/global_data_init.c
>

[...]

[PATCH v3 2/2] selftests: Add test for overriding global data value before load

[Toke Høiland-Jørgensen]

This extends the global_data test to also exercise the new
bpf_map__set_initial_value() function. The test simply overrides the global
data section with all zeroes, and checks that the new value makes it into
the kernel map on load.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 .../selftests/bpf/prog_tests/global_data.c    | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/global_data.c b/tools/testing/selftests/bpf/prog_tests/global_data.c
index c680926fce73..f018ce53a8d1 100644
--- a/tools/testing/selftests/bpf/prog_tests/global_data.c
+++ b/tools/testing/selftests/bpf/prog_tests/global_data.c
@@ -121,6 +121,65 @@ static void test_global_data_rdonly(struct bpf_object *obj, __u32 duration)
 	      "err %d errno %d\n", err, errno);
 }
 
+static void test_global_data_set_rdonly(__u32 duration)
+{
+	const char *file = "./test_global_data.o";
+	int err = -ENOMEM, map_fd, zero = 0;
+	__u8 *buff = NULL, *newval = NULL;
+	struct bpf_program *prog;
+	struct bpf_object *obj;
+	struct bpf_map *map;
+	size_t sz;
+
+	obj = bpf_object__open_file(file, NULL);
+	if (CHECK_FAIL(!obj))
+		return;
+	prog = bpf_program__next(NULL, obj);
+	if (CHECK_FAIL(!prog))
+		goto out;
+	err = bpf_program__set_sched_cls(prog);
+	if (CHECK_FAIL(err))
+		goto out;
+
+	map = bpf_object__find_map_by_name(obj, "test_glo.rodata");
+	if (CHECK_FAIL(!map || !bpf_map__is_internal(map)))
+		goto out;
+
+	sz = bpf_map__def(map)->value_size;
+	newval = malloc(sz);
+	if (CHECK_FAIL(!newval))
+		goto out;
+	memset(newval, 0, sz);
+
+	/* wrong size, should fail */
+	err = bpf_map__set_initial_value(map, newval, sz - 1);
+	if (CHECK(!err, "reject set initial value wrong size", "err %d\n", err))
+		goto out;
+
+	err = bpf_map__set_initial_value(map, newval, sz);
+	if (CHECK_FAIL(err))
+		goto out;
+
+	err = bpf_object__load(obj);
+	if (CHECK_FAIL(err))
+		goto out;
+
+	map_fd = bpf_map__fd(map);
+	if (CHECK_FAIL(map_fd < 0))
+		goto out;
+
+	buff = malloc(sz);
+	if (buff)
+		err = bpf_map_lookup_elem(map_fd, &zero, buff);
+	CHECK(!buff || err || memcmp(buff, newval, sz),
+	      "compare .rodata map data override",
+	      "err %d errno %d\n", err, errno);
+out:
+	free(buff);
+	free(newval);
+	bpf_object__close(obj);
+}
+
 void test_global_data(void)
 {
 	const char *file = "./test_global_data.o";
@@ -144,4 +203,6 @@ void test_global_data(void)
 	test_global_data_rdonly(obj, duration);
 
 	bpf_object__close(obj);
+
+	test_global_data_set_rdonly(duration);
 }
-- 
2.26.0

Re: [PATCH v3 2/2] selftests: Add test for overriding global data value before load

[Andrii Nakryiko]

On Sat, Mar 28, 2020 at 11:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> This extends the global_data test to also exercise the new
> bpf_map__set_initial_value() function. The test simply overrides the global
> data section with all zeroes, and checks that the new value makes it into
> the kernel map on load.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  .../selftests/bpf/prog_tests/global_data.c    | 61 +++++++++++++++++++
>  1 file changed, 61 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/global_data.c b/tools/testing/selftests/bpf/prog_tests/global_data.c
> index c680926fce73..f018ce53a8d1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/global_data.c
> +++ b/tools/testing/selftests/bpf/prog_tests/global_data.c
> @@ -121,6 +121,65 @@ static void test_global_data_rdonly(struct bpf_object *obj, __u32 duration)
>               "err %d errno %d\n", err, errno);
>  }
>
> +static void test_global_data_set_rdonly(__u32 duration)
> +{
> +       const char *file = "./test_global_data.o";
> +       int err = -ENOMEM, map_fd, zero = 0;
> +       __u8 *buff = NULL, *newval = NULL;
> +       struct bpf_program *prog;
> +       struct bpf_object *obj;
> +       struct bpf_map *map;
> +       size_t sz;
> +
> +       obj = bpf_object__open_file(file, NULL);

Try using skeleton open and load .o file, it will cut this code almost in half.

> +       if (CHECK_FAIL(!obj))
> +               return;
> +       prog = bpf_program__next(NULL, obj);
> +       if (CHECK_FAIL(!prog))
> +               goto out;
> +       err = bpf_program__set_sched_cls(prog);

Please fix SEC() name for that program instead of setting type explicitly.

> +       if (CHECK_FAIL(err))
> +               goto out;
> +
> +       map = bpf_object__find_map_by_name(obj, "test_glo.rodata");
> +       if (CHECK_FAIL(!map || !bpf_map__is_internal(map)))
> +               goto out;
> +
> +       sz = bpf_map__def(map)->value_size;
> +       newval = malloc(sz);
> +       if (CHECK_FAIL(!newval))
> +               goto out;
> +       memset(newval, 0, sz);
> +
> +       /* wrong size, should fail */
> +       err = bpf_map__set_initial_value(map, newval, sz - 1);
> +       if (CHECK(!err, "reject set initial value wrong size", "err %d\n", err))
> +               goto out;
> +
> +       err = bpf_map__set_initial_value(map, newval, sz);
> +       if (CHECK_FAIL(err))
> +               goto out;
> +
> +       err = bpf_object__load(obj);
> +       if (CHECK_FAIL(err))
> +               goto out;
> +
> +       map_fd = bpf_map__fd(map);
> +       if (CHECK_FAIL(map_fd < 0))
> +               goto out;
> +
> +       buff = malloc(sz);
> +       if (buff)
> +               err = bpf_map_lookup_elem(map_fd, &zero, buff);
> +       CHECK(!buff || err || memcmp(buff, newval, sz),
> +             "compare .rodata map data override",
> +             "err %d errno %d\n", err, errno);
> +out:
> +       free(buff);
> +       free(newval);
> +       bpf_object__close(obj);
> +}
> +
>  void test_global_data(void)
>  {
>         const char *file = "./test_global_data.o";
> @@ -144,4 +203,6 @@ void test_global_data(void)
>         test_global_data_rdonly(obj, duration);
>
>         bpf_object__close(obj);
> +
> +       test_global_data_set_rdonly(duration);

This should either be a sub-test or better yet a separate test altogether.

>  }
> --
> 2.26.0
>

Re: [PATCH v3 2/2] selftests: Add test for overriding global data value before load

[Toke Høiland-Jørgensen]

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

> On Sat, Mar 28, 2020 at 11:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> This extends the global_data test to also exercise the new
>> bpf_map__set_initial_value() function. The test simply overrides the global
>> data section with all zeroes, and checks that the new value makes it into
>> the kernel map on load.
>>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>>  .../selftests/bpf/prog_tests/global_data.c    | 61 +++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/global_data.c b/tools/testing/selftests/bpf/prog_tests/global_data.c
>> index c680926fce73..f018ce53a8d1 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/global_data.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/global_data.c
>> @@ -121,6 +121,65 @@ static void test_global_data_rdonly(struct bpf_object *obj, __u32 duration)
>>               "err %d errno %d\n", err, errno);
>>  }
>>
>> +static void test_global_data_set_rdonly(__u32 duration)
>> +{
>> +       const char *file = "./test_global_data.o";
>> +       int err = -ENOMEM, map_fd, zero = 0;
>> +       __u8 *buff = NULL, *newval = NULL;
>> +       struct bpf_program *prog;
>> +       struct bpf_object *obj;
>> +       struct bpf_map *map;
>> +       size_t sz;
>> +
>> +       obj = bpf_object__open_file(file, NULL);
>
> Try using skeleton open and load .o file, it will cut this code almost
> in half.

Doesn't work, though:

In file included from /home/build/linux/tools/testing/selftests/bpf/prog_tests/global_data_init.c:3:
/home/build/linux/tools/testing/selftests/bpf/test_global_data.skel.h:31:14: error: field ‘struct1’ has incomplete type
   31 |   struct foo struct1;
      |              ^~~~~~~
/home/build/linux/tools/testing/selftests/bpf/test_global_data.skel.h:37:14: error: field ‘struct3’ has incomplete type
   37 |   struct foo struct3;
      |              ^~~~~~~
/home/build/linux/tools/testing/selftests/bpf/test_global_data.skel.h:45:14: error: field ‘struct0’ has incomplete type
   45 |   struct foo struct0;
      |              ^~~~~~~
/home/build/linux/tools/testing/selftests/bpf/test_global_data.skel.h:46:14: error: field ‘struct2’ has incomplete type
   46 |   struct foo struct2;
      |              ^~~~~~~
make: *** [Makefile:361: /home/build/linux/tools/testing/selftests/bpf/global_data_init.test.o] Error 1

Just fixing the program SEC name as you suggested below already gets rid
of half the setup code, though, so doesn't really make much difference
anyway :)

>> +       if (CHECK_FAIL(!obj))
>> +               return;
>> +       prog = bpf_program__next(NULL, obj);
>> +       if (CHECK_FAIL(!prog))
>> +               goto out;
>> +       err = bpf_program__set_sched_cls(prog);
>
> Please fix SEC() name for that program instead of setting type explicitly.

Yeah, that helps, thanks!

>> +       if (CHECK_FAIL(err))
>> +               goto out;
>> +
>> +       map = bpf_object__find_map_by_name(obj, "test_glo.rodata");
>> +       if (CHECK_FAIL(!map || !bpf_map__is_internal(map)))
>> +               goto out;
>> +
>> +       sz = bpf_map__def(map)->value_size;
>> +       newval = malloc(sz);
>> +       if (CHECK_FAIL(!newval))
>> +               goto out;
>> +       memset(newval, 0, sz);
>> +
>> +       /* wrong size, should fail */
>> +       err = bpf_map__set_initial_value(map, newval, sz - 1);
>> +       if (CHECK(!err, "reject set initial value wrong size", "err %d\n", err))
>> +               goto out;
>> +
>> +       err = bpf_map__set_initial_value(map, newval, sz);
>> +       if (CHECK_FAIL(err))
>> +               goto out;
>> +
>> +       err = bpf_object__load(obj);
>> +       if (CHECK_FAIL(err))
>> +               goto out;
>> +
>> +       map_fd = bpf_map__fd(map);
>> +       if (CHECK_FAIL(map_fd < 0))
>> +               goto out;
>> +
>> +       buff = malloc(sz);
>> +       if (buff)
>> +               err = bpf_map_lookup_elem(map_fd, &zero, buff);
>> +       CHECK(!buff || err || memcmp(buff, newval, sz),
>> +             "compare .rodata map data override",
>> +             "err %d errno %d\n", err, errno);
>> +out:
>> +       free(buff);
>> +       free(newval);
>> +       bpf_object__close(obj);
>> +}
>> +
>>  void test_global_data(void)
>>  {
>>         const char *file = "./test_global_data.o";
>> @@ -144,4 +203,6 @@ void test_global_data(void)
>>         test_global_data_rdonly(obj, duration);
>>
>>         bpf_object__close(obj);
>> +
>> +       test_global_data_set_rdonly(duration);
>
> This should either be a sub-test or better yet a separate test
> altogether.

Sure, will move it to its own file.

-Toke

Re: [PATCH v3 2/2] selftests: Add test for overriding global data value before load

[Andrii Nakryiko]

On Sun, Mar 29, 2020 at 6:10 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Sat, Mar 28, 2020 at 11:29 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> This extends the global_data test to also exercise the new
> >> bpf_map__set_initial_value() function. The test simply overrides the global
> >> data section with all zeroes, and checks that the new value makes it into
> >> the kernel map on load.
> >>
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> ---
> >>  .../selftests/bpf/prog_tests/global_data.c    | 61 +++++++++++++++++++
> >>  1 file changed, 61 insertions(+)
> >>
> >> diff --git a/tools/testing/selftests/bpf/prog_tests/global_data.c b/tools/testing/selftests/bpf/prog_tests/global_data.c
> >> index c680926fce73..f018ce53a8d1 100644
> >> --- a/tools/testing/selftests/bpf/prog_tests/global_data.c
> >> +++ b/tools/testing/selftests/bpf/prog_tests/global_data.c
> >> @@ -121,6 +121,65 @@ static void test_global_data_rdonly(struct bpf_object *obj, __u32 duration)
> >>               "err %d errno %d\n", err, errno);
> >>  }
> >>
> >> +static void test_global_data_set_rdonly(__u32 duration)
> >> +{
> >> +       const char *file = "./test_global_data.o";
> >> +       int err = -ENOMEM, map_fd, zero = 0;
> >> +       __u8 *buff = NULL, *newval = NULL;
> >> +       struct bpf_program *prog;
> >> +       struct bpf_object *obj;
> >> +       struct bpf_map *map;
> >> +       size_t sz;
> >> +
> >> +       obj = bpf_object__open_file(file, NULL);
> >
> > Try using skeleton open and load .o file, it will cut this code almost
> > in half.
>
> Doesn't work, though:

Right, because test_global_data uses BPF-only struct definitions. In
real life those types should be shared in a common header file.

For this feature, I'd suggest to create a separate trivial BPF program
with a simple trivial BPF program and single global variable. There is
nothing you really need from test_global_data setup.

>
> In file included from /home/build/linux/tools/testing/selftests/bpf/prog_tests/global_data_init.c:3:
> /home/build/linux/tools/testing/selftests/bpf/test_global_data.skel.h:31:14: error: field ‘struct1’ has incomplete type
>    31 |   struct foo struct1;
>       |              ^~~~~~~
> /home/build/linux/tools/testing/selftests/bpf/test_global_data.skel.h:37:14: error: field ‘struct3’ has incomplete type
>    37 |   struct foo struct3;
>       |              ^~~~~~~
> /home/build/linux/tools/testing/selftests/bpf/test_global_data.skel.h:45:14: error: field ‘struct0’ has incomplete type
>    45 |   struct foo struct0;
>       |              ^~~~~~~
> /home/build/linux/tools/testing/selftests/bpf/test_global_data.skel.h:46:14: error: field ‘struct2’ has incomplete type
>    46 |   struct foo struct2;
>       |              ^~~~~~~
> make: *** [Makefile:361: /home/build/linux/tools/testing/selftests/bpf/global_data_init.test.o] Error 1
>
> Just fixing the program SEC name as you suggested below already gets rid
> of half the setup code, though, so doesn't really make much difference
> anyway :)
>
> >> +       if (CHECK_FAIL(!obj))
> >> +               return;
> >> +       prog = bpf_program__next(NULL, obj);
> >> +       if (CHECK_FAIL(!prog))
> >> +               goto out;
> >> +       err = bpf_program__set_sched_cls(prog);
> >
> > Please fix SEC() name for that program instead of setting type explicitly.
>
> Yeah, that helps, thanks!
>
> >> +       if (CHECK_FAIL(err))
> >> +               goto out;
> >> +
> >> +       map = bpf_object__find_map_by_name(obj, "test_glo.rodata");
> >> +       if (CHECK_FAIL(!map || !bpf_map__is_internal(map)))
> >> +               goto out;
> >> +
> >> +       sz = bpf_map__def(map)->value_size;
> >> +       newval = malloc(sz);
> >> +       if (CHECK_FAIL(!newval))
> >> +               goto out;
> >> +       memset(newval, 0, sz);
> >> +
> >> +       /* wrong size, should fail */
> >> +       err = bpf_map__set_initial_value(map, newval, sz - 1);
> >> +       if (CHECK(!err, "reject set initial value wrong size", "err %d\n", err))
> >> +               goto out;
> >> +
> >> +       err = bpf_map__set_initial_value(map, newval, sz);
> >> +       if (CHECK_FAIL(err))
> >> +               goto out;
> >> +
> >> +       err = bpf_object__load(obj);
> >> +       if (CHECK_FAIL(err))
> >> +               goto out;
> >> +
> >> +       map_fd = bpf_map__fd(map);
> >> +       if (CHECK_FAIL(map_fd < 0))
> >> +               goto out;
> >> +
> >> +       buff = malloc(sz);
> >> +       if (buff)
> >> +               err = bpf_map_lookup_elem(map_fd, &zero, buff);
> >> +       CHECK(!buff || err || memcmp(buff, newval, sz),
> >> +             "compare .rodata map data override",
> >> +             "err %d errno %d\n", err, errno);
> >> +out:
> >> +       free(buff);
> >> +       free(newval);
> >> +       bpf_object__close(obj);
> >> +}
> >> +
> >>  void test_global_data(void)
> >>  {
> >>         const char *file = "./test_global_data.o";
> >> @@ -144,4 +203,6 @@ void test_global_data(void)
> >>         test_global_data_rdonly(obj, duration);
> >>
> >>         bpf_object__close(obj);
> >> +
> >> +       test_global_data_set_rdonly(duration);
> >
> > This should either be a sub-test or better yet a separate test
> > altogether.
>
> Sure, will move it to its own file.
>
> -Toke
>