Skip to contentSkip to conversation

bpftool: support loading flow dissector

PATCH Superseded Stanislav Fomichev 4 participants 18 messages Nov 8, 2018

Subsystems (7)
Networking Network drivers Kselftest BPF +3 more subsystems

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Stanislav Fomichev

author
#0 lore raw

v3 cover bpftool: support loading flow dissector

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 which is 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 actual support to the bpftool

See forth patch for the description/details.

Stanislav Fomichev (4):
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
bpftool: support loading flow dissector

 .../bpftool/Documentation/bpftool-prog.rst    |  36 ++-
 tools/bpf/bpftool/bash-completion/bpftool     |   6 +-
 tools/bpf/bpftool/common.c                    |  30 +-
 tools/bpf/bpftool/main.h                      |   1 +
 tools/bpf/bpftool/prog.c                      | 112 ++++++--
 tools/lib/bpf/libbpf.c                        | 258 ++++++++++++++++--
 tools/lib/bpf/libbpf.h                        |  11 +
 tools/testing/selftests/bpf/bpf_flow.c        |   2 +-
 .../selftests/bpf/test_flow_dissector.sh      |   2 +-
 9 files changed, 368 insertions(+), 90 deletions(-)
Signature · 1 line
2.19.1.930.g4563a0d9d0-goog
author
#1 lore raw

PATCH v3 1/4 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
author
#2 lore raw

PATCH v3 2/4 libbpf: cleanup after partial failure in bpf_object__pin

In reply to Stanislav Fomichev

bpftool will use bpf_object__pin in the next commit 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).

Signed-off-by Stanislav Fomichev <sdf at google.com>
---
 tools/lib/bpf/libbpf.c | 248 ++++++++++++++++++++++++++++++++++++-----
 tools/lib/bpf/libbpf.h |  11 ++
 2 files changed, 230 insertions(+), 29 deletions(-)
tools/lib/bpf/libbpf.c +219 -29
index d6e62e90e8d4..db84c85554e7 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,6 +1866,28 @@ int bpf_map__pin(struct bpf_map *map, const char *path)
 	}
 
 	pr_debug("pinned map '%s'\n", path);
+
+	return 0;
+}
+
+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;
 }
 

@@ -1803,14 +1915,17 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 
 		len = snprintf(buf, PATH_MAX, "%s/%s", path,
 			       bpf_map__name(map));
-		if (len < 0)
-			return -EINVAL;
-		else if (len >= PATH_MAX)
-			return -ENAMETOOLONG;
+		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)
-			return err;
+			goto err_unpin_maps;
 	}
 
 	bpf_object__for_each_program(prog, obj) {

@@ -1819,17 +1934,56 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 
 		len = snprintf(buf, PATH_MAX, "%s/%s", path,
 			       prog->section_name);
-		if (len < 0)
-			return -EINVAL;
-		else if (len >= PATH_MAX)
-			return -ENAMETOOLONG;
+		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)
-			return err;
+			goto err_unpin_programs;
 	}
 
 	return 0;
+
+err_unpin_programs:
+	for (prog = bpf_program__prev(prog, obj);
+	     prog != NULL;
+	     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);
+	}
+
+err_unpin_maps:
+	for (map = bpf_map__prev(map, obj);
+	     map != NULL;
+	     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;
 }
 
 void bpf_object__close(struct bpf_object *obj)

@@ -1918,23 +2072,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 +2095,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__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__next(prog, obj);
+		prog = __bpf_program__iter(prog, obj, -1);
 	} while (prog && bpf_program__is_function_storage(prog, obj));
 
 	return prog;

@@ -2272,10 +2444,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 +2456,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 +11 -0
index 1f3468dad8b2..785b27f761de 100644
@@ -112,6 +112,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 +134,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 +267,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 +284,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
author
#3 lore raw

PATCH v3 3/4 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 db84c85554e7..8407a880acbe 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
author
#4 lore raw

PATCH v3 4/4 bpftool: support loading flow dissector

In reply to Stanislav Fomichev

This commit adds support for loading/attaching/detaching flow dissector program. The structure of the flow dissector program is assumed to be the same as in the selftests:

* flow_dissector section with the main entry point
* a bunch of tail call progs
* a jmp_table map that is populated with the tail call progs

When bpftool load is called with a flow_dissector prog (i.e. when the first section is flow_dissector of 'type flow_dissector' argument is passed), we load and pin all the programs/maps. 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

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>
---
 .../bpftool/Documentation/bpftool-prog.rst    |  36 ++++--
 tools/bpf/bpftool/bash-completion/bpftool     |   6 +-
 tools/bpf/bpftool/common.c                    |  30 ++---
 tools/bpf/bpftool/main.h                      |   1 +
 tools/bpf/bpftool/prog.c                      | 112 +++++++++++++-----
 5 files changed, 126 insertions(+), 59 deletions(-)
tools/bpf/bpftool/Documentation/bpftool-prog.rst +23 -13
index ac4e904b10fb..0374634c3087 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,9 +25,9 @@ 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 attach** *PROG* *ATTACH_TYPE* *MAP*
-|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
+|	**bpftool** **prog { load | loadall }** *OBJ* *FILE* [**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**
 |
 |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }

@@ -39,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

@@ -79,8 +82,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*]
+	**bpftool prog { load | loadall }** *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** will pin only the first bpf program
+		  from the *OBJ*, **bpftool prog loadall** will pin all maps
+		  and programs from the *OBJ*.
 		  **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

@@ -97,13 +103,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 +3 -3
index 3f78e6404589..ad0fc919f7ec 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

@@ -299,7 +299,7 @@ _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
 

@@ -309,7 +309,7 @@ _bpftool()
                     fi
                     return 0
                     ;;
-                load)
+                load|loadall)
                     local obj
 
                     if [[ ${#words[@]} -lt 6 ]]; then
tools/bpf/bpftool/common.c +15 -15
index 25af85304ebe..f671a921dec5 100644
@@ -169,34 +169,24 @@ 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 +194,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 +84 -28

207 lines not on this page · raw message

Signature · 1 line
2.19.1.930.g4563a0d9d0-goog
#5 lore raw

Hi Stanislav, thanks for the changes! More comments below.

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

Quoted 97 lines

This commit adds support for loading/attaching/detaching flow dissector program. The structure of the flow dissector program is assumed to be the same as in the selftests:

* flow_dissector section with the main entry point
* a bunch of tail call progs
* a jmp_table map that is populated with the tail call progs

When bpftool load is called with a flow_dissector prog (i.e. when the first section is flow_dissector of 'type flow_dissector' argument is passed), we load and pin all the programs/maps. 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

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>
---
  .../bpftool/Documentation/bpftool-prog.rst    |  36 ++++--
  tools/bpf/bpftool/bash-completion/bpftool     |   6 +-
  tools/bpf/bpftool/common.c                    |  30 ++---
  tools/bpf/bpftool/main.h                      |   1 +
  tools/bpf/bpftool/prog.c                      | 112 +++++++++++++-----
  5 files changed, 126 insertions(+), 59 deletions(-)
tools/bpf/bpftool/Documentation/bpftool-prog.rst +8 -5
index ac4e904b10fb..0374634c3087 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,9 +25,9 @@ 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 attach** *PROG* *ATTACH_TYPE* *MAP*
-|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
+|	**bpftool** **prog { load | loadall }** *OBJ* *FILE* [**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**
  |
  |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }

@@ -39,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

tools/bpf/bpftool/Documentation/bpftool-prog.rst DESCRIPTION · line 89

Quoted 9 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +4 -1
@@ -79,8 +82,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*]
+	**bpftool prog { load | loadall }** *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** will pin only the first bpf program
+		  from the *OBJ*, **bpftool prog loadall** will pin all maps
+		  and programs from the *OBJ*.

This could be improved regarding maps: with "bpftool prog load" I think we also load and pin all maps, but your description implies this is only the case with "loadall"

Quoted 15 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +1 -7
  		  **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

@@ -97,13 +103,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*]

tools/bpf/bpftool/Documentation/bpftool-prog.rst DESCRIPTION · line 116

Quoted 10 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +10 -0
+                  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.

While at it could you please fix those two paragraphs to use tabs for indentation, as the rest of the doc? Thanks!

Quoted 26 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +0 -0
  	**bpftool prog help**
  		  Print short help message.
tools/bpf/bpftool/bash-completion/bpftool +2 -2
index 3f78e6404589..ad0fc919f7ec 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

@@ -299,7 +299,7 @@ _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

tools/bpf/bpftool/bash-completion/bpftool _bpftool() · line 315

Quoted 8 lines
tools/bpf/bpftool/bash-completion/bpftool +1 -1
@@ -309,7 +309,7 @@ _bpftool()
                      fi
                      return 0
                      ;;
-                load)
+                load|loadall)
                      local obj
 
                      if [[ ${#words[@]} -lt 6 ]]; then

You also want to update completion for the program types, at line 341 or so. Feel free to split that list on several lines, by the way :).

Quoted 20 lines
tools/bpf/bpftool/common.c +1 -5
index 25af85304ebe..f671a921dec5 100644
@@ -169,34 +169,24 @@ 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;
-

tools/bpf/bpftool/common.c int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type) · line 186

Quoted 10 lines
tools/bpf/bpftool/common.c +2 -2
  	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;
  	}

