[PATCH bpf-next 0/2] bpftool: support loading flow dissector

[PATCH bpf-next 0/2] bpftool: support loading flow dissector

[Stanislav Fomichev]

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

See second patch for the description/details.

Stanislav Fomichev (2):
  selftests/bpf: rename flow dissector section to flow_dissector
  bpftool: support loading flow dissector

 .../bpftool/Documentation/bpftool-prog.rst    |  16 ++-
 tools/bpf/bpftool/common.c                    |  32 +++--
 tools/bpf/bpftool/main.h                      |   1 +
 tools/bpf/bpftool/prog.c                      | 135 +++++++++++++++---
 tools/testing/selftests/bpf/bpf_flow.c        |   2 +-
 .../selftests/bpf/test_flow_dissector.sh      |   2 +-
 6 files changed, 143 insertions(+), 45 deletions(-)

-- 
2.19.1.930.g4563a0d9d0-goog

[PATCH bpf-next 1/2] selftests/bpf: rename flow dissector section to flow_dissector

[Stanislav Fomichev]

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@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(-)

diff --git a/tools/testing/selftests/bpf/bpf_flow.c b/tools/testing/selftests/bpf/bpf_flow.c
index 107350a7821d..b9798f558ca7 100644
--- a/tools/testing/selftests/bpf/bpf_flow.c
+++ b/tools/testing/selftests/bpf/bpf_flow.c
@@ -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)
diff --git a/tools/testing/selftests/bpf/test_flow_dissector.sh b/tools/testing/selftests/bpf/test_flow_dissector.sh
index c0fb073b5eab..d23d4da66b83 100755
--- a/tools/testing/selftests/bpf/test_flow_dissector.sh
+++ b/tools/testing/selftests/bpf/test_flow_dissector.sh
@@ -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
-- 
2.19.1.930.g4563a0d9d0-goog

[PATCH bpf-next 2/2] bpftool: support loading flow dissector

[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 and build the jump table.

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 prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector

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

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 .../bpftool/Documentation/bpftool-prog.rst    |  16 ++-
 tools/bpf/bpftool/common.c                    |  32 +++--
 tools/bpf/bpftool/main.h                      |   1 +
 tools/bpf/bpftool/prog.c                      | 135 +++++++++++++++---
 4 files changed, 141 insertions(+), 43 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index ac4e904b10fb..3caa9153435b 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -25,8 +25,8 @@ MAP COMMANDS
 |	**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 attach** *PROG* *ATTACH_TYPE* [*MAP*]
+|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
 |	**bpftool** **prog help**
 |
 |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
@@ -39,7 +39,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
@@ -97,13 +99,13 @@ DESCRIPTION
 		  contain a dot character ('.'), which is reserved for future
 		  extensions of *bpffs*.
 
-        **bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
+        **bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
                   Attach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
-                  to the map *MAP*.
+                  to the optional map *MAP*.
 
-        **bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
+        **bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
                   Detach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
-                  from the map *MAP*.
+                  from the optional map *MAP*.
 
 	**bpftool prog help**
 		  Print short help message.
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index 25af85304ebe..963881142dfb 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -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,22 @@ 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 = mount_bpffs_for_pin(name);
+
+	if (err) {
+		p_err("can't mount bpffs for pin %s: %s",
+		      name, strerror(errno));
+		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;
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 28322ace2856..1383824c9baf 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -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);
 
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 5302ee282409..f3a07ec3a444 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -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,9 +725,10 @@ 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)) {
+	if (!REQ_ARGS(3)) {
 		p_err("too few parameters for map attach");
 		return -EINVAL;
 	}
@@ -741,10 +743,11 @@ static int do_attach(int argc, char **argv)
 		return -EINVAL;
 	}
 	NEXT_ARG();
-
-	mapfd = map_parse_fd(&argc, &argv);
-	if (mapfd < 0)
-		return mapfd;
+	if (argc > 0) {
+		mapfd = map_parse_fd(&argc, &argv);
+		if (mapfd < 0)
+			return mapfd;
+	}
 
 	err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
 	if (err) {
@@ -760,9 +763,10 @@ 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)) {
+	if (!REQ_ARGS(3)) {
 		p_err("too few parameters for map detach");
 		return -EINVAL;
 	}
@@ -777,10 +781,11 @@ static int do_detach(int argc, char **argv)
 		return -EINVAL;
 	}
 	NEXT_ARG();
-
-	mapfd = map_parse_fd(&argc, &argv);
-	if (mapfd < 0)
-		return mapfd;
+	if (argc > 0) {
+		mapfd = map_parse_fd(&argc, &argv);
+		if (mapfd < 0)
+			return mapfd;
+	}
 
 	err = bpf_prog_detach2(progfd, mapfd, attach_type);
 	if (err) {
@@ -792,6 +797,56 @@ static int do_detach(int argc, char **argv)
 		jsonw_null(json_wtr);
 	return 0;
 }
+
+/* Flow dissector consists of a main program and a jump table for each
+ * supported protocol. The assumption here is that the first prog is the main
+ * one and the other progs are used in the tail calls. In this routine we
+ * build the jump table for the non-main progs.
+ */
+static int build_flow_dissector_jmp_table(struct bpf_object *obj,
+					  struct bpf_program *prog,
+					  const char *jmp_table_map)
+{
+	struct bpf_map *jmp_table;
+	struct bpf_program *pos;
+	int i = 0;
+	int prog_fd, jmp_table_fd, fd;
+
+	prog_fd = bpf_program__fd(prog);
+	if (prog_fd < 0) {
+		p_err("failed to get fd of main prog");
+		return prog_fd;
+	}
+
+	jmp_table = bpf_object__find_map_by_name(obj, jmp_table_map);
+	if (jmp_table == NULL) {
+		p_err("failed to find '%s' map", jmp_table_map);
+		return -1;
+	}
+
+	jmp_table_fd = bpf_map__fd(jmp_table);
+	if (jmp_table_fd < 0) {
+		p_err("failed to get fd of jmp_table");
+		return jmp_table_fd;
+	}
+
+	bpf_object__for_each_program(pos, obj) {
+		fd = bpf_program__fd(pos);
+		if (fd < 0) {
+			p_err("failed to get fd of '%s'",
+			      bpf_program__title(pos, false));
+			return fd;
+		}
+
+		if (fd != prog_fd) {
+			bpf_map_update_elem(jmp_table_fd, &i, &fd, BPF_ANY);
+			++i;
+		}
+	}
+
+	return 0;
+}
+
 static int do_load(int argc, char **argv)
 {
 	enum bpf_attach_type expected_attach_type;
@@ -800,7 +855,7 @@ static int do_load(int argc, char **argv)
 	};
 	struct map_replace *map_replace = NULL;
 	unsigned int old_map_fds = 0;
