[bpf-next] libbpf: use original struct sizes for skeleton iterations

Commit Message

Mykyta Yatsenko [e-mail] July 2, 2024, 7:59 p.m. UTC

From: Mykyta Yatsenko <[e-mail]>

BPF skeletons can be created by previous or newer versions of libbpf,
for which sizeof(bpf_map_skeleton) or bpf_prog_skeleton may differ.
Libbpf should not rely on array indexing, but instead use original
sizes via map_skel_sz/prog_skel_sz for iterating over the
arrays of maps/progs.

Signed-off-by: Mykyta Yatsenko <[e-mail]>
---
 tools/lib/bpf/libbpf.c | 52 +++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 19 deletions(-)

Comments

Andrii Nakryiko July 2, 2024, 9:01 p.m. UTC | #1

On Tue, Jul 2, 2024 at 1:12 PM Mykyta Yatsenko
<[e-mail]> wrote:
>
> From: Mykyta Yatsenko <[e-mail]>
>
> BPF skeletons can be created by previous or newer versions of libbpf,
> for which sizeof(bpf_map_skeleton) or bpf_prog_skeleton may differ.
> Libbpf should not rely on array indexing, but instead use original
> sizes via map_skel_sz/prog_skel_sz for iterating over the
> arrays of maps/progs.
>
> Signed-off-by: Mykyta Yatsenko <[e-mail]>
> ---
>  tools/lib/bpf/libbpf.c | 52 +++++++++++++++++++++++++++---------------
>  1 file changed, 33 insertions(+), 19 deletions(-)
>

See mechanical suggestions below.

But importantly we should also try to make new generated skeleton
files as backwards compatible with all the libbpf versions with this
bug as possible. Can you add also bpftool patch that would keep
map_skel_sz at old value (not including new link field) if there are
no auto-attachable maps? This way latest bpftool won't break currently
released libbpf versions.

pw-bot: cr


> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 4a28fac4908a..b46cf29d74b6 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -13712,14 +13712,16 @@ int libbpf_num_possible_cpus(void)
>
>  static int populate_skeleton_maps(const struct bpf_object *obj,

we can pass `struct bpf_object_skeleton *s` here instead of obj and
maps (they are accessible as s->obj and s->maps)

>                                   struct bpf_map_skeleton *maps,
> -                                 size_t map_cnt)
> +                                 size_t map_cnt, size_t map_sz)

and then we don't need to pass map_sz, it's s->map_skel_sz