Nitpick: unnecessary brackets.

Quoted 112 lines
tools/bpf/bpftool/common.c +12 -8
-	/* 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 +194,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 +19 -10
index 5302ee282409..a4346dd673b1 100644
@@ -81,6 +81,7 @@ static const char * const attach_type_strings[] = {
  	[BPF_SK_SKB_STREAM_PARSER] = "stream_parser",
  	[BPF_SK_SKB_STREAM_VERDICT] = "stream_verdict",
  	[BPF_SK_MSG_VERDICT] = "msg_verdict",
+	[BPF_FLOW_DISSECTOR] = "flow_dissector",
  	[__MAX_BPF_ATTACH_TYPE] = NULL,
  };
 

@@ -724,10 +725,11 @@ int map_replace_compar(const void *p1, const void *p2)
  static int do_attach(int argc, char **argv)
  {
  	enum bpf_attach_type attach_type;
-	int err, mapfd, progfd;
+	int err, progfd;
+	int mapfd = 0;
 
-	if (!REQ_ARGS(5)) {
-		p_err("too few parameters for map attach");
+	if (!REQ_ARGS(3)) {
+		p_err("too few parameters for attach");
  		return -EINVAL;
  	}
 

@@ -740,11 +742,17 @@ static int do_attach(int argc, char **argv)
  		p_err("invalid attach type");
  		return -EINVAL;
  	}
-	NEXT_ARG();
+	if (attach_type != BPF_FLOW_DISSECTOR) {
+		NEXT_ARG();
+		if (!REQ_ARGS(2)) {
+			p_err("too few parameters for map attach");
+			return -EINVAL;
+		}
 
-	mapfd = map_parse_fd(&argc, &argv);
-	if (mapfd < 0)
-		return mapfd;
+		mapfd = map_parse_fd(&argc, &argv);
+		if (mapfd < 0)
+			return mapfd;
+	}
 
  	err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
  	if (err) {

@@ -760,10 +768,11 @@ static int do_attach(int argc, char **argv)
  static int do_detach(int argc, char **argv)
  {
  	enum bpf_attach_type attach_type;
-	int err, mapfd, progfd;
+	int err, progfd;
+	int mapfd = 0;
 
-	if (!REQ_ARGS(5)) {
-		p_err("too few parameters for map detach");
+	if (!REQ_ARGS(3)) {
+		p_err("too few parameters for detach");
  		return -EINVAL;
  	}

tools/bpf/bpftool/prog.c static int do_detach(int argc, char **argv) · line 793

Quoted 10 lines
tools/bpf/bpftool/prog.c +6 -1
@@ -776,11 +785,17 @@ static int do_detach(int argc, char **argv)
  		p_err("invalid attach type");
  		return -EINVAL;
  	}
-	NEXT_ARG();
+	if (attach_type != BPF_FLOW_DISSECTOR) {
+		NEXT_ARG();
+		if (!REQ_ARGS(2)) {
+			p_err("too few parameters for map detach");
+			return -EINVAL;
+		}

Would that make sense to factor argument checks or parsing for do_attach() and do_detach() to some extent? In order to reduce the number of attach-type-based exceptions to add in the code if we have other attach types that do not take maps in the future.

Quoted 59 lines
tools/bpf/bpftool/prog.c +18 -10
-	mapfd = map_parse_fd(&argc, &argv);
-	if (mapfd < 0)
-		return mapfd;
+		mapfd = map_parse_fd(&argc, &argv);
+		if (mapfd < 0)
+			return mapfd;
+	}
 
  	err = bpf_prog_detach2(progfd, mapfd, attach_type);
  	if (err) {

@@ -792,15 +807,16 @@ static int do_detach(int argc, char **argv)
  		jsonw_null(json_wtr);
  	return 0;
  }
-static int do_load(int argc, char **argv)
+
+static int load_with_options(int argc, char **argv, bool first_prog_only)
  {
  	enum bpf_attach_type expected_attach_type;
  	struct bpf_object_open_attr attr = {
  		.prog_type	= BPF_PROG_TYPE_UNSPEC,
  	};
  	struct map_replace *map_replace = NULL;
+	struct bpf_program *prog = NULL, *pos;
  	unsigned int old_map_fds = 0;
-	struct bpf_program *prog;
  	struct bpf_object *obj;
  	struct bpf_map *map;
  	const char *pinfile;

@@ -918,14 +934,20 @@ static int do_load(int argc, char **argv)
  		goto err_free_reuse_maps;
  	}
 
-	prog = bpf_program__next(NULL, obj);
-	if (!prog) {
-		p_err("object file doesn't contain any bpf program");
-		goto err_close_obj;
+	if (first_prog_only) {
+		prog = bpf_program__next(NULL, obj);
+		if (!prog) {
+			p_err("object file doesn't contain any bpf program");
+			goto err_close_obj;
+		}
  	}
 
-	bpf_program__set_ifindex(prog, ifindex);
  	if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
+		if (!prog) {
+			p_err("can not guess program type when loading all programs\n");
+			goto err_close_obj;
+		}
+
  		const char *sec_name = bpf_program__title(prog, false);
 
  		err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,

@@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
  			goto err_close_obj;
  		}

tools/bpf/bpftool/prog.c static int do_load(int argc, char **argv) · line 967

Quoted 10 lines
tools/bpf/bpftool/prog.c +7 -2
  	}
-	bpf_program__set_type(prog, attr.prog_type);
-	bpf_program__set_expected_attach_type(prog, expected_attach_type);
+
+	bpf_object__for_each_program(pos, obj) {
+		bpf_program__set_ifindex(pos, ifindex);
+		bpf_program__set_type(pos, attr.prog_type);
+		bpf_program__set_expected_attach_type(pos,
+						      expected_attach_type);
+	}

I still believe you can have programs of different types here, and be able to load them. I tried it and managed to have it working fine. If no type is provided from command line we can retrieve types for each program from its section name. If a type is provided on the command line, we can do the same, but I am not sure we should do it, or impose that type for all programs instead.

tools/bpf/bpftool/prog.c static int do_load(int argc, char **argv) · line 1035

Quoted 13 lines
tools/bpf/bpftool/prog.c +3 -1
  	qsort(map_replace, old_map_fds, sizeof(*map_replace),
  	      map_replace_compar);

@@ -1001,9 +1028,25 @@ static int do_load(int argc, char **argv)
  		goto err_close_obj;
  	}
 
-	if (do_pin_fd(bpf_program__fd(prog), pinfile))
+	err = mount_bpffs_for_pin(pinfile);
+	if (err)
  		goto err_close_obj;
 
+	if (prog) {

Nit: Maybe "if (first_prog_only) {" instead? If I understand correctly, at this stage it should be equivalent, but in my opinion it would make it easier to understand why we have two cases here.

Quoted 68 lines
tools/bpf/bpftool/prog.c +31 -4
+		err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
+		if (err) {
+			p_err("failed to pin program %s",
+			      bpf_program__title(prog, false));
+			goto err_close_obj;
+		}
+	} else {
+		err = bpf_object__pin(obj, pinfile);
+		if (err) {
+			p_err("failed to pin all programs");
+			goto err_close_obj;
+		}
+	}
+
  	if (json_output)
  		jsonw_null(json_wtr);
 

@@ -1023,6 +1066,16 @@ static int do_load(int argc, char **argv)
  	return -1;
  }
 
+static int do_load(int argc, char **argv)
+{
+	return load_with_options(argc, argv, true);
+}
+
+static int do_loadall(int argc, char **argv)
+{
+	return load_with_options(argc, argv, false);
+}
+
  static int do_help(int argc, char **argv)
  {
  	if (json_output) {

@@ -1035,10 +1088,11 @@ static int do_help(int argc, char **argv)
  		"       %s %s dump xlated PROG [{ file FILE | opcodes | visual }]\n"
  		"       %s %s dump jited  PROG [{ file FILE | opcodes }]\n"
  		"       %s %s pin   PROG FILE\n"
-		"       %s %s load  OBJ  FILE [type TYPE] [dev NAME] \\\n"
+		"       %s %s { load | loadall } OBJ  FILE \\\n"
+		"                         [type TYPE] [dev NAME] \\\n"
  		"                         [map { idx IDX | name NAME } MAP]\n"
-		"       %s %s attach PROG ATTACH_TYPE MAP\n"
-		"       %s %s detach PROG ATTACH_TYPE MAP\n"
+		"       %s %s attach PROG ATTACH_TYPE [MAP]\n"
+		"       %s %s detach PROG ATTACH_TYPE [MAP]\n"
  		"       %s %s help\n"
  		"\n"
  		"       " HELP_SPEC_MAP "\n"

@@ -1050,7 +1104,8 @@ static int do_help(int argc, char **argv)
  		"                 cgroup/bind4 | cgroup/bind6 | cgroup/post_bind4 |\n"
  		"                 cgroup/post_bind6 | cgroup/connect4 | cgroup/connect6 |\n"
  		"                 cgroup/sendmsg4 | cgroup/sendmsg6 }\n"
-		"       ATTACH_TYPE := { msg_verdict | skb_verdict | skb_parse }\n"
+		"       ATTACH_TYPE := { msg_verdict | skb_verdict | skb_parse |\n"
+		"                        flow_dissector }\n"
  		"       " HELP_SPEC_OPTIONS "\n"
  		"",
  		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],

@@ -1067,6 +1122,7 @@ static const struct cmd cmds[] = {
  	{ "dump",	do_dump },
  	{ "pin",	do_pin },
  	{ "load",	do_load },
+	{ "loadall",	do_loadall },
  	{ "attach",	do_attach },
  	{ "detach",	do_detach },
  	{ 0 }
#6 lore raw
Quoted 1 line · Quentin Monnet

Hi Stanislav, thanks for the changes! More comments below.

Thank you for another round of review!

Quoted 98 lines

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

Quoted 97 lines

This commit adds support for loading/attaching/detaching flow dissector program. The structure of the flow dissector program is assumed to be the same as in the selftests:

* flow_dissector section with the main entry point
* a bunch of tail call progs
* a jmp_table map that is populated with the tail call progs

When bpftool load is called with a flow_dissector prog (i.e. when the first section is flow_dissector of 'type flow_dissector' argument is passed), we load and pin all the programs/maps. 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

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>
---
  .../bpftool/Documentation/bpftool-prog.rst    |  36 ++++--
  tools/bpf/bpftool/bash-completion/bpftool     |   6 +-
  tools/bpf/bpftool/common.c                    |  30 ++---
  tools/bpf/bpftool/main.h                      |   1 +
  tools/bpf/bpftool/prog.c                      | 112 +++++++++++++-----
  5 files changed, 126 insertions(+), 59 deletions(-)
tools/bpf/bpftool/Documentation/bpftool-prog.rst +9 -6
index ac4e904b10fb..0374634c3087 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,9 +25,9 @@ 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 attach** *PROG* *ATTACH_TYPE* *MAP*
-|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
+|	**bpftool** **prog { load | loadall }** *OBJ* *FILE* [**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**
  |
  |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }

@@ -39,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

@@ -79,8 +82,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*]
+	**bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
Quoted 8 lines

tools/bpf/bpftool/Documentation/bpftool-prog.rst DESCRIPTION · line 89

Quoted 4 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +3 -0
  		  Load bpf program from binary *OBJ* and pin as *FILE*.
+		  **bpftool prog load** will pin only the first bpf program
+		  from the *OBJ*, **bpftool prog loadall** will pin all maps
+		  and programs from the *OBJ*.

This could be improved regarding maps: with "bpftool prog load" I think we also load and pin all maps, but your description implies this is only the case with "loadall"

I don't think we pin any maps with bpftool prog load, we certainly load them, but we don't pin any afaict. Can you point me to the code where we pin the maps?

Quoted 19 lines
Quoted 19 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +6 -7
  		  **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

@@ -97,13 +103,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.
+
Quoted 8 lines

tools/bpf/bpftool/Documentation/bpftool-prog.rst DESCRIPTION · line 116

Quoted 5 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +5 -0
+        **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.

While at it could you please fix those two paragraphs to use tabs for indentation, as the rest of the doc? Thanks!

Time to teach my vim to use tabs in .rst files. Sorry about that.

Quoted 25 lines
Quoted 25 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +0 -0
  	**bpftool prog help**
  		  Print short help message.
tools/bpf/bpftool/bash-completion/bpftool +2 -2
index 3f78e6404589..ad0fc919f7ec 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

@@ -299,7 +299,7 @@ _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

@@ -309,7 +309,7 @@ _bpftool()
                      fi
                      return 0
Quoted 8 lines

tools/bpf/bpftool/bash-completion/bpftool _bpftool() · line 315

Quoted 5 lines
tools/bpf/bpftool/bash-completion/bpftool +1 -1
                      ;;
-                load)
+                load|loadall)
                      local obj
                      if [[ ${#words[@]} -lt 6 ]]; then

You also want to update completion for the program types, at line 341 or so. Feel free to split that list on several lines, by the way :).

Will do, thanks!

Quoted 21 lines
Quoted 21 lines
tools/bpf/bpftool/common.c +1 -5
index 25af85304ebe..f671a921dec5 100644
@@ -169,34 +169,24 @@ 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);
Quoted 8 lines

tools/bpf/bpftool/common.c int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type) · line 186

Quoted 6 lines
tools/bpf/bpftool/common.c +2 -2
-	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;
  	}

Nitpick: unnecessary brackets.

Ack.

Quoted 108 lines
Quoted 108 lines
tools/bpf/bpftool/common.c +12 -8
-	/* 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 +194,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 +22 -11
index 5302ee282409..a4346dd673b1 100644
@@ -81,6 +81,7 @@ static const char * const attach_type_strings[] = {
  	[BPF_SK_SKB_STREAM_PARSER] = "stream_parser",
  	[BPF_SK_SKB_STREAM_VERDICT] = "stream_verdict",
  	[BPF_SK_MSG_VERDICT] = "msg_verdict",
+	[BPF_FLOW_DISSECTOR] = "flow_dissector",
  	[__MAX_BPF_ATTACH_TYPE] = NULL,
  };

@@ -724,10 +725,11 @@ int map_replace_compar(const void *p1, const void *p2)
  static int do_attach(int argc, char **argv)
  {
  	enum bpf_attach_type attach_type;
-	int err, mapfd, progfd;
+	int err, progfd;
+	int mapfd = 0;
-	if (!REQ_ARGS(5)) {
-		p_err("too few parameters for map attach");
+	if (!REQ_ARGS(3)) {
+		p_err("too few parameters for attach");
  		return -EINVAL;
  	}

@@ -740,11 +742,17 @@ static int do_attach(int argc, char **argv)
  		p_err("invalid attach type");
  		return -EINVAL;
  	}
-	NEXT_ARG();
+	if (attach_type != BPF_FLOW_DISSECTOR) {
+		NEXT_ARG();
+		if (!REQ_ARGS(2)) {
+			p_err("too few parameters for map attach");
+			return -EINVAL;
+		}
-	mapfd = map_parse_fd(&argc, &argv);
-	if (mapfd < 0)
-		return mapfd;
+		mapfd = map_parse_fd(&argc, &argv);
+		if (mapfd < 0)
+			return mapfd;
+	}
  	err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
  	if (err) {

@@ -760,10 +768,11 @@ static int do_attach(int argc, char **argv)
  static int do_detach(int argc, char **argv)
  {
  	enum bpf_attach_type attach_type;
-	int err, mapfd, progfd;
+	int err, progfd;
+	int mapfd = 0;
-	if (!REQ_ARGS(5)) {
-		p_err("too few parameters for map detach");
+	if (!REQ_ARGS(3)) {
+		p_err("too few parameters for detach");
  		return -EINVAL;
  	}

@@ -776,11 +785,17 @@ static int do_detach(int argc, char **argv)
  		p_err("invalid attach type");
  		return -EINVAL;
  	}
-	NEXT_ARG();
+	if (attach_type != BPF_FLOW_DISSECTOR) {
+		NEXT_ARG();
+		if (!REQ_ARGS(2)) {
Quoted 8 lines

tools/bpf/bpftool/prog.c static int do_detach(int argc, char **argv) · line 793

Quoted 3 lines
tools/bpf/bpftool/prog.c +3 -0
+			p_err("too few parameters for map detach");
+			return -EINVAL;
+		}

Would that make sense to factor argument checks or parsing for do_attach() and do_detach() to some extent? In order to reduce the number of attach-type-based exceptions to add in the code if we have other attach types that do not take maps in the future.

I can move all argument parsing into a new function and use it from both do_attach and do_detach.

Quoted 63 lines
Quoted 63 lines
tools/bpf/bpftool/prog.c +24 -12
-	mapfd = map_parse_fd(&argc, &argv);
-	if (mapfd < 0)
-		return mapfd;
+		mapfd = map_parse_fd(&argc, &argv);
+		if (mapfd < 0)
+			return mapfd;
+	}
  	err = bpf_prog_detach2(progfd, mapfd, attach_type);
  	if (err) {

@@ -792,15 +807,16 @@ static int do_detach(int argc, char **argv)
  		jsonw_null(json_wtr);
  	return 0;
  }
-static int do_load(int argc, char **argv)
+
+static int load_with_options(int argc, char **argv, bool first_prog_only)
  {
  	enum bpf_attach_type expected_attach_type;
  	struct bpf_object_open_attr attr = {
  		.prog_type	= BPF_PROG_TYPE_UNSPEC,
  	};
  	struct map_replace *map_replace = NULL;
+	struct bpf_program *prog = NULL, *pos;
  	unsigned int old_map_fds = 0;
-	struct bpf_program *prog;
  	struct bpf_object *obj;
  	struct bpf_map *map;
  	const char *pinfile;

@@ -918,14 +934,20 @@ static int do_load(int argc, char **argv)
  		goto err_free_reuse_maps;
  	}
-	prog = bpf_program__next(NULL, obj);
-	if (!prog) {
-		p_err("object file doesn't contain any bpf program");
-		goto err_close_obj;
+	if (first_prog_only) {
+		prog = bpf_program__next(NULL, obj);
+		if (!prog) {
+			p_err("object file doesn't contain any bpf program");
+			goto err_close_obj;
+		}
  	}
-	bpf_program__set_ifindex(prog, ifindex);
  	if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
+		if (!prog) {
+			p_err("can not guess program type when loading all programs\n");
+			goto err_close_obj;
+		}
+
  		const char *sec_name = bpf_program__title(prog, false);
  		err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,

@@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
  			goto err_close_obj;
  		}
  	}
-	bpf_program__set_type(prog, attr.prog_type);
-	bpf_program__set_expected_attach_type(prog, expected_attach_type);
+
+	bpf_object__for_each_program(pos, obj) {
+		bpf_program__set_ifindex(pos, ifindex);
+		bpf_program__set_type(pos, attr.prog_type);
+		bpf_program__set_expected_attach_type(pos,
+						      expected_attach_type);
Quoted 8 lines

tools/bpf/bpftool/prog.c static int do_load(int argc, char **argv) · line 967

Quoted 1 line
tools/bpf/bpftool/prog.c +1 -0
+	}

I still believe you can have programs of different types here, and be able to load them. I tried it and managed to have it working fine. If no type is provided from command line we can retrieve types for each program from its section name. If a type is provided on the command line, we can do the same, but I am not sure we should do it, or impose that type for all programs instead.

I can move auto-detection into this new bpf_object__for_each_program loop. So if no type is specified, try to infer the type from each prog section name, otherwise, use the provided one for all progs. Do we want something like that?
Btw, do you have some existing real life example of where it's needed so I can test this new implementation? (maybe something under samples/ ?)

Quoted 6 lines
Quoted 6 lines
tools/bpf/bpftool/prog.c +0 -1
  	qsort(map_replace, old_map_fds, sizeof(*map_replace),
  	      map_replace_compar);

@@ -1001,9 +1028,25 @@ static int do_load(int argc, char **argv)
  		goto err_close_obj;
  	}
-	if (do_pin_fd(bpf_program__fd(prog), pinfile))
Quoted 8 lines

tools/bpf/bpftool/prog.c static int do_load(int argc, char **argv) · line 1035

Quoted 4 lines
tools/bpf/bpftool/prog.c +3 -0
+	err = mount_bpffs_for_pin(pinfile);
+	if (err)
  		goto err_close_obj;
+	if (prog) {

Nit: Maybe "if (first_prog_only) {" instead? If I understand correctly, at this stage it should be equivalent, but in my opinion it would make it easier to understand why we have two cases here.

Sure, I can do that if you think that's more readable, I don't have a preference.

Quoted 67 lines
Quoted 66 lines
tools/bpf/bpftool/prog.c +31 -4
+		err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
+		if (err) {
+			p_err("failed to pin program %s",
+			      bpf_program__title(prog, false));
+			goto err_close_obj;
+		}
+	} else {
+		err = bpf_object__pin(obj, pinfile);
+		if (err) {
+			p_err("failed to pin all programs");
+			goto err_close_obj;
+		}
+	}
+
  	if (json_output)
  		jsonw_null(json_wtr);

@@ -1023,6 +1066,16 @@ static int do_load(int argc, char **argv)
  	return -1;
  }
+static int do_load(int argc, char **argv)
+{
+	return load_with_options(argc, argv, true);
+}
+
+static int do_loadall(int argc, char **argv)
+{
+	return load_with_options(argc, argv, false);
+}
+
  static int do_help(int argc, char **argv)
  {
  	if (json_output) {

@@ -1035,10 +1088,11 @@ static int do_help(int argc, char **argv)
  		"       %s %s dump xlated PROG [{ file FILE | opcodes | visual }]\n"
  		"       %s %s dump jited  PROG [{ file FILE | opcodes }]\n"
  		"       %s %s pin   PROG FILE\n"
-		"       %s %s load  OBJ  FILE [type TYPE] [dev NAME] \\\n"
+		"       %s %s { load | loadall } OBJ  FILE \\\n"
+		"                         [type TYPE] [dev NAME] \\\n"
  		"                         [map { idx IDX | name NAME } MAP]\n"
-		"       %s %s attach PROG ATTACH_TYPE MAP\n"
-		"       %s %s detach PROG ATTACH_TYPE MAP\n"
+		"       %s %s attach PROG ATTACH_TYPE [MAP]\n"
+		"       %s %s detach PROG ATTACH_TYPE [MAP]\n"
  		"       %s %s help\n"
  		"\n"
  		"       " HELP_SPEC_MAP "\n"

@@ -1050,7 +1104,8 @@ static int do_help(int argc, char **argv)
  		"                 cgroup/bind4 | cgroup/bind6 | cgroup/post_bind4 |\n"
  		"                 cgroup/post_bind6 | cgroup/connect4 | cgroup/connect6 |\n"
  		"                 cgroup/sendmsg4 | cgroup/sendmsg6 }\n"
-		"       ATTACH_TYPE := { msg_verdict | skb_verdict | skb_parse }\n"
+		"       ATTACH_TYPE := { msg_verdict | skb_verdict | skb_parse |\n"
+		"                        flow_dissector }\n"
  		"       " HELP_SPEC_OPTIONS "\n"
  		"",
  		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],

@@ -1067,6 +1122,7 @@ static const struct cmd cmds[] = {
  	{ "dump",	do_dump },
  	{ "pin",	do_pin },
  	{ "load",	do_load },
+	{ "loadall",	do_loadall },
  	{ "attach",	do_attach },
  	{ "detach",	do_detach },
  	{ 0 }
#7 lore raw

2018-11-08 10:01 UTC-0800 ~ Stanislav Fomichev <sdf at fomichev.me>

Quoted 110 lines · Quentin Monnet
Quoted 1 line

Hi Stanislav, thanks for the changes! More comments below.

Thank you for another round of review!

Quoted 98 lines

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

Quoted 97 lines

This commit adds support for loading/attaching/detaching flow dissector program. The structure of the flow dissector program is assumed to be the same as in the selftests:

* flow_dissector section with the main entry point
* a bunch of tail call progs
* a jmp_table map that is populated with the tail call progs

When bpftool load is called with a flow_dissector prog (i.e. when the first section is flow_dissector of 'type flow_dissector' argument is passed), we load and pin all the programs/maps. 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

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>
---
  .../bpftool/Documentation/bpftool-prog.rst    |  36 ++++--
  tools/bpf/bpftool/bash-completion/bpftool     |   6 +-
  tools/bpf/bpftool/common.c                    |  30 ++---
  tools/bpf/bpftool/main.h                      |   1 +
  tools/bpf/bpftool/prog.c                      | 112 +++++++++++++-----
  5 files changed, 126 insertions(+), 59 deletions(-)
tools/bpf/bpftool/Documentation/bpftool-prog.rst +9 -6
index ac4e904b10fb..0374634c3087 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,9 +25,9 @@ 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 attach** *PROG* *ATTACH_TYPE* *MAP*
-|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
+|	**bpftool** **prog { load | loadall }** *OBJ* *FILE* [**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**
  |
  |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }

@@ -39,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

@@ -79,8 +82,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*]
+	**bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
Quoted 8 lines

tools/bpf/bpftool/Documentation/bpftool-prog.rst DESCRIPTION · line 89

Quoted 4 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +3 -0
  		  Load bpf program from binary *OBJ* and pin as *FILE*.
+		  **bpftool prog load** will pin only the first bpf program
+		  from the *OBJ*, **bpftool prog loadall** will pin all maps
+		  and programs from the *OBJ*.

This could be improved regarding maps: with "bpftool prog load" I think we also load and pin all maps, but your description implies this is only the case with "loadall"

Quoted 4 lines

I don't think we pin any maps with bpftool prog load, we certainly load them, but we don't pin any afaict. Can you point me to the code where we pin the maps?

My bad. I read "pin" but thought "load". It does not pin them indeed, sorry about that.

Quoted 27 lines
Quoted 19 lines
Quoted 19 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +6 -7
  		  **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

@@ -97,13 +103,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.
+
Quoted 8 lines

tools/bpf/bpftool/Documentation/bpftool-prog.rst DESCRIPTION · line 116

Quoted 5 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +5 -0
+        **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.

While at it could you please fix those two paragraphs to use tabs for indentation, as the rest of the doc? Thanks!

Quoted 1 line

Time to teach my vim to use tabs in .rst files. Sorry about that.

Those paragraphs were using spaces already, so you didn't introduce that :). But all others use tabs so its a good occasion to fix it.

Quoted 182 lines
Quoted 25 lines
Quoted 25 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +0 -0
  	**bpftool prog help**
  		  Print short help message.
tools/bpf/bpftool/bash-completion/bpftool +2 -2
index 3f78e6404589..ad0fc919f7ec 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

@@ -299,7 +299,7 @@ _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

@@ -309,7 +309,7 @@ _bpftool()
                      fi
                      return 0
Quoted 8 lines

tools/bpf/bpftool/bash-completion/bpftool _bpftool() · line 315

Quoted 5 lines
tools/bpf/bpftool/bash-completion/bpftool +1 -1
                      ;;
-                load)
+                load|loadall)
                      local obj
                      if [[ ${#words[@]} -lt 6 ]]; then

You also want to update completion for the program types, at line 341 or so. Feel free to split that list on several lines, by the way :).

Will do, thanks!

Quoted 21 lines
Quoted 21 lines
tools/bpf/bpftool/common.c +1 -5
index 25af85304ebe..f671a921dec5 100644
@@ -169,34 +169,24 @@ 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);
Quoted 8 lines

tools/bpf/bpftool/common.c int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type) · line 186

Quoted 6 lines
tools/bpf/bpftool/common.c +2 -2
-	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;
  	}

Nitpick: unnecessary brackets.

Ack.

Quoted 108 lines
Quoted 108 lines
tools/bpf/bpftool/common.c +12 -8
-	/* 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 +194,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 +22 -11
index 5302ee282409..a4346dd673b1 100644
@@ -81,6 +81,7 @@ static const char * const attach_type_strings[] = {
  	[BPF_SK_SKB_STREAM_PARSER] = "stream_parser",
  	[BPF_SK_SKB_STREAM_VERDICT] = "stream_verdict",
  	[BPF_SK_MSG_VERDICT] = "msg_verdict",
+	[BPF_FLOW_DISSECTOR] = "flow_dissector",
  	[__MAX_BPF_ATTACH_TYPE] = NULL,
  };

@@ -724,10 +725,11 @@ int map_replace_compar(const void *p1, const void *p2)
  static int do_attach(int argc, char **argv)
  {
  	enum bpf_attach_type attach_type;
-	int err, mapfd, progfd;
+	int err, progfd;
+	int mapfd = 0;
-	if (!REQ_ARGS(5)) {
-		p_err("too few parameters for map attach");
+	if (!REQ_ARGS(3)) {
+		p_err("too few parameters for attach");
  		return -EINVAL;
  	}

@@ -740,11 +742,17 @@ static int do_attach(int argc, char **argv)
  		p_err("invalid attach type");
  		return -EINVAL;
  	}
-	NEXT_ARG();
+	if (attach_type != BPF_FLOW_DISSECTOR) {
+		NEXT_ARG();
+		if (!REQ_ARGS(2)) {
+			p_err("too few parameters for map attach");
+			return -EINVAL;
+		}
-	mapfd = map_parse_fd(&argc, &argv);
-	if (mapfd < 0)
-		return mapfd;
+		mapfd = map_parse_fd(&argc, &argv);
+		if (mapfd < 0)
+			return mapfd;
+	}
  	err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
  	if (err) {

@@ -760,10 +768,11 @@ static int do_attach(int argc, char **argv)
  static int do_detach(int argc, char **argv)
  {
  	enum bpf_attach_type attach_type;
-	int err, mapfd, progfd;
+	int err, progfd;
+	int mapfd = 0;
-	if (!REQ_ARGS(5)) {
-		p_err("too few parameters for map detach");
+	if (!REQ_ARGS(3)) {
+		p_err("too few parameters for detach");
  		return -EINVAL;
  	}

@@ -776,11 +785,17 @@ static int do_detach(int argc, char **argv)
  		p_err("invalid attach type");
  		return -EINVAL;
  	}
-	NEXT_ARG();
+	if (attach_type != BPF_FLOW_DISSECTOR) {
+		NEXT_ARG();
+		if (!REQ_ARGS(2)) {
Quoted 8 lines

tools/bpf/bpftool/prog.c static int do_detach(int argc, char **argv) · line 793

Quoted 3 lines
tools/bpf/bpftool/prog.c +3 -0
+			p_err("too few parameters for map detach");
+			return -EINVAL;
+		}

Would that make sense to factor argument checks or parsing for do_attach() and do_detach() to some extent? In order to reduce the number of attach-type-based exceptions to add in the code if we have other attach types that do not take maps in the future.

Quoted 2 lines

I can move all argument parsing into a new function and use it from both do_attach and do_detach.

Sounds good to me, thanks!

Quoted 71 lines
Quoted 63 lines
Quoted 63 lines
tools/bpf/bpftool/prog.c +24 -12
-	mapfd = map_parse_fd(&argc, &argv);
-	if (mapfd < 0)
-		return mapfd;
+		mapfd = map_parse_fd(&argc, &argv);
+		if (mapfd < 0)
+			return mapfd;
+	}
  	err = bpf_prog_detach2(progfd, mapfd, attach_type);
  	if (err) {

@@ -792,15 +807,16 @@ static int do_detach(int argc, char **argv)
  		jsonw_null(json_wtr);
  	return 0;
  }
-static int do_load(int argc, char **argv)
+
+static int load_with_options(int argc, char **argv, bool first_prog_only)
  {
  	enum bpf_attach_type expected_attach_type;
  	struct bpf_object_open_attr attr = {
  		.prog_type	= BPF_PROG_TYPE_UNSPEC,
  	};
  	struct map_replace *map_replace = NULL;
+	struct bpf_program *prog = NULL, *pos;
  	unsigned int old_map_fds = 0;
-	struct bpf_program *prog;
  	struct bpf_object *obj;
  	struct bpf_map *map;
  	const char *pinfile;

@@ -918,14 +934,20 @@ static int do_load(int argc, char **argv)
  		goto err_free_reuse_maps;
  	}
-	prog = bpf_program__next(NULL, obj);
-	if (!prog) {
-		p_err("object file doesn't contain any bpf program");
-		goto err_close_obj;
+	if (first_prog_only) {
+		prog = bpf_program__next(NULL, obj);
+		if (!prog) {
+			p_err("object file doesn't contain any bpf program");
+			goto err_close_obj;
+		}
  	}
-	bpf_program__set_ifindex(prog, ifindex);
  	if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
+		if (!prog) {
+			p_err("can not guess program type when loading all programs\n");
+			goto err_close_obj;
+		}
+
  		const char *sec_name = bpf_program__title(prog, false);
  		err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,

@@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
  			goto err_close_obj;
  		}
  	}
-	bpf_program__set_type(prog, attr.prog_type);
-	bpf_program__set_expected_attach_type(prog, expected_attach_type);
+
+	bpf_object__for_each_program(pos, obj) {
+		bpf_program__set_ifindex(pos, ifindex);
+		bpf_program__set_type(pos, attr.prog_type);
+		bpf_program__set_expected_attach_type(pos,
+						      expected_attach_type);
Quoted 8 lines

tools/bpf/bpftool/prog.c static int do_load(int argc, char **argv) · line 967

Quoted 1 line
tools/bpf/bpftool/prog.c +1 -0
+	}

I still believe you can have programs of different types here, and be able to load them. I tried it and managed to have it working fine. If no type is provided from command line we can retrieve types for each program from its section name. If a type is provided on the command line, we can do the same, but I am not sure we should do it, or impose that type for all programs instead.

Quoted 4 lines

I can move auto-detection into this new bpf_object__for_each_program loop. So if no type is specified, try to infer the type from each prog section name, otherwise, use the provided one for all progs. Do we want something like that?

This is what I have in mind. But others may disagree.

Quoted 2 lines

Btw, do you have some existing real life example of where it's needed so I can test this new implementation? (maybe something under samples/ ?)

I thought about an ELF file containing both an XDP and a TC classifier program for example. XDP can mark programs for TC, then TC process them with all the facilities we have for skbs. It does not _have_ to be in the same ELF file, but could be.

I haven't searched samples/bpf/ in depth, but a grep on SEC shows a couple of files with several types (kprobe/kretprobe, classifier/xdp). samples/bpf/xdp2skb_meta_kern.c looks like a good candidate. Or actually for testing purposes, I simply used the following:

	#define SEC(NAME) __attribute__((section(NAME), used))

	int _version SEC("version") = 1;

	SEC("classifier")
	int func()
	{
		return 1;
	}

	SEC("xdp")
	int funcbar()
	{
		return 0;
	}
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
tools/bpf/bpftool/prog.c +0 -1
  	qsort(map_replace, old_map_fds, sizeof(*map_replace),
  	      map_replace_compar);

@@ -1001,9 +1028,25 @@ static int do_load(int argc, char **argv)
  		goto err_close_obj;
  	}
-	if (do_pin_fd(bpf_program__fd(prog), pinfile))
Quoted 8 lines

tools/bpf/bpftool/prog.c static int do_load(int argc, char **argv) · line 1035

Quoted 4 lines
tools/bpf/bpftool/prog.c +3 -0
+	err = mount_bpffs_for_pin(pinfile);
+	if (err)
  		goto err_close_obj;
+	if (prog) {

Nit: Maybe "if (first_prog_only) {" instead? If I understand correctly, at this stage it should be equivalent, but in my opinion it would make it easier to understand why we have two cases here.

Quoted 2 lines

Sure, I can do that if you think that's more readable, I don't have a preference.

Thanks!
Quentin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <http://lists.linaro.org/pipermail/linux-kselftest-mirror/attachments/20181108/b15e0ad6/attachment.sig>

#8 lore raw
Quoted 409 lines · Quentin Monnet

2018-11-08 10:01 UTC-0800 ~ Stanislav Fomichev <sdf at fomichev.me>

Quoted 110 lines · Quentin Monnet
Quoted 1 line

Hi Stanislav, thanks for the changes! More comments below.

Thank you for another round of review!

Quoted 98 lines

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

Quoted 97 lines

This commit adds support for loading/attaching/detaching flow dissector program. The structure of the flow dissector program is assumed to be the same as in the selftests:

* flow_dissector section with the main entry point
* a bunch of tail call progs
* a jmp_table map that is populated with the tail call progs

When bpftool load is called with a flow_dissector prog (i.e. when the first section is flow_dissector of 'type flow_dissector' argument is passed), we load and pin all the programs/maps. 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

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>
---
  .../bpftool/Documentation/bpftool-prog.rst    |  36 ++++--
  tools/bpf/bpftool/bash-completion/bpftool     |   6 +-
  tools/bpf/bpftool/common.c                    |  30 ++---
  tools/bpf/bpftool/main.h                      |   1 +
  tools/bpf/bpftool/prog.c                      | 112 +++++++++++++-----
  5 files changed, 126 insertions(+), 59 deletions(-)
tools/bpf/bpftool/Documentation/bpftool-prog.rst +9 -6
index ac4e904b10fb..0374634c3087 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,9 +25,9 @@ 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 attach** *PROG* *ATTACH_TYPE* *MAP*
-|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
+|	**bpftool** **prog { load | loadall }** *OBJ* *FILE* [**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**
  |
  |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }

@@ -39,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

@@ -79,8 +82,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*]
+	**bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
Quoted 8 lines

tools/bpf/bpftool/Documentation/bpftool-prog.rst DESCRIPTION · line 89

Quoted 4 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +3 -0
  		  Load bpf program from binary *OBJ* and pin as *FILE*.
+		  **bpftool prog load** will pin only the first bpf program
+		  from the *OBJ*, **bpftool prog loadall** will pin all maps
+		  and programs from the *OBJ*.

This could be improved regarding maps: with "bpftool prog load" I think we also load and pin all maps, but your description implies this is only the case with "loadall"

Quoted 4 lines

I don't think we pin any maps with bpftool prog load, we certainly load them, but we don't pin any afaict. Can you point me to the code where we pin the maps?

My bad. I read "pin" but thought "load". It does not pin them indeed, sorry about that.

Quoted 27 lines
Quoted 19 lines
Quoted 19 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +6 -7
  		  **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

@@ -97,13 +103,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.
+
Quoted 8 lines

tools/bpf/bpftool/Documentation/bpftool-prog.rst DESCRIPTION · line 116

Quoted 5 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +5 -0
+        **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.

While at it could you please fix those two paragraphs to use tabs for indentation, as the rest of the doc? Thanks!

Quoted 1 line

Time to teach my vim to use tabs in .rst files. Sorry about that.

Those paragraphs were using spaces already, so you didn't introduce that :). But all others use tabs so its a good occasion to fix it.

Quoted 182 lines
Quoted 25 lines
Quoted 25 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +0 -0
  	**bpftool prog help**
  		  Print short help message.
tools/bpf/bpftool/bash-completion/bpftool +2 -2
index 3f78e6404589..ad0fc919f7ec 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

@@ -299,7 +299,7 @@ _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

@@ -309,7 +309,7 @@ _bpftool()
                      fi
                      return 0
Quoted 8 lines

tools/bpf/bpftool/bash-completion/bpftool _bpftool() · line 315

Quoted 5 lines
tools/bpf/bpftool/bash-completion/bpftool +1 -1
                      ;;
-                load)
+                load|loadall)
                      local obj
                      if [[ ${#words[@]} -lt 6 ]]; then

You also want to update completion for the program types, at line 341 or so. Feel free to split that list on several lines, by the way :).

Will do, thanks!

Quoted 21 lines
Quoted 21 lines
tools/bpf/bpftool/common.c +1 -5
index 25af85304ebe..f671a921dec5 100644
@@ -169,34 +169,24 @@ 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);
Quoted 8 lines

tools/bpf/bpftool/common.c int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type) · line 186

Quoted 6 lines
tools/bpf/bpftool/common.c +2 -2
-	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;
  	}

Nitpick: unnecessary brackets.

Ack.

Quoted 108 lines
Quoted 108 lines
tools/bpf/bpftool/common.c +12 -8
-	/* 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 +194,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 +22 -11
index 5302ee282409..a4346dd673b1 100644
@@ -81,6 +81,7 @@ static const char * const attach_type_strings[] = {
  	[BPF_SK_SKB_STREAM_PARSER] = "stream_parser",
  	[BPF_SK_SKB_STREAM_VERDICT] = "stream_verdict",
  	[BPF_SK_MSG_VERDICT] = "msg_verdict",
+	[BPF_FLOW_DISSECTOR] = "flow_dissector",
  	[__MAX_BPF_ATTACH_TYPE] = NULL,
  };

@@ -724,10 +725,11 @@ int map_replace_compar(const void *p1, const void *p2)
  static int do_attach(int argc, char **argv)
  {
  	enum bpf_attach_type attach_type;
-	int err, mapfd, progfd;
+	int err, progfd;
+	int mapfd = 0;
-	if (!REQ_ARGS(5)) {
-		p_err("too few parameters for map attach");
+	if (!REQ_ARGS(3)) {
+		p_err("too few parameters for attach");
  		return -EINVAL;
  	}

@@ -740,11 +742,17 @@ static int do_attach(int argc, char **argv)
  		p_err("invalid attach type");
  		return -EINVAL;
  	}
-	NEXT_ARG();
+	if (attach_type != BPF_FLOW_DISSECTOR) {
+		NEXT_ARG();
+		if (!REQ_ARGS(2)) {
+			p_err("too few parameters for map attach");
+			return -EINVAL;
+		}
-	mapfd = map_parse_fd(&argc, &argv);
-	if (mapfd < 0)
-		return mapfd;
+		mapfd = map_parse_fd(&argc, &argv);
+		if (mapfd < 0)
+			return mapfd;
+	}
  	err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
  	if (err) {

@@ -760,10 +768,11 @@ static int do_attach(int argc, char **argv)
  static int do_detach(int argc, char **argv)
  {
  	enum bpf_attach_type attach_type;
-	int err, mapfd, progfd;
+	int err, progfd;
+	int mapfd = 0;
-	if (!REQ_ARGS(5)) {
-		p_err("too few parameters for map detach");
+	if (!REQ_ARGS(3)) {
+		p_err("too few parameters for detach");
  		return -EINVAL;
  	}

@@ -776,11 +785,17 @@ static int do_detach(int argc, char **argv)
  		p_err("invalid attach type");
  		return -EINVAL;
  	}
-	NEXT_ARG();
+	if (attach_type != BPF_FLOW_DISSECTOR) {
+		NEXT_ARG();
+		if (!REQ_ARGS(2)) {
Quoted 8 lines

tools/bpf/bpftool/prog.c static int do_detach(int argc, char **argv) · line 793

Quoted 3 lines
tools/bpf/bpftool/prog.c +3 -0
+			p_err("too few parameters for map detach");
+			return -EINVAL;
+		}

Would that make sense to factor argument checks or parsing for do_attach() and do_detach() to some extent? In order to reduce the number of attach-type-based exceptions to add in the code if we have other attach types that do not take maps in the future.

Quoted 2 lines

I can move all argument parsing into a new function and use it from both do_attach and do_detach.

Sounds good to me, thanks!

Quoted 71 lines
Quoted 71 lines
Quoted 54 lines
tools/bpf/bpftool/prog.c +18 -10
-	mapfd = map_parse_fd(&argc, &argv);
-	if (mapfd < 0)
-		return mapfd;
+		mapfd = map_parse_fd(&argc, &argv);
+		if (mapfd < 0)
+			return mapfd;
+	}
  	err = bpf_prog_detach2(progfd, mapfd, attach_type);
  	if (err) {

@@ -792,15 +807,16 @@ static int do_detach(int argc, char **argv)
  		jsonw_null(json_wtr);
  	return 0;
  }
-static int do_load(int argc, char **argv)
+
+static int load_with_options(int argc, char **argv, bool first_prog_only)
  {
  	enum bpf_attach_type expected_attach_type;
  	struct bpf_object_open_attr attr = {
  		.prog_type	= BPF_PROG_TYPE_UNSPEC,
  	};
  	struct map_replace *map_replace = NULL;
+	struct bpf_program *prog = NULL, *pos;
  	unsigned int old_map_fds = 0;
-	struct bpf_program *prog;
  	struct bpf_object *obj;
  	struct bpf_map *map;
  	const char *pinfile;

@@ -918,14 +934,20 @@ static int do_load(int argc, char **argv)
  		goto err_free_reuse_maps;
  	}
-	prog = bpf_program__next(NULL, obj);
-	if (!prog) {
-		p_err("object file doesn't contain any bpf program");
-		goto err_close_obj;
+	if (first_prog_only) {
+		prog = bpf_program__next(NULL, obj);
+		if (!prog) {
+			p_err("object file doesn't contain any bpf program");
+			goto err_close_obj;
+		}
  	}
-	bpf_program__set_ifindex(prog, ifindex);
  	if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
+		if (!prog) {
+			p_err("can not guess program type when loading all programs\n");
+			goto err_close_obj;
+		}
+
  		const char *sec_name = bpf_program__title(prog, false);
  		err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,

@@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
  			goto err_close_obj;
  		}

tools/bpf/bpftool/prog.c static int do_load(int argc, char **argv) · line 967

Quoted 10 lines
tools/bpf/bpftool/prog.c +7 -2
  	}
-	bpf_program__set_type(prog, attr.prog_type);
-	bpf_program__set_expected_attach_type(prog, expected_attach_type);
+
+	bpf_object__for_each_program(pos, obj) {
+		bpf_program__set_ifindex(pos, ifindex);
+		bpf_program__set_type(pos, attr.prog_type);
+		bpf_program__set_expected_attach_type(pos,
+						      expected_attach_type);
+	}

I still believe you can have programs of different types here, and be able to load them. I tried it and managed to have it working fine. If no type is provided from command line we can retrieve types for each program from its section name. If a type is provided on the command line, we can do the same, but I am not sure we should do it, or impose that type for all programs instead.

Quoted 6 lines
Quoted 4 lines

I can move auto-detection into this new bpf_object__for_each_program loop. So if no type is specified, try to infer the type from each prog section name, otherwise, use the provided one for all progs. Do we want something like that?

This is what I have in mind. But others may disagree.

Hm, I did another look and here is the problem.
Current **load** implementation actually loads all progs (and maps) from the
file: do_load -> bpf_object__load -> bpf_object__load_progs. It pins only the first prog, but loads them all. That's why I don't understand how multiprog loading can work currently (because we don't set correct prog_type/expected_attach_type for all progs, only the first one).

Should we have the following functionality instead:
Both **load** and **loadall** load all maps and programs from the *OBJ* and differ only in pinning. **load** pins only the first program from the *OBJ* as *FILE*. **loadall** pins all programs and maps from the *OBJ* under *FILE* directory.

If we want **load** to load only the first prog, we should probably amend bpf_object__load to be able to filter the progs we (don't) want.

Thoughts?

Quoted 13 lines
Quoted 2 lines

Btw, do you have some existing real life example of where it's needed so I can test this new implementation? (maybe something under samples/ ?)

I thought about an ELF file containing both an XDP and a TC classifier program for example. XDP can mark programs for TC, then TC process them with all the facilities we have for skbs. It does not _have_ to be in the same ELF file, but could be.

I haven't searched samples/bpf/ in depth, but a grep on SEC shows a couple of files with several types (kprobe/kretprobe, classifier/xdp). samples/bpf/xdp2skb_meta_kern.c looks like a good candidate. Or actually for testing purposes, I simply used the following:

Quoted 15 lines
	#define SEC(NAME) __attribute__((section(NAME), used))

	int _version SEC("version") = 1;

	SEC("classifier")
	int func()
	{
		return 1;
	}

	SEC("xdp")
	int funcbar()
	{
		return 0;
	}

Thanks, I just found socket_cookie_prog.c in selftests which has cgroup/connect6 and sockops sections, I can probably use that for testing.

Quoted 20 lines
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
tools/bpf/bpftool/prog.c +0 -1
  	qsort(map_replace, old_map_fds, sizeof(*map_replace),
  	      map_replace_compar);

@@ -1001,9 +1028,25 @@ static int do_load(int argc, char **argv)
  		goto err_close_obj;
  	}
-	if (do_pin_fd(bpf_program__fd(prog), pinfile))
Quoted 8 lines

tools/bpf/bpftool/prog.c static int do_load(int argc, char **argv) · line 1035

Quoted 4 lines
tools/bpf/bpftool/prog.c +3 -0
+	err = mount_bpffs_for_pin(pinfile);
+	if (err)
  		goto err_close_obj;
+	if (prog) {

Nit: Maybe "if (first_prog_only) {" instead? If I understand correctly, at this stage it should be equivalent, but in my opinion it would make it easier to understand why we have two cases here.

Quoted 2 lines

Sure, I can do that if you think that's more readable, I don't have a preference.

Thanks!
Quentin

#9 lore raw
In reply to Quentin Monnet
Quoted 13 lines · Quentin Monnet
Quoted 13 lines
Quoted 13 lines

tools/bpf/bpftool/Documentation/bpftool-prog.rst DESCRIPTION · line 88

Quoted 9 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +4 -1
@@ -79,8 +82,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*]
+	**bpftool prog { load | loadall }** *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** will pin only the first bpf program
+		  from the *OBJ*, **bpftool prog loadall** will pin all maps
+		  and programs from the *OBJ*.

This could be improved regarding maps: with "bpftool prog load" I think we also load and pin all maps, but your description implies this is only the case with "loadall"

Quoted 7 lines
Quoted 4 lines

I don't think we pin any maps with bpftool prog load, we certainly load them, but we don't pin any afaict. Can you point me to the code where we pin the maps?

My bad. I read "pin" but thought "load". It does not pin them indeed, sorry about that.

Right, but I don't see much reason why prog loadall should pin maps. The reason to pin program(s) is to hold some reference and to be able to find them. Since we have the programs pinned we should be able to find their maps with relative ease.

$ bpftool prog show pinned /sys/fs/bpf/prog
7: cgroup_skb  tag 2a142ef67aaad174  gpl
	loaded_at 2018-11-08T11:02:25-0800  uid 0
	xlated 296B  jited 229B  memlock 4096B  map_ids 6,7

possibly:

$ bpftool -j prog show pinned /sys/fs/bpf/prog | jq '.map_ids[0]'
6

Moreover, I think program and map names may collide making ELFs unloadable..

#10 lore raw
Quoted 19 lines · Jakub Kicinski
Quoted 18 lines · Quentin Monnet
Quoted 17 lines
Quoted 5 lines
Quoted 5 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +1 -1
@@ -79,8 +82,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*]
+	**bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
Quoted 8 lines

tools/bpf/bpftool/Documentation/bpftool-prog.rst DESCRIPTION · line 89

Quoted 4 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +3 -0
  		  Load bpf program from binary *OBJ* and pin as *FILE*.
+		  **bpftool prog load** will pin only the first bpf program
+		  from the *OBJ*, **bpftool prog loadall** will pin all maps
+		  and programs from the *OBJ*.

This could be improved regarding maps: with "bpftool prog load" I think we also load and pin all maps, but your description implies this is only the case with "loadall"

I don't think we pin any maps with bpftool prog load, we certainly load them, but we don't pin any afaict. Can you point me to the code where we pin the maps?

Quoted 4 lines
Quoted 2 lines

My bad. I read "pin" but thought "load". It does not pin them indeed, sorry about that.

Right, but I don't see much reason why prog loadall should pin maps.

It does seem convenient to have an option to pin everything, so we don't require anything else to find the ids of the maps. If we are pinning all progs, might as well pin the maps, why not? See my example in the commit message, for example, where I just hard code the expected map name. Convenient :-)

Quoted 9 lines

The reason to pin program(s) is to hold some reference and to be able to find them. Since we have the programs pinned we should be able to find their maps with relative ease.

$ bpftool prog show pinned /sys/fs/bpf/prog
7: cgroup_skb  tag 2a142ef67aaad174  gpl
	loaded_at 2018-11-08T11:02:25-0800  uid 0
	xlated 296B  jited 229B  memlock 4096B  map_ids 6,7
Quoted 7 lines

possibly:

$ bpftool -j prog show pinned /sys/fs/bpf/prog | jq '.map_ids[0]'
6

Moreover, I think program and map names may collide making ELFs unloadable..

It doesn't sound like a big problem, sounds like a constraint we can live with.

#11 lore raw
Quoted 23 lines · Stanislav Fomichev
Quoted 22 lines · Jakub Kicinski
Quoted 20 lines · Quentin Monnet
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +1 -1
@@ -79,8 +82,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*]
+	**bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
Quoted 8 lines

tools/bpf/bpftool/Documentation/bpftool-prog.rst DESCRIPTION · line 89

Quoted 4 lines
tools/bpf/bpftool/Documentation/bpftool-prog.rst +3 -0
  		  Load bpf program from binary *OBJ* and pin as *FILE*.
+		  **bpftool prog load** will pin only the first bpf program
+		  from the *OBJ*, **bpftool prog loadall** will pin all maps
+		  and programs from the *OBJ*.

This could be improved regarding maps: with "bpftool prog load" I think we also load and pin all maps, but your description implies this is only the case with "loadall"

Quoted 4 lines

I don't think we pin any maps with bpftool prog load, we certainly load them, but we don't pin any afaict. Can you point me to the code where we pin the maps?

My bad. I read "pin" but thought "load". It does not pin them indeed, sorry about that.

Quoted 6 lines
Quoted 1 line

Right, but I don't see much reason why prog loadall should pin maps.

It does seem convenient to have an option to pin everything, so we don't require anything else to find the ids of the maps. If we are pinning all progs, might as well pin the maps, why not? See my example in the commit message, for example, where I just hard code the expected map name. Convenient :-)

Sure. I can see how its convenient to your use case. A lot of people will be very used to finding out the maps because if program is loaded with iproute2 tools neither programs nor maps get pinned.

Please add a "pinmaps MAP_DIR" optional parameter as a separate patch.

Quoted 18 lines
Quoted 9 lines

The reason to pin program(s) is to hold some reference and to be able to find them. Since we have the programs pinned we should be able to find their maps with relative ease.

$ bpftool prog show pinned /sys/fs/bpf/prog
7: cgroup_skb  tag 2a142ef67aaad174  gpl
	loaded_at 2018-11-08T11:02:25-0800  uid 0
	xlated 296B  jited 229B  memlock 4096B  map_ids 6,7
Quoted 7 lines

possibly:

$ bpftool -j prog show pinned /sys/fs/bpf/prog | jq '.map_ids[0]'
6

Moreover, I think program and map names may collide making ELFs unloadable..

It doesn't sound like a big problem, sounds like a constraint we can live with.

#12 lore raw
In reply to Quentin Monnet
Quoted 4 lines · Quentin Monnet

tools/bpf/bpftool/prog.c static int do_load(int argc, char **argv) · line 947

Quoted 4 lines
tools/bpf/bpftool/prog.c +2 -1
-	bpf_program__set_ifindex(prog, ifindex);
  	if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
+		if (!prog) {
+			p_err("can not guess program type when loading all programs\n");

No new lines in p_err(), beaks JSON.

Quoted 18 lines
Quoted 18 lines
tools/bpf/bpftool/prog.c +9 -2
+			goto err_close_obj;
+		}
+
  		const char *sec_name = bpf_program__title(prog, false);
 
  		err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,

@@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
  			goto err_close_obj;
  		}
  	}
-	bpf_program__set_type(prog, attr.prog_type);
-	bpf_program__set_expected_attach_type(prog, expected_attach_type);
+
+	bpf_object__for_each_program(pos, obj) {
+		bpf_program__set_ifindex(pos, ifindex);
+		bpf_program__set_type(pos, attr.prog_type);
+		bpf_program__set_expected_attach_type(pos,
+						      expected_attach_type);
Quoted 8 lines

Review static int do_load(int argc, char **argv) · line 967

Quoted 1 line
(quoted diff) +1 -0
+	}

I still believe you can have programs of different types here, and be able to load them. I tried it and managed to have it working fine. If no type is provided from command line we can retrieve types for each program from its section name. If a type is provided on the command line, we can do the same, but I am not sure we should do it, or impose that type for all programs instead.

attr->prog_type is one per object, though. How do we set that one?

#13 lore raw
Quoted 7 lines · Jakub Kicinski
Quoted 4 lines · Quentin Monnet

tools/bpf/bpftool/prog.c static int do_load(int argc, char **argv) · line 947

Quoted 4 lines
tools/bpf/bpftool/prog.c +2 -1
-	bpf_program__set_ifindex(prog, ifindex);
  	if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
+		if (!prog) {
+			p_err("can not guess program type when loading all programs\n");

No new lines in p_err(), beaks JSON.

Thanks, I'll probably remove that altogether and do auto inference of prog_type from the section name here.

Quoted 27 lines
Quoted 18 lines
Quoted 18 lines
tools/bpf/bpftool/prog.c +9 -2
+			goto err_close_obj;
+		}
+
  		const char *sec_name = bpf_program__title(prog, false);
 
  		err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,

@@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
  			goto err_close_obj;
  		}
  	}
-	bpf_program__set_type(prog, attr.prog_type);
-	bpf_program__set_expected_attach_type(prog, expected_attach_type);
+
+	bpf_object__for_each_program(pos, obj) {
+		bpf_program__set_ifindex(pos, ifindex);
+		bpf_program__set_type(pos, attr.prog_type);
+		bpf_program__set_expected_attach_type(pos,
+						      expected_attach_type);
Quoted 8 lines

Review static int do_load(int argc, char **argv) · line 967

Quoted 1 line
(quoted diff) +1 -0
+	}

I still believe you can have programs of different types here, and be able to load them. I tried it and managed to have it working fine. If no type is provided from command line we can retrieve types for each program from its section name. If a type is provided on the command line, we can do the same, but I am not sure we should do it, or impose that type for all programs instead.

Quoted 1 line

attr->prog_type is one per object, though. How do we set that one?

Isn't it used only in __bpf_object__open_xattr to require/not-require kernel version in the bpf prog?

It will probably work quite nicely for both of our options:

* type not specified: prog_type = BPF_PROG_TYPE_UNSPEC, need kernel version (over cautious?)
* type specified (and applied to all progs): using bpf_prog_type__needs_kver to require/not requqire kernel version

#14 lore raw
Quoted 31 lines · Stanislav Fomichev
Quoted 27 lines
Quoted 18 lines
Quoted 18 lines
tools/bpf/bpftool/prog.c +9 -2
+			goto err_close_obj;
+		}
+
  		const char *sec_name = bpf_program__title(prog, false);
 
  		err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,

@@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
  			goto err_close_obj;
  		}
  	}
-	bpf_program__set_type(prog, attr.prog_type);
-	bpf_program__set_expected_attach_type(prog, expected_attach_type);
+
+	bpf_object__for_each_program(pos, obj) {
+		bpf_program__set_ifindex(pos, ifindex);
+		bpf_program__set_type(pos, attr.prog_type);
+		bpf_program__set_expected_attach_type(pos,
+						      expected_attach_type);
Quoted 8 lines

Review static int do_load(int argc, char **argv) · line 967

Quoted 1 line
(quoted diff) +1 -0
+	}

I still believe you can have programs of different types here, and be able to load them. I tried it and managed to have it working fine. If no type is provided from command line we can retrieve types for each program from its section name. If a type is provided on the command line, we can do the same, but I am not sure we should do it, or impose that type for all programs instead.

Quoted 1 line

attr->prog_type is one per object, though. How do we set that one?

Isn't it used only in __bpf_object__open_xattr to require/not-require kernel version in the bpf prog?

Quoted 6 lines

It will probably work quite nicely for both of our options:

* type not specified: prog_type = BPF_PROG_TYPE_UNSPEC, need kernel version (over cautious?)
* type specified (and applied to all progs): using bpf_prog_type__needs_kver to require/not requqire kernel version

Right, but they you can't infer it from the program name, since there's multiple.

#15 lore raw
In reply to Stanislav Fomichev
Quoted 7 lines · Stanislav Fomichev

This commit adds support for loading/attaching/detaching flow dissector program. The structure of the flow dissector program is assumed to be the same as in the selftests:

* flow_dissector section with the main entry point
* a bunch of tail call progs
* a jmp_table map that is populated with the tail call progs

Could you split the loadall changes and the flow_dissector changes into two separate patches?

#16 lore raw
Quoted 11 lines · Jakub Kicinski
Quoted 7 lines · Stanislav Fomichev

This commit adds support for loading/attaching/detaching flow dissector program. The structure of the flow dissector program is assumed to be the same as in the selftests:

* flow_dissector section with the main entry point
* a bunch of tail call progs
* a jmp_table map that is populated with the tail call progs

Could you split the loadall changes and the flow_dissector changes into two separate patches?

Sure, will do, but let's first agree on the semantical differences of load vs loadall.

So far *load* actually loads _all_ progs (via bpf_object__load), but pins only the first program. Is that what we want? I wonder whether the assumption there was that there is only single program in the object. Should we load only the first program in *load*?

If we add *loadall*, then the difference would be:
*load*:
* loads all maps and only the first program, pins only the first program
*loadall*:
* loads all maps and all programs, pins everything (maps and programs)

Is this the expected behavior?

#17 lore raw
Quoted 20 lines · Stanislav Fomichev
Quoted 11 lines · Jakub Kicinski
Quoted 7 lines · Stanislav Fomichev

This commit adds support for loading/attaching/detaching flow dissector program. The structure of the flow dissector program is assumed to be the same as in the selftests:

* flow_dissector section with the main entry point
* a bunch of tail call progs
* a jmp_table map that is populated with the tail call progs

Could you split the loadall changes and the flow_dissector changes into two separate patches?

Sure, will do, but let's first agree on the semantical differences of load vs loadall.

So far *load* actually loads _all_ progs (via bpf_object__load), but pins only the first program. Is that what we want? I wonder whether the assumption there was that there is only single program in the object. Should we load only the first program in *load*?

Quoted 8 lines

If we add *loadall*, then the difference would be:
*load*:
* loads all maps and only the first program, pins only the first program
*loadall*:
* loads all maps and all programs, pins everything (maps and programs)

Is this the expected behavior?

Loading all programs and maps for "load" is just a libbpf limitation we can remove at some point.