bpftool: support loading flow dissector

PATCH In review Stanislav Fomichev 6 participants 16 messages Nov 9, 2018 – Nov 12, 2018

Subsystems (7)

Networking Network drivers Kselftest BPF +3 more subsystems

lore

Share & tools

lore

Stanislav Fomichev

author

2018-11-09 16:21 UTC #0 lore raw

v5 cover bpftool: support loading flow dissector

v5 changes:
* FILE -> PATH for load/loadall (can be either file or directory now)
* simpler implementation for __bpf_program__pin_name
* removed p_err for REQ_ARGS checks
* parse_atach_detach_args -> parse_attach_detach_args
* for -> while in bpf_object__pin_{programs,maps} recovery

v4 changes:
* addressed another round of comments/style issues from Jakub Kicinski & Quentin Monnet (thanks!)
* implemented bpf_object__pin_maps and bpf_object__pin_programs helpers and used them in bpf_program__pin
* added new pin_name to bpf_program so bpf_program__pin
works with sections that contain '/'
* moved *loadall* command implementation into a separate patch
* added patch that implements *pinmaps* to pin maps when doing load/loadall

v3 changes:
* (maybe) better cleanup for partial failure in bpf_object__pin
* added special case in bpf_program__pin for programs with single instances

v2 changes:
* addressed comments/style issues from Jakub Kicinski & Quentin Monnet
* removed logic that populates jump table
* added cleanup for partial failure in bpf_object__pin

This patch series adds support for loading and attaching flow dissector programs from the bpftool:

* first patch fixes flow dissector section name in the selftests (so libbpf auto-detection works)
* second patch adds proper cleanup to bpf_object__pin, parts of which are now being used to attach all flow dissector progs/maps
* third patch adds special case in bpf_program__pin for programs with single instances (we don't create <prog>/0 pin anymore, just <prog>)
* forth patch adds pin_name to the bpf_program struct
which is now used as a pin name in bpf_program__pin et al
* fifth patch adds *loadall* command that pins all programs, not just the first one
* sixth patch adds *pinmaps* argument to load/loadall to let users pin all maps of the obj file
* seventh patch adds actual flow_dissector support to the bpftool and an example

Stanislav Fomichev (7):
selftests/bpf: rename flow dissector section to flow_dissector
libbpf: cleanup after partial failure in bpf_object__pin
libbpf: bpf_program__pin: add special case for instances.nr == 1
libbpf: add internal pin_name
bpftool: add loadall command
bpftool: add pinmaps argument to the load/loadall
bpftool: support loading flow dissector

 .../bpftool/Documentation/bpftool-prog.rst    |  42 +-
 tools/bpf/bpftool/bash-completion/bpftool     |  21 +-
 tools/bpf/bpftool/common.c                    |  31 +-
 tools/bpf/bpftool/main.h                      |   1 +
 tools/bpf/bpftool/prog.c                      | 183 ++++++---
 tools/lib/bpf/libbpf.c                        | 359 ++++++++++++++++--
 tools/lib/bpf/libbpf.h                        |  18 +
 tools/testing/selftests/bpf/bpf_flow.c        |   2 +-
 .../selftests/bpf/test_flow_dissector.sh      |   2 +-
 9 files changed, 537 insertions(+), 122 deletions(-)

Signature · 1 line

2.19.1.930.g4563a0d9d0-goog

Stanislav Fomichev

author

2018-11-09 16:21 UTC #1 lore raw

PATCH v5 1/7 selftests/bpf: rename flow dissector section to flow_dissector

Makes it compatible with the logic that derives program type from section name in libbpf_prog_type_by_name.

Signed-off-by Stanislav Fomichev <sdf at google.com>

---
 tools/testing/selftests/bpf/bpf_flow.c             | 2 +-
 tools/testing/selftests/bpf/test_flow_dissector.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

tools/testing/selftests/bpf/bpf_flow.c +1 -1

index 107350a7821d..b9798f558ca7 100644
@@ -116,7 +116,7 @@ static __always_inline int parse_eth_proto(struct __sk_buff *skb, __be16 proto)
 	return BPF_DROP;
 }
 
-SEC("dissect")
+SEC("flow_dissector")
 int _dissect(struct __sk_buff *skb)
 {
 	if (!skb->vlan_present)

tools/testing/selftests/bpf/test_flow_dissector.sh +1 -1

index c0fb073b5eab..d23d4da66b83 100755
@@ -59,7 +59,7 @@ else
 fi
 
 # Attach BPF program
-./flow_dissector_load -p bpf_flow.o -s dissect
+./flow_dissector_load -p bpf_flow.o -s flow_dissector
 
 # Setup
 tc qdisc add dev lo ingress

Signature · 1 line

2.19.1.930.g4563a0d9d0-goog

Stanislav Fomichev

author

2018-11-09 16:21 UTC #2 lore raw

PATCH v5 2/7 libbpf: cleanup after partial failure in bpf_object__pin

In reply to Stanislav Fomichev

bpftool will use bpf_object__pin in the next commits to pin all programs and maps from the file; in case of a partial failure, we need to get back to the clean state (undo previous program/map pins).

As part of a cleanup, I've added and exported separate routines to pin all maps (bpf_object__pin_maps) and progs (bpf_object__pin_programs) of an object.

Signed-off-by Stanislav Fomichev <sdf at google.com>

---
 tools/lib/bpf/libbpf.c | 324 ++++++++++++++++++++++++++++++++++++++---
 tools/lib/bpf/libbpf.h |  18 +++
 2 files changed, 319 insertions(+), 23 deletions(-)

tools/lib/bpf/libbpf.c +301 ~~-23~~

index d6e62e90e8d4..341008f47c8a 100644
@@ -1699,6 +1699,34 @@ int bpf_program__pin_instance(struct bpf_program *prog, const char *path,
 	return 0;
 }
 
+int bpf_program__unpin_instance(struct bpf_program *prog, const char *path,
+				int instance)
+{
+	int err;
+
+	err = check_path(path);
+	if (err)
+		return err;
+
+	if (prog == NULL) {
+		pr_warning("invalid program pointer\n");
+		return -EINVAL;
+	}
+
+	if (instance < 0 || instance >= prog->instances.nr) {
+		pr_warning("invalid prog instance %d of prog %s (max %d)\n",
+			   instance, prog->section_name, prog->instances.nr);
+		return -EINVAL;
+	}
+
+	err = unlink(path);
+	if (err != 0)
+		return -errno;
+	pr_debug("unpinned program '%s'\n", path);
+
+	return 0;
+}
+
 static int make_dir(const char *path)
 {
 	char *cp, errmsg[STRERR_BUFSIZE];

@@ -1737,6 +1765,64 @@ int bpf_program__pin(struct bpf_program *prog, const char *path)
 	if (err)
 		return err;
 
+	for (i = 0; i < prog->instances.nr; i++) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%d", path, i);
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin;
+		}
+
+		err = bpf_program__pin_instance(prog, buf, i);
+		if (err)
+			goto err_unpin;
+	}
+
+	return 0;
+
+err_unpin:
+	for (i = i - 1; i >= 0; i--) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%d", path, i);
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		bpf_program__unpin_instance(prog, buf, i);
+	}
+
+	rmdir(path);
+
+	return err;
+}
+
+int bpf_program__unpin(struct bpf_program *prog, const char *path)
+{
+	int i, err;
+
+	err = check_path(path);
+	if (err)
+		return err;
+
+	if (prog == NULL) {
+		pr_warning("invalid program pointer\n");
+		return -EINVAL;
+	}
+
+	if (prog->instances.nr <= 0) {
+		pr_warning("no instances of prog %s to pin\n",
+			   prog->section_name);
+		return -EINVAL;
+	}
+
 	for (i = 0; i < prog->instances.nr; i++) {
 		char buf[PATH_MAX];
 		int len;

@@ -1747,11 +1833,15 @@ int bpf_program__pin(struct bpf_program *prog, const char *path)
 		else if (len >= PATH_MAX)
 			return -ENAMETOOLONG;
 
-		err = bpf_program__pin_instance(prog, buf, i);
+		err = bpf_program__unpin_instance(prog, buf, i);
 		if (err)
 			return err;
 	}
 
+	err = rmdir(path);
+	if (err)
+		return -errno;
+
 	return 0;
 }

@@ -1776,12 +1866,33 @@ int bpf_map__pin(struct bpf_map *map, const char *path)
 	}
 
 	pr_debug("pinned map '%s'\n", path);
+
 	return 0;
 }
 