-	struct bpf_program *prog;
+	struct bpf_program *prog, *pos;
 	struct bpf_object *obj;
 	struct bpf_map *map;
 	const char *pinfile;
@@ -918,13 +973,19 @@ static int do_load(int argc, char **argv)
 		goto err_free_reuse_maps;
 	}
 
-	prog = bpf_program__next(NULL, obj);
+	if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
+		/* for the flow dissector type, the entry point is in the
+		 * section flow_dissector; other progs are tail calls
+		 */
+		prog = bpf_object__find_program_by_title(obj, "flow_dissector");
+	} else {
+		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) {
 		const char *sec_name = bpf_program__title(prog, false);
 
@@ -936,8 +997,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);
+	}
 
 	qsort(map_replace, old_map_fds, sizeof(*map_replace),
 	      map_replace_compar);
@@ -1001,8 +1067,34 @@ 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) {
+		p_err("failed to mount bpffs for pin '%s'", pinfile);
 		goto err_close_obj;
+	}
+
+	if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
+		err = build_flow_dissector_jmp_table(obj, prog, "jmp_table");
+		if (err) {
+			p_err("failed to build flow dissector jump table");
+			goto err_close_obj;
+		}
+		/* flow dissector consist of multiple programs,
+		 * we want to pin them all
+		 */
+		err = bpf_object__pin(obj, pinfile);
+		if (err) {
+			p_err("failed to pin flow dissector object");
+			goto err_close_obj;
+		}
+	} else {
+		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;
+		}
+	}
 
 	if (json_output)
 		jsonw_null(json_wtr);
@@ -1037,8 +1129,8 @@ static int do_help(int argc, char **argv)
 		"       %s %s pin   PROG FILE\n"
 		"       %s %s load  OBJ  FILE [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 +1142,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],
-- 
2.19.1.930.g4563a0d9d0-goog

Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector

[Jakub Kicinski]

On Wed,  7 Nov 2018 11:35:52 -0800, Stanislav Fomichev wrote:
> 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 and build the jump table.
> 
> 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 prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
> 
> Tested by using the above two lines to load the prog in
> the test_flow_dissector.sh selftest.
> 
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  .../bpftool/Documentation/bpftool-prog.rst    |  16 ++-
>  tools/bpf/bpftool/common.c                    |  32 +++--
>  tools/bpf/bpftool/main.h                      |   1 +
>  tools/bpf/bpftool/prog.c                      | 135 +++++++++++++++---

Please add the new attach type to bash completions.

>  4 files changed, 141 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index ac4e904b10fb..3caa9153435b 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -25,8 +25,8 @@ MAP COMMANDS
>  |	**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 attach** *PROG* *ATTACH_TYPE* [*MAP*]
> +|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
>  |	**bpftool** **prog help**
>  |
>  |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> @@ -39,7 +39,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
> @@ -97,13 +99,13 @@ DESCRIPTION
>  		  contain a dot character ('.'), which is reserved for future
>  		  extensions of *bpffs*.
>  
> -        **bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
> +        **bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
>                    Attach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> -                  to the map *MAP*.
> +                  to the optional map *MAP*.

Perhaps we can do better on help?  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*
> +        **bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
>                    Detach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> -                  from the map *MAP*.
> +                  from the optional map *MAP*.
>  
>  	**bpftool prog help**
>  		  Print short help message.
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 25af85304ebe..963881142dfb 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c

> @@ -204,10 +194,22 @@ 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 = mount_bpffs_for_pin(name);

Please don't initialize the error variable with a non-trivial function
call.

> +	if (err) {
> +		p_err("can't mount bpffs for pin %s: %s",
> +		      name, strerror(errno));

I think mount_bpffs_for_pin() will already print an error.  We can't
print two errors, because it will break JSON output.

> +		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;

> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 5302ee282409..f3a07ec3a444 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -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,9 +725,10 @@ 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)) {
> +	if (!REQ_ARGS(3)) {
>  		p_err("too few parameters for map attach");
>  		return -EINVAL;
>  	}
> @@ -741,10 +743,11 @@ static int do_attach(int argc, char **argv)
>  		return -EINVAL;
>  	}
>  	NEXT_ARG();
> -
> -	mapfd = map_parse_fd(&argc, &argv);
> -	if (mapfd < 0)
> -		return mapfd;
> +	if (argc > 0) {

Flow dissector can't need a map right?  I think explicitly checking for
the correct number of arguments once attach type is known would be good.

> +		mapfd = map_parse_fd(&argc, &argv);
> +		if (mapfd < 0)
> +			return mapfd;
> +	}
>  
>  	err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
>  	if (err) {
> @@ -760,9 +763,10 @@ 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)) {
> +	if (!REQ_ARGS(3)) {
>  		p_err("too few parameters for map detach");
>  		return -EINVAL;
>  	}
> @@ -777,10 +781,11 @@ static int do_detach(int argc, char **argv)
>  		return -EINVAL;
>  	}
>  	NEXT_ARG();
> -
> -	mapfd = map_parse_fd(&argc, &argv);
> -	if (mapfd < 0)
> -		return mapfd;
> +	if (argc > 0) {
> +		mapfd = map_parse_fd(&argc, &argv);
> +		if (mapfd < 0)
> +			return mapfd;
> +	}
>  
>  	err = bpf_prog_detach2(progfd, mapfd, attach_type);
>  	if (err) {
> @@ -792,6 +797,56 @@ static int do_detach(int argc, char **argv)
>  		jsonw_null(json_wtr);
>  	return 0;
>  }
> +
> +/* Flow dissector consists of a main program and a jump table for each
> + * supported protocol. The assumption here is that the first prog is the main
> + * one and the other progs are used in the tail calls. In this routine we
> + * build the jump table for the non-main progs.
> + */
> +static int build_flow_dissector_jmp_table(struct bpf_object *obj,
> +					  struct bpf_program *prog,
> +					  const char *jmp_table_map)
> +{
> +	struct bpf_map *jmp_table;
> +	struct bpf_program *pos;
> +	int i = 0;
> +	int prog_fd, jmp_table_fd, fd;

Please order variables longest to shortest.

> +	prog_fd = bpf_program__fd(prog);
> +	if (prog_fd < 0) {
> +		p_err("failed to get fd of main prog");
> +		return prog_fd;
> +	}
> +
> +	jmp_table = bpf_object__find_map_by_name(obj, jmp_table_map);
> +	if (jmp_table == NULL) {

nit: !jmp_table

> +		p_err("failed to find '%s' map", jmp_table_map);
> +		return -1;
> +	}
> +
> +	jmp_table_fd = bpf_map__fd(jmp_table);
> +	if (jmp_table_fd < 0) {
> +		p_err("failed to get fd of jmp_table");
> +		return jmp_table_fd;
> +	}
> +
> +	bpf_object__for_each_program(pos, obj) {
> +		fd = bpf_program__fd(pos);
> +		if (fd < 0) {
> +			p_err("failed to get fd of '%s'",
> +			      bpf_program__title(pos, false));
> +			return fd;
> +		}
> +
> +		if (fd != prog_fd) {
> +			bpf_map_update_elem(jmp_table_fd, &i, &fd, BPF_ANY);
> +			++i;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int do_load(int argc, char **argv)
>  {
>  	enum bpf_attach_type expected_attach_type;
> @@ -800,7 +855,7 @@ static int do_load(int argc, char **argv)
>  	};
>  	struct map_replace *map_replace = NULL;
>  	unsigned int old_map_fds = 0;
> -	struct bpf_program *prog;
> +	struct bpf_program *prog, *pos;

variable order

>  	struct bpf_object *obj;
>  	struct bpf_map *map;
>  	const char *pinfile;
> @@ -918,13 +973,19 @@ static int do_load(int argc, char **argv)
>  		goto err_free_reuse_maps;
>  	}
>  
> -	prog = bpf_program__next(NULL, obj);
> +	if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> +		/* for the flow dissector type, the entry point is in the
> +		 * section flow_dissector; other progs are tail calls
> +		 */
> +		prog = bpf_object__find_program_by_title(obj, "flow_dissector");
> +	} else {
> +		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) {
>  		const char *sec_name = bpf_program__title(prog, false);
>  
> @@ -936,8 +997,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);
> +	}
>  
>  	qsort(map_replace, old_map_fds, sizeof(*map_replace),
>  	      map_replace_compar);
> @@ -1001,8 +1067,34 @@ 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) {
> +		p_err("failed to mount bpffs for pin '%s'", pinfile);

Probably would be a duplicated error again?

>  		goto err_close_obj;
> +	}
> +
> +	if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> +		err = build_flow_dissector_jmp_table(obj, prog, "jmp_table");
> +		if (err) {
> +			p_err("failed to build flow dissector jump table");
> +			goto err_close_obj;
> +		}
> +		/* flow dissector consist of multiple programs,
> +		 * we want to pin them all

Why pin them all shouldn't the main program be the only one pinned?

> +		 */
> +		err = bpf_object__pin(obj, pinfile);
> +		if (err) {
> +			p_err("failed to pin flow dissector object");
> +			goto err_close_obj;
> +		}
> +	} else {
> +		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;
> +		}
> +	}

Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector

[Stanislav Fomichev]

On 11/07, Jakub Kicinski wrote:
> On Wed,  7 Nov 2018 11:35:52 -0800, Stanislav Fomichev wrote:
> > 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 and build the jump table.
> > 
> > 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 prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
> > 
> > Tested by using the above two lines to load the prog in
> > the test_flow_dissector.sh selftest.
> > 
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  .../bpftool/Documentation/bpftool-prog.rst    |  16 ++-
> >  tools/bpf/bpftool/common.c                    |  32 +++--
> >  tools/bpf/bpftool/main.h                      |   1 +
> >  tools/bpf/bpftool/prog.c                      | 135 +++++++++++++++---
> 
> Please add the new attach type to bash completions.
Thanks for a quick review! Will address everything in the v2.
Answered some of your questions below.

> >  4 files changed, 141 insertions(+), 43 deletions(-)
> > 
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > index ac4e904b10fb..3caa9153435b 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > @@ -25,8 +25,8 @@ MAP COMMANDS
> >  |	**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 attach** *PROG* *ATTACH_TYPE* [*MAP*]
> > +|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> >  |	**bpftool** **prog help**
> >  |
> >  |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> > @@ -39,7 +39,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
> > @@ -97,13 +99,13 @@ DESCRIPTION
> >  		  contain a dot character ('.'), which is reserved for future
> >  		  extensions of *bpffs*.
> >  
> > -        **bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
> > +        **bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> >                    Attach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> > -                  to the map *MAP*.
> > +                  to the optional map *MAP*.
> 
> Perhaps we can do better on help?  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*
> > +        **bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> >                    Detach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> > -                  from the map *MAP*.
> > +                  from the optional map *MAP*.
> >  
> >  	**bpftool prog help**
> >  		  Print short help message.
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index 25af85304ebe..963881142dfb 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
> 
> > @@ -204,10 +194,22 @@ 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 = mount_bpffs_for_pin(name);
> 
> Please don't initialize the error variable with a non-trivial function
> call.
> 
> > +	if (err) {
> > +		p_err("can't mount bpffs for pin %s: %s",
> > +		      name, strerror(errno));
> 
> I think mount_bpffs_for_pin() will already print an error.  We can't
> print two errors, because it will break JSON output.
> 
> > +		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;
> 
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index 5302ee282409..f3a07ec3a444 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -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,9 +725,10 @@ 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)) {
> > +	if (!REQ_ARGS(3)) {
> >  		p_err("too few parameters for map attach");
> >  		return -EINVAL;
> >  	}
> > @@ -741,10 +743,11 @@ static int do_attach(int argc, char **argv)
> >  		return -EINVAL;
> >  	}
> >  	NEXT_ARG();
> > -
> > -	mapfd = map_parse_fd(&argc, &argv);
> > -	if (mapfd < 0)
> > -		return mapfd;
> > +	if (argc > 0) {
> 
> Flow dissector can't need a map right?  I think explicitly checking for
> the correct number of arguments once attach type is known would be good.
Makes sense. I initially didn't want to depend on the attach_type too
much, but it might be more readable actually. Will chain in v2.

> > +		mapfd = map_parse_fd(&argc, &argv);
> > +		if (mapfd < 0)
> > +			return mapfd;
> > +	}
> >  
> >  	err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
> >  	if (err) {
> > @@ -760,9 +763,10 @@ 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)) {
> > +	if (!REQ_ARGS(3)) {
> >  		p_err("too few parameters for map detach");
> >  		return -EINVAL;
> >  	}
> > @@ -777,10 +781,11 @@ static int do_detach(int argc, char **argv)
> >  		return -EINVAL;
> >  	}
> >  	NEXT_ARG();
> > -
> > -	mapfd = map_parse_fd(&argc, &argv);
> > -	if (mapfd < 0)
> > -		return mapfd;
> > +	if (argc > 0) {
> > +		mapfd = map_parse_fd(&argc, &argv);
> > +		if (mapfd < 0)
> > +			return mapfd;
> > +	}
> >  
> >  	err = bpf_prog_detach2(progfd, mapfd, attach_type);
> >  	if (err) {
> > @@ -792,6 +797,56 @@ static int do_detach(int argc, char **argv)
> >  		jsonw_null(json_wtr);
> >  	return 0;
> >  }
> > +
> > +/* Flow dissector consists of a main program and a jump table for each
> > + * supported protocol. The assumption here is that the first prog is the main
> > + * one and the other progs are used in the tail calls. In this routine we
> > + * build the jump table for the non-main progs.
> > + */
> > +static int build_flow_dissector_jmp_table(struct bpf_object *obj,
> > +					  struct bpf_program *prog,
> > +					  const char *jmp_table_map)
> > +{
> > +	struct bpf_map *jmp_table;
> > +	struct bpf_program *pos;
> > +	int i = 0;
> > +	int prog_fd, jmp_table_fd, fd;
> 
> Please order variables longest to shortest.
> 
> > +	prog_fd = bpf_program__fd(prog);
> > +	if (prog_fd < 0) {
> > +		p_err("failed to get fd of main prog");
> > +		return prog_fd;
> > +	}
> > +
> > +	jmp_table = bpf_object__find_map_by_name(obj, jmp_table_map);
> > +	if (jmp_table == NULL) {
> 
> nit: !jmp_table
> 
> > +		p_err("failed to find '%s' map", jmp_table_map);
> > +		return -1;
> > +	}
> > +
> > +	jmp_table_fd = bpf_map__fd(jmp_table);
> > +	if (jmp_table_fd < 0) {
> > +		p_err("failed to get fd of jmp_table");
> > +		return jmp_table_fd;
> > +	}
> > +
> > +	bpf_object__for_each_program(pos, obj) {
> > +		fd = bpf_program__fd(pos);
> > +		if (fd < 0) {
> > +			p_err("failed to get fd of '%s'",
> > +			      bpf_program__title(pos, false));
> > +			return fd;
> > +		}
> > +
> > +		if (fd != prog_fd) {
> > +			bpf_map_update_elem(jmp_table_fd, &i, &fd, BPF_ANY);
> > +			++i;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int do_load(int argc, char **argv)
> >  {
> >  	enum bpf_attach_type expected_attach_type;
> > @@ -800,7 +855,7 @@ static int do_load(int argc, char **argv)
> >  	};
> >  	struct map_replace *map_replace = NULL;
> >  	unsigned int old_map_fds = 0;
> > -	struct bpf_program *prog;
> > +	struct bpf_program *prog, *pos;
> 
> variable order
> 
> >  	struct bpf_object *obj;
> >  	struct bpf_map *map;
> >  	const char *pinfile;
> > @@ -918,13 +973,19 @@ static int do_load(int argc, char **argv)
> >  		goto err_free_reuse_maps;
> >  	}
> >  
> > -	prog = bpf_program__next(NULL, obj);
> > +	if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> > +		/* for the flow dissector type, the entry point is in the
> > +		 * section flow_dissector; other progs are tail calls
> > +		 */
> > +		prog = bpf_object__find_program_by_title(obj, "flow_dissector");
> > +	} else {
> > +		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) {
> >  		const char *sec_name = bpf_program__title(prog, false);
> >  
> > @@ -936,8 +997,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);
> > +	}
> >  
> >  	qsort(map_replace, old_map_fds, sizeof(*map_replace),
> >  	      map_replace_compar);
> > @@ -1001,8 +1067,34 @@ 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) {
> > +		p_err("failed to mount bpffs for pin '%s'", pinfile);
> 
> Probably would be a duplicated error again?
> 
> >  		goto err_close_obj;
> > +	}
> > +
> > +	if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> > +		err = build_flow_dissector_jmp_table(obj, prog, "jmp_table");
> > +		if (err) {
> > +			p_err("failed to build flow dissector jump table");
> > +			goto err_close_obj;
> > +		}
> > +		/* flow dissector consist of multiple programs,
> > +		 * we want to pin them all
> 
> Why pin them all shouldn't the main program be the only one pinned?
If I pin only the main program, the tail ones disappear from the jmp_table map
when bpftool exits.
Am I missing something?
Should BPF_MAP_TYPE_PROG_ARRAY hold the referenced progs?

> > +		 */
> > +		err = bpf_object__pin(obj, pinfile);
> > +		if (err) {
> > +			p_err("failed to pin flow dissector object");
> > +			goto err_close_obj;
> > +		}
> > +	} else {
> > +		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;
> > +		}
> > +	}
> 

Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector

[Jakub Kicinski]

On Wed, 7 Nov 2018 13:00:09 -0800, Stanislav Fomichev wrote:
> > > +	}
> > > +
> > > +	if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> > > +		err = build_flow_dissector_jmp_table(obj, prog, "jmp_table");
> > > +		if (err) {
> > > +			p_err("failed to build flow dissector jump table");
> > > +			goto err_close_obj;
> > > +		}
> > > +		/* flow dissector consist of multiple programs,
> > > +		 * we want to pin them all  
> > 
> > Why pin them all shouldn't the main program be the only one pinned?  
> If I pin only the main program, the tail ones disappear from the jmp_table map
> when bpftool exits.
> Am I missing something?
> Should BPF_MAP_TYPE_PROG_ARRAY hold the referenced progs?

It does.

# bpftool map create /sys/fs/bpf/map \
	type prog_array key 4 value 4 entries 4 \
	name tail_call_map
# bpftool prog load perf_event_output_stack.o /sys/fs/bpf/prog \
	 type xdp 
# bpftool map update pinned /sys/fs/bpf/map \
	key 0 0 0 0 \
	value pinned /sys/fs/bpf/prog
# bpftool prog 
11: xdp  name xdp_prog1  tag 6f1b482b27443edf  gpl
	loaded_at 2018-11-07T13:09:20-0800  uid 0
	xlated 144B  jited 130B  memlock 4096B  map_ids 14
# rm /sys/fs/bpf/prog
# bpftool prog 
11: xdp  name xdp_prog1  tag 6f1b482b27443edf  gpl
	loaded_at 2018-11-07T13:09:20-0800  uid 0
	xlated 144B  jited 130B  memlock 4096B  map_ids 14
# rm /sys/fs/bpf/map 
# bpftool prog 
# bpftool prog show id 11
Error: get by id (11): No such file or directory


I think we should remove all this auto tail call construction unless
we have solid annotations in the elf file which can clearly guide the
loader.

Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector

[Quentin Monnet]

Hi Stanislav,

2018-11-07 11:35 UTC-0800 ~ Stanislav Fomichev <sdf@google.com>
> 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 and build the jump table.
> 
> 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 prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
> 
> Tested by using the above two lines to load the prog in
> the test_flow_dissector.sh selftest.
> 
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>  .../bpftool/Documentation/bpftool-prog.rst    |  16 ++-
>  tools/bpf/bpftool/common.c                    |  32 +++--
>  tools/bpf/bpftool/main.h                      |   1 +
>  tools/bpf/bpftool/prog.c                      | 135 +++++++++++++++---
>  4 files changed, 141 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index ac4e904b10fb..3caa9153435b 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -25,8 +25,8 @@ MAP COMMANDS
>  |	**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 attach** *PROG* *ATTACH_TYPE* [*MAP*]
> +|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
>  |	**bpftool** **prog help**
>  |
>  |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> @@ -39,7 +39,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**

        ^
Nitpick: Could you please remove the above pipe?

> +|	}
>  
>  
>  DESCRIPTION
> @@ -97,13 +99,13 @@ DESCRIPTION
>  		  contain a dot character ('.'), which is reserved for future
>  		  extensions of *bpffs*.
>  
> -        **bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
> +        **bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
>                    Attach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> -                  to the map *MAP*.
> +                  to the optional map *MAP*.
>  
> -        **bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
> +        **bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
>                    Detach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> -                  from the map *MAP*.
> +                  from the optional map *MAP*.
>  
>  	**bpftool prog help**
>  		  Print short help message.
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 25af85304ebe..963881142dfb 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -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,22 @@ 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 = mount_bpffs_for_pin(name);
> +
> +	if (err) {
> +		p_err("can't mount bpffs for pin %s: %s",
> +		      name, strerror(errno));
> +		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;
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 28322ace2856..1383824c9baf 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -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);
>  
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 5302ee282409..f3a07ec3a444 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -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,9 +725,10 @@ 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)) {
> +	if (!REQ_ARGS(3)) {
>  		p_err("too few parameters for map attach");
>  		return -EINVAL;
>  	}
> @@ -741,10 +743,11 @@ static int do_attach(int argc, char **argv)
>  		return -EINVAL;
>  	}
>  	NEXT_ARG();
> -
> -	mapfd = map_parse_fd(&argc, &argv);
> -	if (mapfd < 0)
> -		return mapfd;
> +	if (argc > 0) {
> +		mapfd = map_parse_fd(&argc, &argv);
> +		if (mapfd < 0)
> +			return mapfd;
> +	}
>  
>  	err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
>  	if (err) {
> @@ -760,9 +763,10 @@ 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)) {
> +	if (!REQ_ARGS(3)) {
>  		p_err("too few parameters for map detach");
>  		return -EINVAL;
>  	}
> @@ -777,10 +781,11 @@ static int do_detach(int argc, char **argv)
>  		return -EINVAL;
>  	}
>  	NEXT_ARG();
> -
> -	mapfd = map_parse_fd(&argc, &argv);
> -	if (mapfd < 0)
> -		return mapfd;
> +	if (argc > 0) {
> +		mapfd = map_parse_fd(&argc, &argv);
> +		if (mapfd < 0)
> +			return mapfd;
> +	}
>  
>  	err = bpf_prog_detach2(progfd, mapfd, attach_type);
>  	if (err) {
> @@ -792,6 +797,56 @@ static int do_detach(int argc, char **argv)
>  		jsonw_null(json_wtr);
>  	return 0;
>  }
> +
> +/* Flow dissector consists of a main program and a jump table for each
> + * supported protocol. The assumption here is that the first prog is the main
> + * one and the other progs are used in the tail calls. In this routine we
> + * build the jump table for the non-main progs.
> + */
> +static int build_flow_dissector_jmp_table(struct bpf_object *obj,
> +					  struct bpf_program *prog,
> +					  const char *jmp_table_map)
> +{
> +	struct bpf_map *jmp_table;
> +	struct bpf_program *pos;
> +	int i = 0;
> +	int prog_fd, jmp_table_fd, fd;
> +
> +	prog_fd = bpf_program__fd(prog);
> +	if (prog_fd < 0) {
> +		p_err("failed to get fd of main prog");
> +		return prog_fd;
> +	}
> +
> +	jmp_table = bpf_object__find_map_by_name(obj, jmp_table_map);
> +	if (jmp_table == NULL) {
> +		p_err("failed to find '%s' map", jmp_table_map);
> +		return -1;
> +	}
> +
> +	jmp_table_fd = bpf_map__fd(jmp_table);
> +	if (jmp_table_fd < 0) {
> +		p_err("failed to get fd of jmp_table");
> +		return jmp_table_fd;
> +	}
> +
> +	bpf_object__for_each_program(pos, obj) {
> +		fd = bpf_program__fd(pos);
> +		if (fd < 0) {
> +			p_err("failed to get fd of '%s'",
> +			      bpf_program__title(pos, false));
> +			return fd;
> +		}
> +
> +		if (fd != prog_fd) {
> +			bpf_map_update_elem(jmp_table_fd, &i, &fd, BPF_ANY);
> +			++i;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int do_load(int argc, char **argv)
>  {
>  	enum bpf_attach_type expected_attach_type;
> @@ -800,7 +855,7 @@ static int do_load(int argc, char **argv)
>  	};
>  	struct map_replace *map_replace = NULL;
>  	unsigned int old_map_fds = 0;
> -	struct bpf_program *prog;
> +	struct bpf_program *prog, *pos;
>  	struct bpf_object *obj;
>  	struct bpf_map *map;
>  	const char *pinfile;
> @@ -918,13 +973,19 @@ static int do_load(int argc, char **argv)
>  		goto err_free_reuse_maps;
>  	}
>  
> -	prog = bpf_program__next(NULL, obj);
> +	if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> +		/* for the flow dissector type, the entry point is in the
> +		 * section flow_dissector; other progs are tail calls
> +		 */
> +		prog = bpf_object__find_program_by_title(obj, "flow_dissector");

Is the section always called like this? Or could we use instead the same
assumption as above, i.e. the main program is the first one?

> +	} else {
> +		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) {
>  		const char *sec_name = bpf_program__title(prog, false);
>  
> @@ -936,8 +997,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);
> +	}