>  {
>         int i;
>
>         for (i = 0; i < map_cnt; i++) {
> -               struct bpf_map **map = maps[i].map;
> -               const char *name = maps[i].name;
> -               void **mmaped = maps[i].mmaped;
> +               struct bpf_map_skeleton *map_skel = (struct bpf_map_skeleton *)
> +                       ((char *)maps + map_sz * i);

unnecessary casts, try something like:

struct bpf_map_skeleton *map_skel = (void *)s->maps + skel->map_skel_sz * i;

fits on single line :)


> +               struct bpf_map **map = map_skel->map;
> +               const char *name = map_skel->name;
> +               void **mmaped = map_skel->mmaped;
>
>                 *map = bpf_object__find_map_by_name(obj, name);
>                 if (!*map) {
> @@ -13736,13 +13738,15 @@ static int populate_skeleton_maps(const struct bpf_object *obj,
>
>  static int populate_skeleton_progs(const struct bpf_object *obj,
>                                    struct bpf_prog_skeleton *progs,
> -                                  size_t prog_cnt)
> +                                  size_t prog_cnt, size_t prog_sz)
>  {
>         int i;
>
>         for (i = 0; i < prog_cnt; i++) {
> -               struct bpf_program **prog = progs[i].prog;
> -               const char *name = progs[i].name;
> +               struct bpf_prog_skeleton *cur_prog = (struct bpf_prog_skeleton *)
> +                       ((char *)progs + prog_sz * i);
> +               struct bpf_program **prog = cur_prog->prog;
> +               const char *name = cur_prog->name;
>

same suggestion as for populate_skeleton_maps

>                 *prog = bpf_object__find_program_by_name(obj, name);
>                 if (!*prog) {
> @@ -13783,13 +13787,13 @@ int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
>         }
>
>         *s->obj = obj;
> -       err = populate_skeleton_maps(obj, s->maps, s->map_cnt);
> +       err = populate_skeleton_maps(obj, s->maps, s->map_cnt, s->map_skel_sz);
>         if (err) {
>                 pr_warn("failed to populate skeleton maps for '%s': %d\n", s->name, err);
>                 return libbpf_err(err);
>         }
>
> -       err = populate_skeleton_progs(obj, s->progs, s->prog_cnt);
> +       err = populate_skeleton_progs(obj, s->progs, s->prog_cnt, s->prog_skel_sz);
>         if (err) {
>                 pr_warn("failed to populate skeleton progs for '%s': %d\n", s->name, err);
>                 return libbpf_err(err);
> @@ -13819,13 +13823,13 @@ int bpf_object__open_subskeleton(struct bpf_object_subskeleton *s)
>                 return libbpf_err(-errno);
>         }
>
> -       err = populate_skeleton_maps(s->obj, s->maps, s->map_cnt);
> +       err = populate_skeleton_maps(s->obj, s->maps, s->map_cnt, s->map_skel_sz);
>         if (err) {
>                 pr_warn("failed to populate subskeleton maps: %d\n", err);
>                 return libbpf_err(err);
>         }
>
> -       err = populate_skeleton_progs(s->obj, s->progs, s->prog_cnt);
> +       err = populate_skeleton_progs(s->obj, s->progs, s->prog_cnt, s->prog_skel_sz);
>         if (err) {
>                 pr_warn("failed to populate subskeleton maps: %d\n", err);
>                 return libbpf_err(err);
> @@ -13879,10 +13883,12 @@ int bpf_object__load_skeleton(struct bpf_object_skeleton *s)
>         }
>
>         for (i = 0; i < s->map_cnt; i++) {
> -               struct bpf_map *map = *s->maps[i].map;
> +               struct bpf_map_skeleton *map_skel = (struct bpf_map_skeleton *)
> +                       ((char *)s->maps + s->map_skel_sz * i);

same as above

> +               struct bpf_map *map = *map_skel->map;
>                 size_t mmap_sz = bpf_map_mmap_sz(map);
>                 int prot, map_fd = map->fd;
> -               void **mmaped = s->maps[i].mmaped;
> +               void **mmaped = map_skel->mmaped;
>
>                 if (!mmaped)
>                         continue;
> @@ -13930,8 +13936,10 @@ int bpf_object__attach_skeleton(struct bpf_object_skeleton *s)
>         int i, err;
>
>         for (i = 0; i < s->prog_cnt; i++) {
> -               struct bpf_program *prog = *s->progs[i].prog;
> -               struct bpf_link **link = s->progs[i].link;
> +               struct bpf_prog_skeleton *prog_skel = (struct bpf_prog_skeleton *)
> +                       ((char *)s->progs + s->prog_skel_sz * i);
> +               struct bpf_program *prog = *prog_skel->prog;
> +               struct bpf_link **link = prog_skel->link;
>

ditto

>                 if (!prog->autoload || !prog->autoattach)
>                         continue;

[...]

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 4a28fac4908a..b46cf29d74b6 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -13712,14 +13712,16 @@  int libbpf_num_possible_cpus(void)
 
 static int populate_skeleton_maps(const struct bpf_object *obj,
 				  struct bpf_map_skeleton *maps,
-				  size_t map_cnt)
+				  size_t map_cnt, size_t map_sz)
 {
 	int i;
 
 	for (i = 0; i < map_cnt; i++) {
-		struct bpf_map **map = maps[i].map;
-		const char *name = maps[i].name;
-		void **mmaped = maps[i].mmaped;
+		struct bpf_map_skeleton *map_skel = (struct bpf_map_skeleton *)
+			((char *)maps + map_sz * i);
+		struct bpf_map **map = map_skel->map;
+		const char *name = map_skel->name;
+		void **mmaped = map_skel->mmaped;
 
 		*map = bpf_object__find_map_by_name(obj, name);
 		if (!*map) {
@@ -13736,13 +13738,15 @@  static int populate_skeleton_maps(const struct bpf_object *obj,
 
 static int populate_skeleton_progs(const struct bpf_object *obj,
 				   struct bpf_prog_skeleton *progs,
-				   size_t prog_cnt)
+				   size_t prog_cnt, size_t prog_sz)
 {
 	int i;
 
 	for (i = 0; i < prog_cnt; i++) {
-		struct bpf_program **prog = progs[i].prog;
-		const char *name = progs[i].name;
+		struct bpf_prog_skeleton *cur_prog = (struct bpf_prog_skeleton *)
+			((char *)progs + prog_sz * i);
+		struct bpf_program **prog = cur_prog->prog;
+		const char *name = cur_prog->name;
 
 		*prog = bpf_object__find_program_by_name(obj, name);
 		if (!*prog) {
@@ -13783,13 +13787,13 @@  int bpf_object__open_skeleton(struct bpf_object_skeleton *s,
 	}
 
 	*s->obj = obj;
-	err = populate_skeleton_maps(obj, s->maps, s->map_cnt);
+	err = populate_skeleton_maps(obj, s->maps, s->map_cnt, s->map_skel_sz);
 	if (err) {
 		pr_warn("failed to populate skeleton maps for '%s': %d\n", s->name, err);
 		return libbpf_err(err);
 	}
 
-	err = populate_skeleton_progs(obj, s->progs, s->prog_cnt);
+	err = populate_skeleton_progs(obj, s->progs, s->prog_cnt, s->prog_skel_sz);
 	if (err) {
 		pr_warn("failed to populate skeleton progs for '%s': %d\n", s->name, err);
 		return libbpf_err(err);
@@ -13819,13 +13823,13 @@  int bpf_object__open_subskeleton(struct bpf_object_subskeleton *s)
 		return libbpf_err(-errno);
 	}
 
-	err = populate_skeleton_maps(s->obj, s->maps, s->map_cnt);
+	err = populate_skeleton_maps(s->obj, s->maps, s->map_cnt, s->map_skel_sz);
 	if (err) {
 		pr_warn("failed to populate subskeleton maps: %d\n", err);
 		return libbpf_err(err);
 	}
 
-	err = populate_skeleton_progs(s->obj, s->progs, s->prog_cnt);
+	err = populate_skeleton_progs(s->obj, s->progs, s->prog_cnt, s->prog_skel_sz);
 	if (err) {
 		pr_warn("failed to populate subskeleton maps: %d\n", err);
 		return libbpf_err(err);
@@ -13879,10 +13883,12 @@  int bpf_object__load_skeleton(struct bpf_object_skeleton *s)
 	}
 
 	for (i = 0; i < s->map_cnt; i++) {
-		struct bpf_map *map = *s->maps[i].map;
+		struct bpf_map_skeleton *map_skel = (struct bpf_map_skeleton *)
+			((char *)s->maps + s->map_skel_sz * i);
+		struct bpf_map *map = *map_skel->map;
 		size_t mmap_sz = bpf_map_mmap_sz(map);
 		int prot, map_fd = map->fd;
-		void **mmaped = s->maps[i].mmaped;
+		void **mmaped = map_skel->mmaped;
 
 		if (!mmaped)
 			continue;
@@ -13930,8 +13936,10 @@  int bpf_object__attach_skeleton(struct bpf_object_skeleton *s)
 	int i, err;
 
 	for (i = 0; i < s->prog_cnt; i++) {
-		struct bpf_program *prog = *s->progs[i].prog;
-		struct bpf_link **link = s->progs[i].link;
+		struct bpf_prog_skeleton *prog_skel = (struct bpf_prog_skeleton *)
+			((char *)s->progs + s->prog_skel_sz * i);
+		struct bpf_program *prog = *prog_skel->prog;
+		struct bpf_link **link = prog_skel->link;
 
 		if (!prog->autoload || !prog->autoattach)
 			continue;
@@ -13970,8 +13978,10 @@  int bpf_object__attach_skeleton(struct bpf_object_skeleton *s)
 		return 0;
 
 	for (i = 0; i < s->map_cnt; i++) {
-		struct bpf_map *map = *s->maps[i].map;
-		struct bpf_link **link = s->maps[i].link;
+		struct bpf_map_skeleton *map_skel = (struct bpf_map_skeleton *)
+			((char *)s->maps + s->map_skel_sz * i);
+		struct bpf_map *map = *map_skel->map;
+		struct bpf_link **link = map_skel->link;
 
 		if (!map->autocreate || !map->autoattach)
 			continue;
@@ -14000,7 +14010,9 @@  void bpf_object__detach_skeleton(struct bpf_object_skeleton *s)
 	int i;
 
 	for (i = 0; i < s->prog_cnt; i++) {
-		struct bpf_link **link = s->progs[i].link;
+		struct bpf_prog_skeleton *prog_skel = (struct bpf_prog_skeleton *)
+			((char *)s->progs + s->prog_skel_sz * i);
+		struct bpf_link **link = prog_skel->link;
 
 		bpf_link__destroy(*link);
 		*link = NULL;
@@ -14010,7 +14022,9 @@  void bpf_object__detach_skeleton(struct bpf_object_skeleton *s)
 		return;
 
 	for (i = 0; i < s->map_cnt; i++) {
-		struct bpf_link **link = s->maps[i].link;
+		struct bpf_map_skeleton *map_skel = (struct bpf_map_skeleton *)
+			((char *)s->maps + s->map_skel_sz * i);
+		struct bpf_link **link = map_skel->link;
 
 		if (link) {
 			bpf_link__destroy(*link);