-int bpf_object__pin(struct bpf_object *obj, const char *path)
+int bpf_map__unpin(struct bpf_map *map, const char *path)
+{
+	int err;
+
+	err = check_path(path);
+	if (err)
+		return err;
+
+	if (map == NULL) {
+		pr_warning("invalid map pointer\n");
+		return -EINVAL;
+	}
+
+	err = unlink(path);
+	if (err != 0)
+		return -errno;
+	pr_debug("unpinned map '%s'\n", path);
+
+	return 0;
+}
+
+int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
 {
-	struct bpf_program *prog;
 	struct bpf_map *map;
 	int err;

@@ -1797,6 +1908,53 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 	if (err)
 		return err;
 
+	bpf_map__for_each(map, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       bpf_map__name(map));
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin_maps;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin_maps;
+		}
+
+		err = bpf_map__pin(map, buf);
+		if (err)
+			goto err_unpin_maps;
+	}
+
+	return 0;
+
+err_unpin_maps:
+	while ((map = bpf_map__prev(map, obj))) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       bpf_map__name(map));
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		bpf_map__unpin(map, buf);
+	}
+
+	return err;
+}
+
+int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
+{
+	struct bpf_map *map;
+	int err;
+
+	if (!obj)
+		return -ENOENT;
+
 	bpf_map__for_each(map, obj) {
 		char buf[PATH_MAX];
 		int len;

@@ -1808,11 +1966,78 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 		else if (len >= PATH_MAX)
 			return -ENAMETOOLONG;
 
-		err = bpf_map__pin(map, buf);
+		err = bpf_map__unpin(map, buf);
 		if (err)
 			return err;
 	}
 
+	return 0;
+}
+
+int bpf_object__pin_programs(struct bpf_object *obj, const char *path)
+{
+	struct bpf_program *prog;
+	int err;
+
+	if (!obj)
+		return -ENOENT;
+
+	if (!obj->loaded) {
+		pr_warning("object not yet loaded; load it first\n");
+		return -ENOENT;
+	}
+
+	err = make_dir(path);
+	if (err)
+		return err;
+
+	bpf_object__for_each_program(prog, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       prog->section_name);
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin_programs;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin_programs;
+		}
+
+		err = bpf_program__pin(prog, buf);
+		if (err)
+			goto err_unpin_programs;
+	}
+
+	return 0;
+
+err_unpin_programs:
+	while ((prog = bpf_program__prev(prog, obj))) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       prog->section_name);
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		bpf_program__unpin(prog, buf);
+	}
+
+	return err;
+}
+
+int bpf_object__unpin_programs(struct bpf_object *obj, const char *path)
+{
+	struct bpf_program *prog;
+	int err;
+
+	if (!obj)
+		return -ENOENT;
+
 	bpf_object__for_each_program(prog, obj) {
 		char buf[PATH_MAX];
 		int len;

@@ -1824,7 +2049,7 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 		else if (len >= PATH_MAX)
 			return -ENAMETOOLONG;
 
-		err = bpf_program__pin(prog, buf);
+		err = bpf_program__unpin(prog, buf);
 		if (err)
 			return err;
 	}

@@ -1832,6 +2057,23 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 	return 0;
 }
 
+int bpf_object__pin(struct bpf_object *obj, const char *path)
+{
+	int err;
+
+	err = bpf_object__pin_maps(obj, path);
+	if (err)
+		return err;
+
+	err = bpf_object__pin_programs(obj, path);
+	if (err) {
+		bpf_object__unpin_maps(obj, path);
+		return err;
+	}
+
+	return 0;
+}
+
 void bpf_object__close(struct bpf_object *obj)
 {
 	size_t i;

@@ -1918,23 +2160,20 @@ void *bpf_object__priv(struct bpf_object *obj)
 }
 
 static struct bpf_program *
-__bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
+__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
 {
-	size_t idx;
+	ssize_t idx;
 
 	if (!obj->programs)
 		return NULL;
-	/* First handler */
-	if (prev == NULL)
-		return &obj->programs[0];
 
-	if (prev->obj != obj) {
+	if (p->obj != obj) {
 		pr_warning("error: program handler doesn't match object\n");
 		return NULL;
 	}
 
-	idx = (prev - obj->programs) + 1;
-	if (idx >= obj->nr_programs)
+	idx = (p - obj->programs) + i;
+	if (idx >= obj->nr_programs || idx < 0)
 		return NULL;
 	return &obj->programs[idx];
 }

@@ -1944,8 +2183,29 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
 	struct bpf_program *prog = prev;
 
+	if (prev == NULL)
+		return obj->programs;
+
 	do {
-		prog = __bpf_program__next(prog, obj);
+		prog = __bpf_program__iter(prog, obj, 1);
+	} while (prog && bpf_program__is_function_storage(prog, obj));
+
+	return prog;
+}
+
+struct bpf_program *
+bpf_program__prev(struct bpf_program *next, struct bpf_object *obj)
+{
+	struct bpf_program *prog = next;
+
+	if (next == NULL) {
+		if (!obj->nr_programs)
+			return NULL;
+		return obj->programs + obj->nr_programs - 1;
+	}
+
+	do {
+		prog = __bpf_program__iter(prog, obj, -1);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

@@ -2272,10 +2532,10 @@ void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex)
 	map->map_ifindex = ifindex;
 }
 
-struct bpf_map *
-bpf_map__next(struct bpf_map *prev, struct bpf_object *obj)
+static struct bpf_map *
+__bpf_map__iter(struct bpf_map *m, struct bpf_object *obj, int i)
 {
-	size_t idx;
+	ssize_t idx;
 	struct bpf_map *s, *e;
 
 	if (!obj || !obj->maps)

@@ -2284,21 +2544,39 @@ bpf_map__next(struct bpf_map *prev, struct bpf_object *obj)
 	s = obj->maps;
 	e = obj->maps + obj->nr_maps;
 
-	if (prev == NULL)
-		return s;
-
-	if ((prev < s) || (prev >= e)) {
+	if ((m < s) || (m >= e)) {
 		pr_warning("error in %s: map handler doesn't belong to object\n",
 			   __func__);
 		return NULL;
 	}
 
-	idx = (prev - obj->maps) + 1;
-	if (idx >= obj->nr_maps)
+	idx = (m - obj->maps) + i;
+	if (idx >= obj->nr_maps || idx < 0)
 		return NULL;
 	return &obj->maps[idx];
 }
 
+struct bpf_map *
+bpf_map__next(struct bpf_map *prev, struct bpf_object *obj)
+{
+	if (prev == NULL)
+		return obj->maps;
+
+	return __bpf_map__iter(prev, obj, 1);
+}
+
+struct bpf_map *
+bpf_map__prev(struct bpf_map *next, struct bpf_object *obj)
+{
+	if (next == NULL) {
+		if (!obj->nr_maps)
+			return NULL;
+		return obj->maps + obj->nr_maps - 1;
+	}
+
+	return __bpf_map__iter(next, obj, -1);
+}
+
 struct bpf_map *
 bpf_object__find_map_by_name(struct bpf_object *obj, const char *name)
 {

tools/lib/bpf/libbpf.h +18 -0

index 1f3468dad8b2..b1686a787102 100644
@@ -71,6 +71,13 @@ struct bpf_object *__bpf_object__open_xattr(struct bpf_object_open_attr *attr,
 LIBBPF_API struct bpf_object *bpf_object__open_buffer(void *obj_buf,
 						      size_t obj_buf_sz,
 						      const char *name);
+LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path);
+LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj,
+				      const char *path);
+LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj,
+					const char *path);
+LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
+					  const char *path);
 LIBBPF_API int bpf_object__pin(struct bpf_object *object, const char *path);
 LIBBPF_API void bpf_object__close(struct bpf_object *object);

@@ -112,6 +119,9 @@ LIBBPF_API struct bpf_program *bpf_program__next(struct bpf_program *prog,
 	     (pos) != NULL;				\
 	     (pos) = bpf_program__next((pos), (obj)))
 
+LIBBPF_API struct bpf_program *bpf_program__prev(struct bpf_program *prog,
+						 struct bpf_object *obj);
+
 typedef void (*bpf_program_clear_priv_t)(struct bpf_program *,
 					 void *);

@@ -131,7 +141,11 @@ LIBBPF_API int bpf_program__fd(struct bpf_program *prog);
 LIBBPF_API int bpf_program__pin_instance(struct bpf_program *prog,
 					 const char *path,
 					 int instance);
+LIBBPF_API int bpf_program__unpin_instance(struct bpf_program *prog,
+					   const char *path,
+					   int instance);
 LIBBPF_API int bpf_program__pin(struct bpf_program *prog, const char *path);