I believe it is possible to load program of several types or attach
types from a same object file? If so, we would need to get individual
prog types and attach types for each program with
libbpf_prog_type_by_name().

>  
>  	qsort(map_replace, old_map_fds, sizeof(*map_replace),
>  	      map_replace_compar);
> @@ -1001,8 +1067,34 @@ 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) {
> +		p_err("failed to mount bpffs for pin '%s'", pinfile);
>  		goto err_close_obj;
> +	}
> +
> +	if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> +		err = build_flow_dissector_jmp_table(obj, prog, "jmp_table");
> +		if (err) {
> +			p_err("failed to build flow dissector jump table");
> +			goto err_close_obj;
> +		}
> +		/* flow dissector consist of multiple programs,
> +		 * we want to pin them all
> +		 */
> +		err = bpf_object__pin(obj, pinfile);
> +		if (err) {
> +			p_err("failed to pin flow dissector object");
> +			goto err_close_obj;
> +		}

What happens for the programs previously loaded and pinned in one of the
program fails? Do they remain loaded and pinned even if all programs
were not successfully loaded? Or should we consider removing all links
we added instead and go back to a "clean" state?

> +	} else {
> +		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;
> +		}

I don't have the same opinion as Jakub for pinning :). I was hoping we
could also load additional programs (for tail calls) for
non-flow_dissector programs. Could this be an occasion to update the
code in that direction?