+LIBBPF_API int bpf_program__unpin(struct bpf_program *prog, const char *path);
 LIBBPF_API void bpf_program__unload(struct bpf_program *prog);
 
 struct bpf_insn;

@@ -260,6 +274,9 @@ bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
 	     (pos) != NULL;				\
 	     (pos) = bpf_map__next((pos), (obj)))
 
+LIBBPF_API struct bpf_map *
+bpf_map__prev(struct bpf_map *map, struct bpf_object *obj);
+
 LIBBPF_API int bpf_map__fd(struct bpf_map *map);
 LIBBPF_API const struct bpf_map_def *bpf_map__def(struct bpf_map *map);
 LIBBPF_API const char *bpf_map__name(struct bpf_map *map);

@@ -274,6 +291,7 @@ LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd);
 LIBBPF_API bool bpf_map__is_offload_neutral(struct bpf_map *map);
 LIBBPF_API void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex);
 LIBBPF_API int bpf_map__pin(struct bpf_map *map, const char *path);
+LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path);
 
 LIBBPF_API long libbpf_get_error(const void *ptr);

Signature · 1 line

2.19.1.930.g4563a0d9d0-goog

Stanislav Fomichev

author

2018-11-09 16:21 UTC #8 lore raw

PATCH v5 3/7 libbpf: bpf_program__pin: add special case for instances.nr == 1

In reply to Stanislav Fomichev

When bpf_program has only one instance, don't create a subdirectory with per-instance pin files (<prog>/0). Instead, just create a single pin file for that single instance. This simplifies object pinning by not creating unnecessary subdirectories.

This can potentially break existing users that depend on the case where '/0' is always created. However, I couldn't find any serious usage of bpf_program__pin inside the kernel tree and I suppose there should be none outside.

Signed-off-by Stanislav Fomichev <sdf at google.com>

---
 tools/lib/bpf/libbpf.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

tools/lib/bpf/libbpf.c +10 -0

index 341008f47c8a..97ce9f214002 100644
@@ -1761,6 +1761,11 @@ int bpf_program__pin(struct bpf_program *prog, const char *path)
 		return -EINVAL;
 	}
 
+	if (prog->instances.nr == 1) {
+		/* don't create subdirs when pinning single instance */
+		return bpf_program__pin_instance(prog, path, 0);
+	}
+
 	err = make_dir(path);
 	if (err)
 		return err;

@@ -1823,6 +1828,11 @@ int bpf_program__unpin(struct bpf_program *prog, const char *path)
 		return -EINVAL;
 	}
 
+	if (prog->instances.nr == 1) {
+		/* don't create subdirs when pinning single instance */
+		return bpf_program__unpin_instance(prog, path, 0);
+	}
+
 	for (i = 0; i < prog->instances.nr; i++) {
 		char buf[PATH_MAX];
 		int len;

Signature · 1 line

2.19.1.930.g4563a0d9d0-goog

Stanislav Fomichev

author

2018-11-09 16:21 UTC #9 lore raw

PATCH v5 4/7 libbpf: add internal pin_name

In reply to Stanislav Fomichev

pin_name is the same as section_name where '/' is replaced
by '_'. bpf_object__pin_programs is converted to use pin_name
to avoid the situation where section_name would require creating another subdirectory for a pin (as, for example, when calling bpf_object__pin_programs for programs in sections like "cgroup/connect6").

Signed-off-by Stanislav Fomichev <sdf at google.com>

---
 tools/lib/bpf/libbpf.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

tools/lib/bpf/libbpf.c +26 -3

index 97ce9f214002..e827542ffa3a 100644
@@ -124,6 +124,10 @@ struct bpf_program {
 	char *name;
 	int prog_ifindex;
 	char *section_name;
+	/* section_name with / replaced by _; makes recursive pinning
+	 * in bpf_object__pin_programs easier
+	 */
+	char *pin_name;
 	struct bpf_insn *insns;
 	size_t insns_cnt, main_prog_cnt;
 	enum bpf_prog_type type;

@@ -253,6 +257,7 @@ static void bpf_program__exit(struct bpf_program *prog)
 	bpf_program__unload(prog);
 	zfree(&prog->name);
 	zfree(&prog->section_name);
+	zfree(&prog->pin_name);
 	zfree(&prog->insns);
 	zfree(&prog->reloc_desc);

@@ -261,6 +266,17 @@ static void bpf_program__exit(struct bpf_program *prog)
 	prog->idx = -1;
 }
 
+static char *__bpf_program__pin_name(struct bpf_program *prog)
+{
+	char *name, *p;
+
+	name = p = strdup(prog->section_name);
+	while ((p = strchr(p, '/')))
+		*p = '_';
+
+	return name;
+}
+
 static int
 bpf_program__init(void *data, size_t size, char *section_name, int idx,
 		  struct bpf_program *prog)

@@ -279,6 +295,13 @@ bpf_program__init(void *data, size_t size, char *section_name, int idx,
 		goto errout;
 	}
 
+	prog->pin_name = __bpf_program__pin_name(prog);
+	if (!prog->pin_name) {
+		pr_warning("failed to alloc pin name for prog under section(%d) %s\n",
+			   idx, section_name);
+		goto errout;
+	}
+
 	prog->insns = malloc(size);
 	if (!prog->insns) {
 		pr_warning("failed to alloc insns for prog under section %s\n",

@@ -2006,7 +2029,7 @@ int bpf_object__pin_programs(struct bpf_object *obj, const char *path)
 		int len;
 
 		len = snprintf(buf, PATH_MAX, "%s/%s", path,
-			       prog->section_name);
+			       prog->pin_name);
 		if (len < 0) {
 			err = -EINVAL;
 			goto err_unpin_programs;

@@ -2028,7 +2051,7 @@ int bpf_object__pin_programs(struct bpf_object *obj, const char *path)
 		int len;
 
 		len = snprintf(buf, PATH_MAX, "%s/%s", path,
-			       prog->section_name);
+			       prog->pin_name);
 		if (len < 0)
 			continue;
 		else if (len >= PATH_MAX)

@@ -2053,7 +2076,7 @@ int bpf_object__unpin_programs(struct bpf_object *obj, const char *path)
 		int len;
 
 		len = snprintf(buf, PATH_MAX, "%s/%s", path,
-			       prog->section_name);
+			       prog->pin_name);
 		if (len < 0)
 			return -EINVAL;
 		else if (len >= PATH_MAX)

Signature · 1 line

2.19.1.930.g4563a0d9d0-goog

Stanislav Fomichev

author

2018-11-09 16:21 UTC #10 lore raw

PATCH v5 5/7 bpftool: add loadall command

In reply to Stanislav Fomichev

This patch adds new *loadall* command which slightly differs from the existing *load*. *load* command loads all programs from the obj file, but pins only the first programs. *loadall* pins all programs from the obj file under specified directory.

The intended usecase is flow_dissector, where we want to load a bunch of progs, pin them all and after that construct a jump table.

Signed-off-by Stanislav Fomichev <sdf at google.com>

Acked-by Jakub Kicinski <jakub.kicinski at netronome.com>

---
 .../bpftool/Documentation/bpftool-prog.rst    | 14 ++--
 tools/bpf/bpftool/bash-completion/bpftool     |  4 +-
 tools/bpf/bpftool/common.c                    | 31 ++++----
 tools/bpf/bpftool/main.h                      |  1 +
 tools/bpf/bpftool/prog.c                      | 74 ++++++++++++++-----
 5 files changed, 81 insertions(+), 43 deletions(-)

tools/bpf/bpftool/Documentation/bpftool-prog.rst +9 -5

index ac4e904b10fb..984d125c507a 100644
@@ -15,7 +15,8 @@ SYNOPSIS
 	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
 
 	*COMMANDS* :=
-	{ **show** | **list** | **dump xlated** | **dump jited** | **pin** | **load** | **help** }
+	{ **show** | **list** | **dump xlated** | **dump jited** | **pin** | **load**
+	| **loadall** | **help** }
 
 MAP COMMANDS
 =============

@@ -24,7 +25,7 @@ MAP COMMANDS
 |	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual**}]
 |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes**}]
 |	**bpftool** **prog pin** *PROG* *FILE*
-|	**bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
+|	**bpftool** **prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
 |       **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
 |       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
 |	**bpftool** **prog help**

@@ -79,8 +80,11 @@ DESCRIPTION
 		  contain a dot character ('.'), which is reserved for future
 		  extensions of *bpffs*.
 