> +	}
>  
>  	if (json_output)
>  		jsonw_null(json_wtr);
> @@ -1037,8 +1129,8 @@ static int do_help(int argc, char **argv)
>  		"       %s %s pin   PROG FILE\n"
>  		"       %s %s load  OBJ  FILE [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 +1142,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],
> 

Thanks a lot for the set!
Quentin

Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector

[Jakub Kicinski]

On Wed, 7 Nov 2018 20:08:53 +0000, Quentin Monnet wrote:
> > +		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;
> > +		}  
> 
> I don't have the same opinion as Jakub for pinning :). I was hoping we
> could also load additional programs (for tail calls) for
> non-flow_dissector programs. Could this be an occasion to update the
> code in that direction?

Do you mean having the bpftool construct an array for tail calling
automatically when loading an object?  Or do a "mass pin" of all
programs in an object file?

I'm not convinced about this strategy of auto assembling a tail call
array by assuming that a flow dissector object carries programs for
protocols in order (apart from the main program which doesn't have to
be first, for some reason).

Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector

[Quentin Monnet]

2018-11-07 12:32 UTC-0800 ~ Jakub Kicinski <jakub.kicinski@netronome.com>
> On Wed, 7 Nov 2018 20:08:53 +0000, Quentin Monnet wrote:
>>> +		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;
>>> +		}
>>
>> I don't have the same opinion as Jakub for pinning :). I was hoping we
>> could also load additional programs (for tail calls) for
>> non-flow_dissector programs. Could this be an occasion to update the
>> code in that direction?
> 
> Do you mean having the bpftool construct an array for tail calling
> automatically when loading an object?  Or do a "mass pin" of all
> programs in an object file?
> 
> I'm not convinced about this strategy of auto assembling a tail call
> array by assuming that a flow dissector object carries programs for
> protocols in order (apart from the main program which doesn't have to
> be first, for some reason).

Not constructing the prog array, I don't think this should be the role 
of bpftool either. Much more a "mass pin", so that you have a link to 
each program loaded from the object file and can later add them to a 
prog array map with subsequent calls to bpftool.

Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector

[Stanislav Fomichev]

On 11/07, Quentin Monnet wrote:
> 2018-11-07 12:32 UTC-0800 ~ Jakub Kicinski <jakub.kicinski@netronome.com>
> > On Wed, 7 Nov 2018 20:08:53 +0000, Quentin Monnet wrote:
> > > > +		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;
> > > > +		}
> > > 
> > > I don't have the same opinion as Jakub for pinning :). I was hoping we
> > > could also load additional programs (for tail calls) for
> > > non-flow_dissector programs. Could this be an occasion to update the
> > > code in that direction?
> > 
> > Do you mean having the bpftool construct an array for tail calling
> > automatically when loading an object?  Or do a "mass pin" of all
> > programs in an object file?
> > 
> > I'm not convinced about this strategy of auto assembling a tail call
> > array by assuming that a flow dissector object carries programs for
> > protocols in order (apart from the main program which doesn't have to
> > be first, for some reason).
> 
> Not constructing the prog array, I don't think this should be the role of
> bpftool either. Much more a "mass pin", so that you have a link to each
> program loaded from the object file and can later add them to a prog array
> map with subsequent calls to bpftool.
I agree, constructing the jmp_table is a bit fragile with all the
dependencies on the order of the progs. I'll drop that and will send a
v2 that pins all the programs from the obj file instead and offloads
jmp_table construction to the user. So the supposed use case would be
something like the following:

bpftool prog load bpf_flow.o /sys/fs/bpf/flow type flow_dissector
bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
	key ... value pinned /sys/fs/bpf/flow/<PROTO>/0
bpftool map update ...
bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector

Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector

[Jakub Kicinski]

On Wed, 7 Nov 2018 14:15:06 -0800, Stanislav Fomichev wrote:
> On 11/07, Quentin Monnet wrote:
> > 2018-11-07 12:32 UTC-0800 ~ Jakub Kicinski <jakub.kicinski@netronome.com>  
> > > On Wed, 7 Nov 2018 20:08:53 +0000, Quentin Monnet wrote:  
> > > > > +		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;
> > > > > +		}  
> > > > 
> > > > I don't have the same opinion as Jakub for pinning :). I was hoping we
> > > > could also load additional programs (for tail calls) for
> > > > non-flow_dissector programs. Could this be an occasion to update the
> > > > code in that direction?  
> > > 
> > > Do you mean having the bpftool construct an array for tail calling
> > > automatically when loading an object?  Or do a "mass pin" of all
> > > programs in an object file?
> > > 
> > > I'm not convinced about this strategy of auto assembling a tail call
> > > array by assuming that a flow dissector object carries programs for
> > > protocols in order (apart from the main program which doesn't have to
> > > be first, for some reason).  
> > 
> > Not constructing the prog array, I don't think this should be the role of
> > bpftool either. Much more a "mass pin", so that you have a link to each
> > program loaded from the object file and can later add them to a prog array
> > map with subsequent calls to bpftool.  

Makes sense, specific files named after index or program name/title?
Program name may be nicer?

> I agree, constructing the jmp_table is a bit fragile with all the
> dependencies on the order of the progs. I'll drop that and will send a
> v2 that pins all the programs from the obj file instead and offloads
> jmp_table construction to the user. So the supposed use case would be
> something like the following:
> 
> bpftool prog load bpf_flow.o /sys/fs/bpf/flow type flow_dissector

Okay.  One more thing - how do we differentiate between mass pin and the
existing pin first behaviour?  Should we perhaps add a loadall command
or some flag?

> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> 	key ... value pinned /sys/fs/bpf/flow/<PROTO>/0

Interesting, why $dir/$title/0 ?  Perhaps $dir/$title is sufficient?

> bpftool map update ...
> bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector

Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector

[Stanislav Fomichev]