-	**bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
-		  Load bpf program from binary *OBJ* and pin as *FILE*.
+	**bpftool prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
+		  Load bpf program(s) from binary *OBJ* and pin as *PATH*.
+		  **bpftool prog load** pins only the first program from the
+		  *OBJ* as *PATH*. **bpftool prog loadall** pins all programs
+		  from the *OBJ* under *PATH* directory.
 		  **type** is optional, if not specified program type will be
 		  inferred from section names.
 		  By default bpftool will create new maps as declared in the ELF

@@ -93,7 +97,7 @@ DESCRIPTION
 		  If **dev** *NAME* is specified program will be loaded onto
 		  given networking device (offload).
 
-		  Note: *FILE* must be located in *bpffs* mount. It must not
+		  Note: *PATH* must be located in *bpffs* mount. It must not
 		  contain a dot character ('.'), which is reserved for future
 		  extensions of *bpffs*.

tools/bpf/bpftool/bash-completion/bpftool +2 -2

index 3f78e6404589..780ebafb756a 100644
@@ -243,7 +243,7 @@ _bpftool()
     # Completion depends on object and command in use
     case $object in
         prog)
-            if [[ $command != "load" ]]; then
+            if [[ $command != "load" && $command != "loadall" ]]; then
                 case $prev in
                     id)
                         _bpftool_get_prog_ids

@@ -309,7 +309,7 @@ _bpftool()
                     fi
                     return 0
                     ;;
-                load)
+                load|loadall)
                     local obj
 
                     if [[ ${#words[@]} -lt 6 ]]; then

tools/bpf/bpftool/common.c +15 ~~-16~~

index 25af85304ebe..21ce556c15e1 100644
@@ -169,34 +169,23 @@ int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type)
 	return fd;
 }
 
-int do_pin_fd(int fd, const char *name)
+int mount_bpffs_for_pin(const char *name)
 {
 	char err_str[ERR_MAX_LEN];
 	char *file;
 	char *dir;
 	int err = 0;
 
-	err = bpf_obj_pin(fd, name);
-	if (!err)
-		goto out;
-
 	file = malloc(strlen(name) + 1);
 	strcpy(file, name);
 	dir = dirname(file);
 
-	if (errno != EPERM || is_bpffs(dir)) {
-		p_err("can't pin the object (%s): %s", name, strerror(errno));
+	if (is_bpffs(dir))
+		/* nothing to do if already mounted */
 		goto out_free;
-	}
 
-	/* Attempt to mount bpffs, then retry pinning. */
 	err = mnt_bpffs(dir, err_str, ERR_MAX_LEN);
-	if (!err) {
-		err = bpf_obj_pin(fd, name);
-		if (err)
-			p_err("can't pin the object (%s): %s", name,
-			      strerror(errno));
-	} else {
+	if (err) {
 		err_str[ERR_MAX_LEN - 1] = '\0';
 		p_err("can't mount BPF file system to pin the object (%s): %s",
 		      name, err_str);

@@ -204,10 +193,20 @@ int do_pin_fd(int fd, const char *name)
 
 out_free:
 	free(file);
-out:
 	return err;
 }
 
+int do_pin_fd(int fd, const char *name)
+{
+	int err;
+
+	err = mount_bpffs_for_pin(name);
+	if (err)
+		return err;
+
+	return bpf_obj_pin(fd, name);
+}
+
 int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32))
 {
 	unsigned int id;

tools/bpf/bpftool/main.h +1 -0

index 28322ace2856..1383824c9baf 100644
@@ -129,6 +129,7 @@ const char *get_fd_type_name(enum bpf_obj_type type);
 char *get_fdinfo(int fd, const char *key);
 int open_obj_pinned(char *path);
 int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type);
+int mount_bpffs_for_pin(const char *name);
 int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32));
 int do_pin_fd(int fd, const char *name);

tools/bpf/bpftool/prog.c +54 ~~-20~~

123 lines not on this page · raw message

Signature · 1 line

2.19.1.930.g4563a0d9d0-goog

Stanislav Fomichev

author

2018-11-09 16:21 UTC #11 lore raw

PATCH v5 6/7 bpftool: add pinmaps argument to the load/loadall

In reply to Stanislav Fomichev

This new additional argument lets users pin all maps from the object at specified path.

Signed-off-by Stanislav Fomichev <sdf at google.com>

Acked-by Jakub Kicinski <jakub.kicinski at netronome.com>

---
 .../bpftool/Documentation/bpftool-prog.rst    |  4 +++-
 tools/bpf/bpftool/bash-completion/bpftool     |  3 ++-
 tools/bpf/bpftool/prog.c                      | 24 ++++++++++++++++++-
 3 files changed, 28 insertions(+), 3 deletions(-)

tools/bpf/bpftool/Documentation/bpftool-prog.rst +3 -1

index 984d125c507a..15e9172f7e55 100644
@@ -80,7 +80,7 @@ DESCRIPTION
 		  contain a dot character ('.'), which is reserved for future
 		  extensions of *bpffs*.
 
-	**bpftool prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
+	**bpftool prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] [**pinmaps** *MAP_DIR*]
 		  Load bpf program(s) from binary *OBJ* and pin as *PATH*.
 		  **bpftool prog load** pins only the first program from the
 		  *OBJ* as *PATH*. **bpftool prog loadall** pins all programs

@@ -96,6 +96,8 @@ DESCRIPTION
 		  use, referring to it by **id** or through a **pinned** file.
 		  If **dev** *NAME* is specified program will be loaded onto
 		  given networking device (offload).
+		  Optional **pinmaps** argument can be provided to pin all
+		  maps under *MAP_DIR* directory.
 
 		  Note: *PATH* must be located in *bpffs* mount. It must not
 		  contain a dot character ('.'), which is reserved for future

tools/bpf/bpftool/bash-completion/bpftool +2 -1

index 780ebafb756a..a05d0071f39f 100644
@@ -346,7 +346,7 @@ _bpftool()
                             _bpftool_get_map_ids
                             return 0
                             ;;
-                        pinned)
+                        pinned|pinmaps)
                             _filedir
                             return 0
                             ;;

@@ -358,6 +358,7 @@ _bpftool()
                             COMPREPLY=( $( compgen -W "map" -- "$cur" ) )
                             _bpftool_once_attr 'type'
                             _bpftool_once_attr 'dev'
+                            _bpftool_once_attr 'pinmaps'
                             return 0
                             ;;
                     esac

tools/bpf/bpftool/prog.c +23 -1

index 3abfee5533e6..4bd6ad19fc05 100644
@@ -802,6 +802,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 	struct map_replace *map_replace = NULL;
 	struct bpf_program *prog = NULL, *pos;
 	unsigned int old_map_fds = 0;
+	const char *pinmaps = NULL;
 	struct bpf_object *obj;
 	struct bpf_map *map;
 	const char *pinfile;

@@ -906,6 +907,13 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 				goto err_free_reuse_maps;
 			}
 			NEXT_ARG();
+		} else if (is_prefix(*argv, "pinmaps")) {
+			NEXT_ARG();
+
+			if (!REQ_ARGS(1))
+				goto err_free_reuse_maps;
+
+			pinmaps = GET_ARG();
 		} else {
 			p_err("expected no more arguments, 'type', 'map' or 'dev', got: '%s'?",
 			      *argv);

@@ -1026,6 +1034,14 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 		}
 	}
 
+	if (pinmaps) {
+		err = bpf_object__pin_maps(obj, pinmaps);
+		if (err) {
+			p_err("failed to pin all maps");
+			goto err_unpin;
+		}
+	}
+
 	if (json_output)
 		jsonw_null(json_wtr);

@@ -1036,6 +1052,11 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 
 	return 0;
 
+err_unpin:
+	if (first_prog_only)
+		unlink(pinfile);
+	else
+		bpf_object__unpin_programs(obj, pinfile);
 err_close_obj:
 	bpf_object__close(obj);
 err_free_reuse_maps:

@@ -1069,7 +1090,8 @@ static int do_help(int argc, char **argv)
 		"       %s %s pin   PROG FILE\n"
 		"       %s %s { load | loadall } OBJ  PATH \\\n"
 		"                         [type TYPE] [dev NAME] \\\n"
-		"                         [map { idx IDX | name NAME } MAP]\n"
+		"                         [map { idx IDX | name NAME } MAP]\\\n"
+		"                         [pinmaps MAP_DIR]\n"
 		"       %s %s attach PROG ATTACH_TYPE MAP\n"
 		"       %s %s detach PROG ATTACH_TYPE MAP\n"
 		"       %s %s help\n"

Signature · 1 line

2.19.1.930.g4563a0d9d0-goog

Stanislav Fomichev

author

2018-11-09 16:21 UTC #12 lore raw

PATCH v5 7/7 bpftool: support loading flow dissector

In reply to Stanislav Fomichev

This commit adds support for loading/attaching/detaching flow dissector program.

When bpftool loadall is called with a flow_dissector prog (i.e. when the 'type flow_dissector' argument is passed), we load and pin all programs. User is responsible to construct the jump table for the tail calls.

The last argument of bpftool attach is made optional for this use case.

Example:

bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
        /sys/fs/bpf/flow type flow_dissector \
	pinmaps /sys/fs/bpf/flow

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 0 0 0 0 \
value pinned /sys/fs/bpf/flow/IP

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 1 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 2 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6OP

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 3 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6FR

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 4 0 0 0 \
value pinned /sys/fs/bpf/flow/MPLS

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 5 0 0 0 \
value pinned /sys/fs/bpf/flow/VLAN

bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector

Tested by using the above lines to load the prog in the test_flow_dissector.sh selftest.

Signed-off-by Stanislav Fomichev <sdf at google.com>

Acked-by Jakub Kicinski <jakub.kicinski at netronome.com>

---
 .../bpftool/Documentation/bpftool-prog.rst    | 26 +++---
 tools/bpf/bpftool/bash-completion/bpftool     | 14 ++-
 tools/bpf/bpftool/prog.c                      | 85 ++++++++++---------
 3 files changed, 74 insertions(+), 51 deletions(-)

tools/bpf/bpftool/Documentation/bpftool-prog.rst +16 ~~-10~~

index 15e9172f7e55..8db78ed82a71 100644
@@ -26,8 +26,8 @@ MAP COMMANDS
 |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes**}]
 |	**bpftool** **prog pin** *PROG* *FILE*
 |	**bpftool** **prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
-|       **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
-|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
+|       **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
+|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
 |	**bpftool** **prog help**
 |
 |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }

@@ -40,7 +40,9 @@ MAP COMMANDS
 |		**cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** | **cgroup/post_bind6** |
 |		**cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4** | **cgroup/sendmsg6**
 |	}
-|       *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** | **skb_parse** }
+|       *ATTACH_TYPE* := {
+|		**msg_verdict** | **skb_verdict** | **skb_parse** | **flow_dissector**
+|	}
 
 
 DESCRIPTION

@@ -103,13 +105,17 @@ DESCRIPTION
 		  contain a dot character ('.'), which is reserved for future
 		  extensions of *bpffs*.
 
-        **bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
-                  Attach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
-                  to the map *MAP*.
-
-        **bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
-                  Detach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
-                  from the map *MAP*.
+	**bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
+		  Attach bpf program *PROG* (with type specified by
+		  *ATTACH_TYPE*). Most *ATTACH_TYPEs* require a *MAP*
+		  parameter, with the exception of *flow_dissector* which is
+		  attached to current networking name space.
+
+	**bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
+		  Detach bpf program *PROG* (with type specified by
+		  *ATTACH_TYPE*). Most *ATTACH_TYPEs* require a *MAP*
+		  parameter, with the exception of *flow_dissector* which is
+		  detached from the current networking name space.
 
 	**bpftool prog help**
 		  Print short help message.

tools/bpf/bpftool/bash-completion/bpftool +12 -2