On 11/07, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 14:15:06 -0800, Stanislav Fomichev wrote:
> > On 11/07, Quentin Monnet wrote:
> > > 2018-11-07 12:32 UTC-0800 ~ Jakub Kicinski <jakub.kicinski@netronome.com>  
> > > > On Wed, 7 Nov 2018 20:08:53 +0000, Quentin Monnet wrote:  
> > > > > > +		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;
> > > > > > +		}  
> > > > > 
> > > > > I don't have the same opinion as Jakub for pinning :). I was hoping we
> > > > > could also load additional programs (for tail calls) for
> > > > > non-flow_dissector programs. Could this be an occasion to update the
> > > > > code in that direction?  
> > > > 
> > > > Do you mean having the bpftool construct an array for tail calling
> > > > automatically when loading an object?  Or do a "mass pin" of all
> > > > programs in an object file?
> > > > 
> > > > I'm not convinced about this strategy of auto assembling a tail call
> > > > array by assuming that a flow dissector object carries programs for
> > > > protocols in order (apart from the main program which doesn't have to
> > > > be first, for some reason).  
> > > 
> > > Not constructing the prog array, I don't think this should be the role of
> > > bpftool either. Much more a "mass pin", so that you have a link to each
> > > program loaded from the object file and can later add them to a prog array
> > > map with subsequent calls to bpftool.  
> 
> Makes sense, specific files named after index or program name/title?
> Program name may be nicer?
> 
> > I agree, constructing the jmp_table is a bit fragile with all the
> > dependencies on the order of the progs. I'll drop that and will send a
> > v2 that pins all the programs from the obj file instead and offloads
> > jmp_table construction to the user. So the supposed use case would be
> > something like the following:
> > 
> > bpftool prog load bpf_flow.o /sys/fs/bpf/flow type flow_dissector
> 
> Okay.  One more thing - how do we differentiate between mass pin and the
> existing pin first behaviour?  Should we perhaps add a loadall command
> or some flag?
In v2 I did by program type:
* flow_dissector -> pin all
* not flow_dissector -> pin first?

But we can have loadall or something like:
load OBJ [pinfirst|pinall] FILE|DIR [type TYPE]

If we want to add user control, I'd go with loadall command,
adding more optional flags in between is a mess..

> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > 	key ... value pinned /sys/fs/bpf/flow/<PROTO>/0
> 
> Interesting, why $dir/$title/0 ?  Perhaps $dir/$title is sufficient?
That's how bpf_program__pin does the pinning, for each program instance.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n1744

> 
> > bpftool map update ...
> > bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
> 

Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector

[Jakub Kicinski]

On Wed, 7 Nov 2018 15:12:07 -0800, Stanislav Fomichev wrote:
> > > I agree, constructing the jmp_table is a bit fragile with all the
> > > dependencies on the order of the progs. I'll drop that and will send a
> > > v2 that pins all the programs from the obj file instead and offloads
> > > jmp_table construction to the user. So the supposed use case would be
> > > something like the following:
> > > 
> > > bpftool prog load bpf_flow.o /sys/fs/bpf/flow type flow_dissector  
> > 
> > Okay.  One more thing - how do we differentiate between mass pin and the
> > existing pin first behaviour?  Should we perhaps add a loadall command
> > or some flag?  
> In v2 I did by program type:
> * flow_dissector -> pin all
> * not flow_dissector -> pin first?
> 
> But we can have loadall or something like:
> load OBJ [pinfirst|pinall] FILE|DIR [type TYPE]
> 
> If we want to add user control, I'd go with loadall command,
> adding more optional flags in between is a mess..

I think user control would be good.  Agreed on loadall being better.

Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector

[Stanislav Fomichev]

On 11/07, Quentin Monnet wrote:
> Hi Stanislav,
> 
> 2018-11-07 11:35 UTC-0800 ~ Stanislav Fomichev <sdf@google.com>
> > 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 and build the jump table.
> > 
> > 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 prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
> > 
> > Tested by using the above two lines to load the prog in
> > the test_flow_dissector.sh selftest.
> > 
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >  .../bpftool/Documentation/bpftool-prog.rst    |  16 ++-
> >  tools/bpf/bpftool/common.c                    |  32 +++--
> >  tools/bpf/bpftool/main.h                      |   1 +
> >  tools/bpf/bpftool/prog.c                      | 135 +++++++++++++++---
> >  4 files changed, 141 insertions(+), 43 deletions(-)
> > 
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > index ac4e904b10fb..3caa9153435b 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > @@ -25,8 +25,8 @@ MAP COMMANDS
> >  |	**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 attach** *PROG* *ATTACH_TYPE* [*MAP*]
> > +|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> >  |	**bpftool** **prog help**
> >  |
> >  |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> > @@ -39,7 +39,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**
> 
>         ^
> Nitpick: Could you please remove the above pipe?
Ah, I missed that one, my bad, thanks.