index a05d0071f39f..45c2db257d2b 100644
@@ -299,7 +299,8 @@ _bpftool()
                     fi
 
                     if [[ ${#words[@]} == 6 ]]; then
-                        COMPREPLY=( $( compgen -W "msg_verdict skb_verdict skb_parse" -- "$cur" ) )
+                        COMPREPLY=( $( compgen -W "msg_verdict skb_verdict \
+                            skb_parse flow_dissector" -- "$cur" ) )
                         return 0
                     fi

@@ -338,7 +339,16 @@ _bpftool()
 
                     case $prev in
                         type)
-                            COMPREPLY=( $( compgen -W "socket kprobe kretprobe classifier action tracepoint raw_tracepoint xdp perf_event cgroup/skb cgroup/sock cgroup/dev lwt_in lwt_out lwt_xmit lwt_seg6local sockops sk_skb sk_msg lirc_mode2 cgroup/bind4 cgroup/bind6 cgroup/connect4 cgroup/connect6 cgroup/sendmsg4 cgroup/sendmsg6 cgroup/post_bind4 cgroup/post_bind6" -- \
+                            COMPREPLY=( $( compgen -W "socket kprobe \
+                                kretprobe classifier flow_dissector \
+                                action tracepoint raw_tracepoint \
+                                xdp perf_event cgroup/skb cgroup/sock \
+                                cgroup/dev lwt_in lwt_out lwt_xmit \
+                                lwt_seg6local sockops sk_skb sk_msg \
+                                lirc_mode2 cgroup/bind4 cgroup/bind6 \
+                                cgroup/connect4 cgroup/connect6 \
+                                cgroup/sendmsg4 cgroup/sendmsg6 \
+                                cgroup/post_bind4 cgroup/post_bind6" -- \
                                                    "$cur" ) )
                             return 0
                             ;;

tools/bpf/bpftool/prog.c +46 ~~-39~~

124 lines not on this page · raw message

Signature · 1 line

2.19.1.930.g4563a0d9d0-goog

Quentin Monnet

Acked-by on the v5 series

2018-11-09 17:38 UTC #13 lore raw

In reply to Stanislav Fomichev

2018-11-09 08:21 UTC-0800 ~ Stanislav Fomichev <sdf at google.com>

Quoted 31 lines

v5 changes:
* FILE -> PATH for load/loadall (can be either file or directory now)
* simpler implementation for __bpf_program__pin_name
* removed p_err for REQ_ARGS checks
* parse_atach_detach_args -> parse_attach_detach_args
* for -> while in bpf_object__pin_{programs,maps} recovery
v4 changes:
* addressed another round of comments/style issues from Jakub Kicinski & Quentin Monnet (thanks!)
* implemented bpf_object__pin_maps and bpf_object__pin_programs helpers and used them in bpf_program__pin
* added new pin_name to bpf_program so bpf_program__pin
works with sections that contain '/'
* moved *loadall* command implementation into a separate patch
* added patch that implements *pinmaps* to pin maps when doing load/loadall
v3 changes:
* (maybe) better cleanup for partial failure in bpf_object__pin
* added special case in bpf_program__pin for programs with single instances
v2 changes:
* addressed comments/style issues from Jakub Kicinski & Quentin Monnet
* removed logic that populates jump table
* added cleanup for partial failure in bpf_object__pin
This patch series adds support for loading and attaching flow dissector programs from the bpftool:

Quoted 14 lines

* first patch fixes flow dissector section name in the selftests (so libbpf auto-detection works)
* second patch adds proper cleanup to bpf_object__pin, parts of which are now being used to attach all flow dissector progs/maps
* third patch adds special case in bpf_program__pin for programs with single instances (we don't create <prog>/0 pin anymore, just <prog>)
* forth patch adds pin_name to the bpf_program struct
which is now used as a pin name in bpf_program__pin et al
* fifth patch adds *loadall* command that pins all programs, not just the first one
* sixth patch adds *pinmaps* argument to load/loadall to let users pin all maps of the obj file
* seventh patch adds actual flow_dissector support to the bpftool and an example

The series look good to me, thanks!

For the bpftool parts:
Acked-by: Quentin Monnet <quentin.monnet at netronome.com>

Jakub Kicinski

2018-11-09 20:15 UTC #14 lore raw

In reply to Stanislav Fomichev

Quoted 6 lines · Stanislav Fomichev

v5 changes:
* FILE -> PATH for load/loadall (can be either file or directory now)
* simpler implementation for __bpf_program__pin_name
* removed p_err for REQ_ARGS checks
* parse_atach_detach_args -> parse_attach_detach_args
* for -> while in bpf_object__pin_{programs,maps} recovery

Thanks! Patch 3 needs attention from maintainers but the rest LGTM!

Alexei Starovoitov

2018-11-11 00:00 UTC #15 lore raw

Quoted 9 lines · Jakub Kicinski

Quoted 6 lines · Stanislav Fomichev
v5 changes:
* FILE -> PATH for load/loadall (can be either file or directory now)
* simpler implementation for __bpf_program__pin_name
* removed p_err for REQ_ARGS checks
* parse_atach_detach_args -> parse_attach_detach_args
* for -> while in bpf_object__pin_{programs,maps} recovery
Thanks! Patch 3 needs attention from maintainers but the rest LGTM!

Whole thing looks good to me.
Applied, Thanks everyone

Martin Lau

2018-11-12 21:53 UTC #3 lore raw

Re: [PATCH v5 bpf-next 2/7] libbpf: cleanup after partial failure in bpf_object__pin

In reply to Stanislav Fomichev

On Fri, Nov 09, 2018@08:21:41AM -0800, Stanislav Fomichev wrote:
[ ... ]

Quoted 30 lines

tools/lib/bpf/libbpf.c +5 -8

@@ -1918,23 +2160,20 @@ void *bpf_object__priv(struct bpf_object *obj)
 }
 
 static struct bpf_program *
-__bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
+__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
 {
-	size_t idx;
+	ssize_t idx;
 
 	if (!obj->programs)
 		return NULL;
-	/* First handler */
-	if (prev == NULL)
-		return &obj->programs[0];
 
-	if (prev->obj != obj) {
+	if (p->obj != obj) {
 		pr_warning("error: program handler doesn't match object\n");
 		return NULL;
 	}
 
-	idx = (prev - obj->programs) + 1;
-	if (idx >= obj->nr_programs)
+	idx = (p - obj->programs) + i;
+	if (idx >= obj->nr_programs || idx < 0)
 		return NULL;
 	return &obj->programs[idx];
 }

tools/lib/bpf/libbpf.c bpf_program__next(struct bpf_program *prev, struct bpf_object *obj) · line 2188

Quoted 6 lines

tools/lib/bpf/libbpf.c +3 -0

@@ -1944,8 +2183,29 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
 	struct bpf_program *prog = prev;
 
+	if (prev == NULL)
+		return obj->programs;
+

This patch breaks the behavior introduced in
commit eac7d84519a3 ("tools: libbpf: don't return '.text' as a program for multi-function programs"):
"Make bpf_program__next() skip over '.text' section if object file
has pseudo calls. The '.text' section is hardly a program in that
case, it's more of a storage for code of functions other than main."

For example, the userspace could have been doing:

	prog = bpf_program__next(NULL, obj);
	bpf_program__set_type(prog, BPF_PROG_TYPE_TRACEPOINT);
	bpf_object__load(obj);

For the bpf_prog.o that has pseudo calls, after this patch in bpf-next, the prog returned by bpf_program__next() could be in ".text" instead of the main bpf program. The next bpf_program__set_type() has no effect to the main program. The following bpf_object__load() will catch user in surprise with the main bpf prog in
the wrong BPF_PROG_TYPE.

Quoted 24 lines

tools/lib/bpf/libbpf.c +19 -1

 	do {
-		prog = __bpf_program__next(prog, obj);
+		prog = __bpf_program__iter(prog, obj, 1);
+	} while (prog && bpf_program__is_function_storage(prog, obj));
+
+	return prog;
+}
+
+struct bpf_program *
+bpf_program__prev(struct bpf_program *next, struct bpf_object *obj)
+{
+	struct bpf_program *prog = next;
+
+	if (next == NULL) {
+		if (!obj->nr_programs)
+			return NULL;
+		return obj->programs + obj->nr_programs - 1;
+	}
+
+	do {
+		prog = __bpf_program__iter(prog, obj, -1);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

Stanislav Fomichev

2018-11-12 22:10 UTC #4 lore raw

Re: [PATCH v5 bpf-next 2/7] libbpf: cleanup after partial failure in bpf_object__pin

Quoted 49 lines · Martin Lau

On Fri, Nov 09, 2018 at 08:21:41AM -0800, Stanislav Fomichev wrote:
[ ... ]
Quoted 30 lines
(quoted diff) +5 -8
@@ -1918,23 +2160,20 @@ void *bpf_object__priv(struct bpf_object *obj)
 }
 
 static struct bpf_program *
-__bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
+__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
 {
-	size_t idx;
+	ssize_t idx;
 
 	if (!obj->programs)
 		return NULL;
-	/* First handler */
-	if (prev == NULL)
-		return &obj->programs[0];
 
-	if (prev->obj != obj) {
+	if (p->obj != obj) {
 		pr_warning("error: program handler doesn't match object\n");
 		return NULL;
 	}
 
-	idx = (prev - obj->programs) + 1;
-	if (idx >= obj->nr_programs)
+	idx = (p - obj->programs) + i;
+	if (idx >= obj->nr_programs || idx < 0)
 		return NULL;
 	return &obj->programs[idx];
 }
Review bpf_program__next(struct bpf_program *prev, struct bpf_object *obj) · line 2188
Quoted 6 lines
(quoted diff) +3 -0
@@ -1944,8 +2183,29 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
 	struct bpf_program *prog = prev;
 
+	if (prev == NULL)
+		return obj->programs;
+
This patch breaks the behavior introduced in
commit eac7d84519a3 ("tools: libbpf: don't return '.text' as a program for multi-function programs"):
"Make bpf_program__next() skip over '.text' section if object file
has pseudo calls. The '.text' section is hardly a program in that
case, it's more of a storage for code of functions other than main."
For example, the userspace could have been doing:
	prog = bpf_program__next(NULL, obj);
	bpf_program__set_type(prog, BPF_PROG_TYPE_TRACEPOINT);
	bpf_object__load(obj);

Quoted 6 lines

For the bpf_prog.o that has pseudo calls, after this patch in bpf-next, the prog returned by bpf_program__next() could be in ".text" instead of the main bpf program. The next bpf_program__set_type() has no effect to the main program. The following bpf_object__load() will catch user in surprise with the main bpf prog in
the wrong BPF_PROG_TYPE.

Will something like the following fix your concern? (plus, assuming the same for prev):

tools/lib/bpf/libbpf.c +5 -2

@@ -2216,8 +2216,11 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
        struct bpf_program *prog = prev;
 
-       if (prev == NULL)
-               return obj->programs;
+       if (prev == NULL) {
+               prog = obj->programs;
+               if (!prog || !bpf_program__is_function_storage(prog, obj))
+                       return prog;
+       }
 
        do {
                prog = __bpf_program__iter(prog, obj, 1);

Any suggestions for a better way to do it?

Quoted 24 lines

(quoted diff) +19 -1

 	do {
-		prog = __bpf_program__next(prog, obj);
+		prog = __bpf_program__iter(prog, obj, 1);
+	} while (prog && bpf_program__is_function_storage(prog, obj));
+
+	return prog;
+}
+
+struct bpf_program *
+bpf_program__prev(struct bpf_program *next, struct bpf_object *obj)
+{
+	struct bpf_program *prog = next;
+
+	if (next == NULL) {
+		if (!obj->nr_programs)
+			return NULL;
+		return obj->programs + obj->nr_programs - 1;
+	}
+
+	do {
+		prog = __bpf_program__iter(prog, obj, -1);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

Martin Lau

2018-11-12 23:10 UTC #5 lore raw

Re: [PATCH v5 bpf-next 2/7] libbpf: cleanup after partial failure in bpf_object__pin

Quoted 77 lines · Stanislav Fomichev

Quoted 49 lines · Martin Lau
On Fri, Nov 09, 2018 at 08:21:41AM -0800, Stanislav Fomichev wrote:
[ ... ]
Quoted 30 lines
(quoted diff) +5 -8
@@ -1918,23 +2160,20 @@ void *bpf_object__priv(struct bpf_object *obj)
 }
 
 static struct bpf_program *
-__bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
+__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
 {
-	size_t idx;
+	ssize_t idx;
 
 	if (!obj->programs)
 		return NULL;
-	/* First handler */
-	if (prev == NULL)
-		return &obj->programs[0];
 
-	if (prev->obj != obj) {
+	if (p->obj != obj) {
 		pr_warning("error: program handler doesn't match object\n");
 		return NULL;
 	}
 
-	idx = (prev - obj->programs) + 1;
-	if (idx >= obj->nr_programs)
+	idx = (p - obj->programs) + i;
+	if (idx >= obj->nr_programs || idx < 0)
 		return NULL;
 	return &obj->programs[idx];
 }
Review bpf_program__next(struct bpf_program *prev, struct bpf_object *obj) · line 2188
Quoted 6 lines
(quoted diff) +3 -0
@@ -1944,8 +2183,29 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
 	struct bpf_program *prog = prev;
 
+	if (prev == NULL)
+		return obj->programs;
+
This patch breaks the behavior introduced in
commit eac7d84519a3 ("tools: libbpf: don't return '.text' as a program for multi-function programs"):
"Make bpf_program__next() skip over '.text' section if object file
has pseudo calls. The '.text' section is hardly a program in that
case, it's more of a storage for code of functions other than main."
For example, the userspace could have been doing:
	prog = bpf_program__next(NULL, obj);
	bpf_program__set_type(prog, BPF_PROG_TYPE_TRACEPOINT);
	bpf_object__load(obj);
Quoted 6 lines
For the bpf_prog.o that has pseudo calls, after this patch in bpf-next, the prog returned by bpf_program__next() could be in ".text" instead of the main bpf program. The next bpf_program__set_type() has no effect to the main program. The following bpf_object__load() will catch user in surprise with the main bpf prog in
the wrong BPF_PROG_TYPE.
Will something like the following fix your concern? (plus, assuming the same for prev):
tools/lib/bpf/libbpf.c +5 -2
@@ -2216,8 +2216,11 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
        struct bpf_program *prog = prev;
 
-       if (prev == NULL)
-               return obj->programs;
+       if (prev == NULL) {
+               prog = obj->programs;
+               if (!prog || !bpf_program__is_function_storage(prog, obj))
+                       return prog;
+       }
 
        do {
                prog = __bpf_program__iter(prog, obj, 1);

Quoted 1 line

Any suggestions for a better way to do it?

I think that would work. The bpf_program__prev() will need the same treatment though...

Here is my mostly untested fix to unblock my other dev works. It moves the very first NULL check back to __bpf_program__iter():

Quoted 1 line

From de1c89ae1768e756825a6874268b5b1686695c93 Mon Sep 17 00:00:00 2001

From: Martin KaFai Lau <kafai at fb.com>
Date: Mon, 12 Nov 2018 14:52:39 -0800
Subject: [PATCH] bpf: libbpf: Fix bpf_program__next() API

This patch restores the behavior in
commit eac7d84519a3 ("tools: libbpf: don't return '.text' as a program for multi-function programs"):
such that bpf_program__next() does not return pseudo programs in ".text".

Fixes 0c19a9fbc9cd ("libbpf: cleanup after partial failure in bpf_object__pin")

Signed-off-by Martin KaFai Lau <kafai at fb.com>

---
 tools/lib/bpf/libbpf.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

tools/lib/bpf/libbpf.c +11 ~~-14~~

index e827542ffa3a..a01eb9584e52 100644
@@ -2193,19 +2193,25 @@ void *bpf_object__priv(struct bpf_object *obj)
 }
 
 static struct bpf_program *
-__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
+__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, bool forward)
 {
+	size_t nr_programs = obj->nr_programs;
 	ssize_t idx;
 
-	if (!obj->programs)
+	if (!nr_programs)
 		return NULL;
 
+	if (!p)
+		/* Iter from the beginning */
+		return forward ? &obj->programs[0] :
+			&obj->programs[nr_programs - 1];
+
 	if (p->obj != obj) {
 		pr_warning("error: program handler doesn't match object\n");
 		return NULL;
 	}
 
-	idx = (p - obj->programs) + i;
+	idx = (p - obj->programs) + (forward ? 1 : -1);
 	if (idx >= obj->nr_programs || idx < 0)
 		return NULL;
 	return &obj->programs[idx];

@@ -2216,11 +2222,8 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
 	struct bpf_program *prog = prev;
 
-	if (prev == NULL)
-		return obj->programs;
-
 	do {
-		prog = __bpf_program__iter(prog, obj, 1);
+		prog = __bpf_program__iter(prog, obj, true);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

@@ -2231,14 +2234,8 @@ bpf_program__prev(struct bpf_program *next, struct bpf_object *obj)
 {
 	struct bpf_program *prog = next;
 
-	if (next == NULL) {
-		if (!obj->nr_programs)
-			return NULL;
-		return obj->programs + obj->nr_programs - 1;
-	}
-
 	do {
-		prog = __bpf_program__iter(prog, obj, -1);
+		prog = __bpf_program__iter(prog, obj, false);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

Signature · 1 line

2.17.1

Quoted 25 lines

(quoted diff) +19 -1

 	do {
-		prog = __bpf_program__next(prog, obj);
+		prog = __bpf_program__iter(prog, obj, 1);
+	} while (prog && bpf_program__is_function_storage(prog, obj));
+
+	return prog;
+}
+
+struct bpf_program *
+bpf_program__prev(struct bpf_program *next, struct bpf_object *obj)
+{
+	struct bpf_program *prog = next;
+
+	if (next == NULL) {
+		if (!obj->nr_programs)
+			return NULL;
+		return obj->programs + obj->nr_programs - 1;
+	}
+
+	do {
+		prog = __bpf_program__iter(prog, obj, -1);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

Stanislav Fomichev

2018-11-12 23:29 UTC #6 lore raw

Re: [PATCH v5 bpf-next 2/7] libbpf: cleanup after partial failure in bpf_object__pin

Quoted 78 lines · Martin Lau

Quoted 77 lines · Stanislav Fomichev
Quoted 49 lines · Martin Lau
On Fri, Nov 09, 2018@08:21:41AM -0800, Stanislav Fomichev wrote:
[ ... ]
Quoted 30 lines
tools/lib/bpf/libbpf.c +5 -8
@@ -1918,23 +2160,20 @@ void *bpf_object__priv(struct bpf_object *obj)
 }
 
 static struct bpf_program *
-__bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
+__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
 {
-	size_t idx;
+	ssize_t idx;
 
 	if (!obj->programs)
 		return NULL;
-	/* First handler */
-	if (prev == NULL)
-		return &obj->programs[0];
 
-	if (prev->obj != obj) {
+	if (p->obj != obj) {
 		pr_warning("error: program handler doesn't match object\n");
 		return NULL;
 	}
 
-	idx = (prev - obj->programs) + 1;
-	if (idx >= obj->nr_programs)
+	idx = (p - obj->programs) + i;
+	if (idx >= obj->nr_programs || idx < 0)
 		return NULL;
 	return &obj->programs[idx];
 }
tools/lib/bpf/libbpf.c bpf_program__next(struct bpf_program *prev, struct bpf_object *obj) · line 2188
Quoted 6 lines
tools/lib/bpf/libbpf.c +3 -0
@@ -1944,8 +2183,29 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
 	struct bpf_program *prog = prev;
 
+	if (prev == NULL)
+		return obj->programs;
+
This patch breaks the behavior introduced in
commit eac7d84519a3 ("tools: libbpf: don't return '.text' as a program for multi-function programs"):
"Make bpf_program__next() skip over '.text' section if object file
has pseudo calls. The '.text' section is hardly a program in that
case, it's more of a storage for code of functions other than main."
For example, the userspace could have been doing:
	prog = bpf_program__next(NULL, obj);
	bpf_program__set_type(prog, BPF_PROG_TYPE_TRACEPOINT);
	bpf_object__load(obj);
Quoted 6 lines
For the bpf_prog.o that has pseudo calls, after this patch in bpf-next, the prog returned by bpf_program__next() could be in ".text" instead of the main bpf program. The next bpf_program__set_type() has no effect to the main program. The following bpf_object__load() will catch user in surprise with the main bpf prog in
the wrong BPF_PROG_TYPE.
Will something like the following fix your concern? (plus, assuming the same for prev):
tools/lib/bpf/libbpf.c +5 -2
@@ -2216,8 +2216,11 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
        struct bpf_program *prog = prev;
 
-       if (prev == NULL)
-               return obj->programs;
+       if (prev == NULL) {
+               prog = obj->programs;
+               if (!prog || !bpf_program__is_function_storage(prog, obj))
+                       return prog;
+       }
 
        do {
                prog = __bpf_program__iter(prog, obj, 1);

Quoted 6 lines

Quoted 1 line
Any suggestions for a better way to do it?
I think that would work. The bpf_program__prev() will need the same treatment though...
Here is my mostly untested fix to unblock my other dev works. It moves the very first NULL check back to __bpf_program__iter():

I like your version and it works with my simple flow dissector test :-) Thanks for spotting and fixing it!

Quoted 105 lines

From de1c89ae1768e756825a6874268b5b1686695c93 Mon Sep 17 00:00:00 2001
From: Martin KaFai Lau <kafai at fb.com>
Date: Mon, 12 Nov 2018 14:52:39 -0800
Subject: [PATCH] bpf: libbpf: Fix bpf_program__next() API

Fixes 0c19a9fbc9cd ("libbpf: cleanup after partial failure in bpf_object__pin")

Signed-off-by Martin KaFai Lau <kafai at fb.com>

---
 tools/lib/bpf/libbpf.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

tools/lib/bpf/libbpf.c +11 ~~-14~~

index e827542ffa3a..a01eb9584e52 100644
@@ -2193,19 +2193,25 @@ void *bpf_object__priv(struct bpf_object *obj)
 }
 
 static struct bpf_program *
-__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
+__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, bool forward)
 {
+	size_t nr_programs = obj->nr_programs;
 	ssize_t idx;
 
-	if (!obj->programs)
+	if (!nr_programs)
 		return NULL;
 
+	if (!p)
+		/* Iter from the beginning */
+		return forward ? &obj->programs[0] :
+			&obj->programs[nr_programs - 1];
+
 	if (p->obj != obj) {
 		pr_warning("error: program handler doesn't match object\n");
 		return NULL;
 	}
 
-	idx = (p - obj->programs) + i;
+	idx = (p - obj->programs) + (forward ? 1 : -1);
 	if (idx >= obj->nr_programs || idx < 0)
 		return NULL;
 	return &obj->programs[idx];

@@ -2216,11 +2222,8 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
 	struct bpf_program *prog = prev;
 
-	if (prev == NULL)
-		return obj->programs;
-
 	do {
-		prog = __bpf_program__iter(prog, obj, 1);
+		prog = __bpf_program__iter(prog, obj, true);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

@@ -2231,14 +2234,8 @@ bpf_program__prev(struct bpf_program *next, struct bpf_object *obj)
 {
 	struct bpf_program *prog = next;
 
-	if (next == NULL) {
-		if (!obj->nr_programs)
-			return NULL;
-		return obj->programs + obj->nr_programs - 1;
-	}
-
 	do {
-		prog = __bpf_program__iter(prog, obj, -1);
+		prog = __bpf_program__iter(prog, obj, false);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

Signature · 1 line

2.17.1

Quoted 25 lines

(quoted diff) +19 -1

 	do {
-		prog = __bpf_program__next(prog, obj);
+		prog = __bpf_program__iter(prog, obj, 1);
+	} while (prog && bpf_program__is_function_storage(prog, obj));
+
+	return prog;
+}
+
+struct bpf_program *
+bpf_program__prev(struct bpf_program *next, struct bpf_object *obj)
+{
+	struct bpf_program *prog = next;
+
+	if (next == NULL) {
+		if (!obj->nr_programs)
+			return NULL;
+		return obj->programs + obj->nr_programs - 1;
+	}
+
+	do {
+		prog = __bpf_program__iter(prog, obj, -1);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

Martin Lau

2018-11-12 23:41 UTC #7 lore raw

Re: [PATCH v5 bpf-next 2/7] libbpf: cleanup after partial failure in bpf_object__pin

Quoted 79 lines · Stanislav Fomichev

Quoted 78 lines · Martin Lau
Quoted 77 lines · Stanislav Fomichev
Quoted 49 lines · Martin Lau
On Fri, Nov 09, 2018 at 08:21:41AM -0800, Stanislav Fomichev wrote:
[ ... ]
Quoted 30 lines
tools/lib/bpf/libbpf.c +5 -8
@@ -1918,23 +2160,20 @@ void *bpf_object__priv(struct bpf_object *obj)
 }
 
 static struct bpf_program *
-__bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
+__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
 {
-	size_t idx;
+	ssize_t idx;
 
 	if (!obj->programs)
 		return NULL;
-	/* First handler */
-	if (prev == NULL)
-		return &obj->programs[0];
 
-	if (prev->obj != obj) {
+	if (p->obj != obj) {
 		pr_warning("error: program handler doesn't match object\n");
 		return NULL;
 	}
 
-	idx = (prev - obj->programs) + 1;
-	if (idx >= obj->nr_programs)
+	idx = (p - obj->programs) + i;
+	if (idx >= obj->nr_programs || idx < 0)
 		return NULL;
 	return &obj->programs[idx];
 }
tools/lib/bpf/libbpf.c bpf_program__next(struct bpf_program *prev, struct bpf_object *obj) · line 2188
Quoted 6 lines
tools/lib/bpf/libbpf.c +3 -0
@@ -1944,8 +2183,29 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
 	struct bpf_program *prog = prev;
 
+	if (prev == NULL)
+		return obj->programs;
+
This patch breaks the behavior introduced in
commit eac7d84519a3 ("tools: libbpf: don't return '.text' as a program for multi-function programs"):
"Make bpf_program__next() skip over '.text' section if object file
has pseudo calls. The '.text' section is hardly a program in that
case, it's more of a storage for code of functions other than main."
For example, the userspace could have been doing:
	prog = bpf_program__next(NULL, obj);
	bpf_program__set_type(prog, BPF_PROG_TYPE_TRACEPOINT);
	bpf_object__load(obj);
Quoted 6 lines
For the bpf_prog.o that has pseudo calls, after this patch in bpf-next, the prog returned by bpf_program__next() could be in ".text" instead of the main bpf program. The next bpf_program__set_type() has no effect to the main program. The following bpf_object__load() will catch user in surprise with the main bpf prog in
the wrong BPF_PROG_TYPE.
Will something like the following fix your concern? (plus, assuming the same for prev):
tools/lib/bpf/libbpf.c +5 -2
@@ -2216,8 +2216,11 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
        struct bpf_program *prog = prev;
 
-       if (prev == NULL)
-               return obj->programs;
+       if (prev == NULL) {
+               prog = obj->programs;
+               if (!prog || !bpf_program__is_function_storage(prog, obj))
+                       return prog;
+       }
 
        do {
                prog = __bpf_program__iter(prog, obj, 1);

Quoted 7 lines

Quoted 6 lines
Quoted 1 line
Any suggestions for a better way to do it?
I think that would work. The bpf_program__prev() will need the same treatment though...
Here is my mostly untested fix to unblock my other dev works. It moves the very first NULL check back to __bpf_program__iter():
I like your version and it works with my simple flow dissector test :-)

Great. I will send out an offical patch.

Quoted 108 lines

Thanks for spotting and fixing it!

Quoted 105 lines

Fixes 0c19a9fbc9cd ("libbpf: cleanup after partial failure in bpf_object__pin")

Signed-off-by Martin KaFai Lau <kafai at fb.com>

---
 tools/lib/bpf/libbpf.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

tools/lib/bpf/libbpf.c +11 ~~-14~~

index e827542ffa3a..a01eb9584e52 100644
@@ -2193,19 +2193,25 @@ void *bpf_object__priv(struct bpf_object *obj)
 }
 
 static struct bpf_program *
-__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, int i)
+__bpf_program__iter(struct bpf_program *p, struct bpf_object *obj, bool forward)
 {
+	size_t nr_programs = obj->nr_programs;
 	ssize_t idx;
 
-	if (!obj->programs)
+	if (!nr_programs)
 		return NULL;
 
+	if (!p)
+		/* Iter from the beginning */
+		return forward ? &obj->programs[0] :
+			&obj->programs[nr_programs - 1];
+
 	if (p->obj != obj) {
 		pr_warning("error: program handler doesn't match object\n");
 		return NULL;
 	}
 
-	idx = (p - obj->programs) + i;
+	idx = (p - obj->programs) + (forward ? 1 : -1);
 	if (idx >= obj->nr_programs || idx < 0)
 		return NULL;
 	return &obj->programs[idx];

@@ -2216,11 +2222,8 @@ bpf_program__next(struct bpf_program *prev, struct bpf_object *obj)
 {
 	struct bpf_program *prog = prev;
 
-	if (prev == NULL)
-		return obj->programs;
-
 	do {
-		prog = __bpf_program__iter(prog, obj, 1);
+		prog = __bpf_program__iter(prog, obj, true);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

@@ -2231,14 +2234,8 @@ bpf_program__prev(struct bpf_program *next, struct bpf_object *obj)
 {
 	struct bpf_program *prog = next;
 
-	if (next == NULL) {
-		if (!obj->nr_programs)
-			return NULL;
-		return obj->programs + obj->nr_programs - 1;
-	}
-
 	do {
-		prog = __bpf_program__iter(prog, obj, -1);
+		prog = __bpf_program__iter(prog, obj, false);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

Signature · 1 line

2.17.1

Quoted 25 lines

(quoted diff) +19 -1

 	do {
-		prog = __bpf_program__next(prog, obj);
+		prog = __bpf_program__iter(prog, obj, 1);
+	} while (prog && bpf_program__is_function_storage(prog, obj));
+
+	return prog;
+}
+
+struct bpf_program *
+bpf_program__prev(struct bpf_program *next, struct bpf_object *obj)
+{
+	struct bpf_program *prog = next;
+
+	if (next == NULL) {
+		if (!obj->nr_programs)
+			return NULL;
+		return obj->programs + obj->nr_programs - 1;
+	}
+
+	do {
+		prog = __bpf_program__iter(prog, obj, -1);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;