> > +|	}
> >  
> >  
> >  DESCRIPTION
> > @@ -97,13 +99,13 @@ DESCRIPTION
> >  		  contain a dot character ('.'), which is reserved for future
> >  		  extensions of *bpffs*.
> >  
> > -        **bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
> > +        **bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> >                    Attach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> > -                  to the map *MAP*.
> > +                  to the optional map *MAP*.
> >  
> > -        **bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
> > +        **bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> >                    Detach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> > -                  from the map *MAP*.
> > +                  from the optional map *MAP*.
> >  
> >  	**bpftool prog help**
> >  		  Print short help message.
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index 25af85304ebe..963881142dfb 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
> > @@ -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,22 @@ 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 = mount_bpffs_for_pin(name);
> > +
> > +	if (err) {
> > +		p_err("can't mount bpffs for pin %s: %s",
> > +		      name, strerror(errno));
> > +		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;
> > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> > index 28322ace2856..1383824c9baf 100644
> > --- a/tools/bpf/bpftool/main.h
> > +++ b/tools/bpf/bpftool/main.h
> > @@ -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);
> >  
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index 5302ee282409..f3a07ec3a444 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -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,9 +725,10 @@ 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)) {
> > +	if (!REQ_ARGS(3)) {
> >  		p_err("too few parameters for map attach");
> >  		return -EINVAL;
> >  	}
> > @@ -741,10 +743,11 @@ static int do_attach(int argc, char **argv)
> >  		return -EINVAL;
> >  	}
> >  	NEXT_ARG();
> > -
> > -	mapfd = map_parse_fd(&argc, &argv);
> > -	if (mapfd < 0)
> > -		return mapfd;
> > +	if (argc > 0) {
> > +		mapfd = map_parse_fd(&argc, &argv);
> > +		if (mapfd < 0)
> > +			return mapfd;
> > +	}
> >  
> >  	err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
> >  	if (err) {
> > @@ -760,9 +763,10 @@ 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)) {
> > +	if (!REQ_ARGS(3)) {
> >  		p_err("too few parameters for map detach");
> >  		return -EINVAL;
> >  	}
> > @@ -777,10 +781,11 @@ static int do_detach(int argc, char **argv)
> >  		return -EINVAL;
> >  	}
> >  	NEXT_ARG();
> > -
> > -	mapfd = map_parse_fd(&argc, &argv);
> > -	if (mapfd < 0)
> > -		return mapfd;
> > +	if (argc > 0) {
> > +		mapfd = map_parse_fd(&argc, &argv);
> > +		if (mapfd < 0)
> > +			return mapfd;
> > +	}
> >  
> >  	err = bpf_prog_detach2(progfd, mapfd, attach_type);
> >  	if (err) {
> > @@ -792,6 +797,56 @@ static int do_detach(int argc, char **argv)
> >  		jsonw_null(json_wtr);
> >  	return 0;
> >  }
> > +
> > +/* Flow dissector consists of a main program and a jump table for each
> > + * supported protocol. The assumption here is that the first prog is the main
> > + * one and the other progs are used in the tail calls. In this routine we
> > + * build the jump table for the non-main progs.
> > + */
> > +static int build_flow_dissector_jmp_table(struct bpf_object *obj,
> > +					  struct bpf_program *prog,
> > +					  const char *jmp_table_map)
> > +{
> > +	struct bpf_map *jmp_table;
> > +	struct bpf_program *pos;
> > +	int i = 0;
> > +	int prog_fd, jmp_table_fd, fd;
> > +
> > +	prog_fd = bpf_program__fd(prog);
> > +	if (prog_fd < 0) {
> > +		p_err("failed to get fd of main prog");
> > +		return prog_fd;
> > +	}
> > +
> > +	jmp_table = bpf_object__find_map_by_name(obj, jmp_table_map);
> > +	if (jmp_table == NULL) {
> > +		p_err("failed to find '%s' map", jmp_table_map);
> > +		return -1;
> > +	}
> > +
> > +	jmp_table_fd = bpf_map__fd(jmp_table);
> > +	if (jmp_table_fd < 0) {
> > +		p_err("failed to get fd of jmp_table");
> > +		return jmp_table_fd;
> > +	}
> > +
> > +	bpf_object__for_each_program(pos, obj) {
> > +		fd = bpf_program__fd(pos);
> > +		if (fd < 0) {
> > +			p_err("failed to get fd of '%s'",
> > +			      bpf_program__title(pos, false));
> > +			return fd;
> > +		}
> > +
> > +		if (fd != prog_fd) {
> > +			bpf_map_update_elem(jmp_table_fd, &i, &fd, BPF_ANY);
> > +			++i;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int do_load(int argc, char **argv)
> >  {
> >  	enum bpf_attach_type expected_attach_type;
> > @@ -800,7 +855,7 @@ static int do_load(int argc, char **argv)
> >  	};
> >  	struct map_replace *map_replace = NULL;
> >  	unsigned int old_map_fds = 0;
> > -	struct bpf_program *prog;
> > +	struct bpf_program *prog, *pos;
> >  	struct bpf_object *obj;
> >  	struct bpf_map *map;
> >  	const char *pinfile;
> > @@ -918,13 +973,19 @@ static int do_load(int argc, char **argv)
> >  		goto err_free_reuse_maps;
> >  	}
> >  
> > -	prog = bpf_program__next(NULL, obj);
> > +	if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> > +		/* for the flow dissector type, the entry point is in the
> > +		 * section flow_dissector; other progs are tail calls
> > +		 */
> > +		prog = bpf_object__find_program_by_title(obj, "flow_dissector");
> 
> Is the section always called like this? Or could we use instead the same
> assumption as above, i.e. the main program is the first one?
Ideally yes, the main section should always be called
flow_dissector (to not break libbpf_prog_type_by_name assumptions).

We can call bpftool with and without the ATTACH_TYPE argument:

1. when 'type flow_dissector' is specified, we try to find section called
  'flow_dissector' and assume, that's the main prog (libbpf_prog_type_by_name)
2. when 'type flow_dissector' is _not_ specified, we try to detect
  program type from the first section

This check here is for the cases where first section is not
flow_dissector (but some tail call) and we want to force proper type.


> > +	} else {
> > +		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) {
> >  		const char *sec_name = bpf_program__title(prog, false);
> >  
> > @@ -936,8 +997,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);
> > +	}
> 
> I believe it is possible to load program of several types or attach
> types from a same object file? If so, we would need to get individual
> prog types and attach types for each program with
> libbpf_prog_type_by_name().
I don't think it works actually. I also assumed that initially, but as
you can see from the patch, I had to loop over the programs and set
correct type. Current implementation will fail if you have two progs
in the same obj AFAICT.

> >  	qsort(map_replace, old_map_fds, sizeof(*map_replace),
> >  	      map_replace_compar);
> > @@ -1001,8 +1067,34 @@ 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) {
> > +		p_err("failed to mount bpffs for pin '%s'", pinfile);
> >  		goto err_close_obj;
> > +	}
> > +
> > +	if (attr.prog_type == BPF_PROG_TYPE_FLOW_DISSECTOR) {
> > +		err = build_flow_dissector_jmp_table(obj, prog, "jmp_table");
> > +		if (err) {
> > +			p_err("failed to build flow dissector jump table");
> > +			goto err_close_obj;
> > +		}
> > +		/* flow dissector consist of multiple programs,
> > +		 * we want to pin them all
> > +		 */
> > +		err = bpf_object__pin(obj, pinfile);
> > +		if (err) {
> > +			p_err("failed to pin flow dissector object");
> > +			goto err_close_obj;
> > +		}
> 
> What happens for the programs previously loaded and pinned in one of the
> program fails? Do they remain loaded and pinned even if all programs
> were not successfully loaded? Or should we consider removing all links
> we added instead and go back to a "clean" state?
They remain loaded. I agree, we should go back to the clean state;
that probably should be achieved by adding proper cleanup to libbpf's
bpf_object__pin when it fails. I can look into that for v2.

> > +	} else {
> > +		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;
> > +		}
> 
> I don't have the same opinion as Jakub for pinning :). I was hoping we
> could also load additional programs (for tail calls) for
> non-flow_dissector programs. Could this be an occasion to update the
> code in that direction?
(See above for loading multiple progs from the same object).

If we want to be able to load non-flow_dissector programs from the same file,
we should have some way to distinguish between flow_dissector and
non-flow_dissector ones; I don't see an easy way to do that.

> > +	}
> >  
> >  	if (json_output)
> >  		jsonw_null(json_wtr);
> > @@ -1037,8 +1129,8 @@ static int do_help(int argc, char **argv)
> >  		"       %s %s pin   PROG FILE\n"
> >  		"       %s %s load  OBJ  FILE [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 +1142,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],
> > 
> 
> Thanks a lot for the set!
Thank you for a review as well!
> Quentin