[PATCH v2 net-next 0/4] bpftool: cgroup bpf operations

[PATCH v2 net-next 0/4] bpftool: cgroup bpf operations

[Roman Gushchin]

This patchset adds basic cgroup bpf operations to bpftool.

Right now there is no convenient way to perform these operations.
The /samples/bpf/load_sock_ops.c implements attach/detacg operations,
but only for BPF_CGROUP_SOCK_OPS programs. Bps (part of bcc) implements
bpf introspection, but lacks any cgroup-related specific.

I find having a tool to perform these basic operations in the kernel tree
very useful, as it can be used in the corresponding bpf documentation
without creating additional dependencies. And bpftool seems to be
a right tool to extend with such functionality.

v2:
  - Added prog load operations
  - All cgroup operations are looking like bpftool cgroup <command>
  - All cgroup-related stuff is moved to a separate file
  - Added support for attach flags
  - Added support for attaching/detaching programs by id, pinned name, etc
  - Changed cgroup detach arguments order
  - Added empty json output for succesful programs
  - Style fixed: includes order, strncmp and macroses, error handling
  - Added man pages

v1:
  https://lwn.net/Articles/740366/

Roman Gushchin (4):
  libbpf: add ability to guess program type based on section name
  libbpf: prefer global symbols as bpf program name source
  bpftool: implement prog load command
  bpftool: implement cgroup bpf operations

 tools/bpf/bpftool/Documentation/bpftool-cgroup.rst |  92 +++++++
 tools/bpf/bpftool/Documentation/bpftool-map.rst    |   2 +-
 tools/bpf/bpftool/Documentation/bpftool-prog.rst   |  12 +-
 tools/bpf/bpftool/Documentation/bpftool.rst        |   8 +-
 tools/bpf/bpftool/cgroup.c                         | 305 +++++++++++++++++++++
 tools/bpf/bpftool/common.c                         |  71 ++---
 tools/bpf/bpftool/main.c                           |   3 +-
 tools/bpf/bpftool/main.h                           |   2 +
 tools/bpf/bpftool/prog.c                           |  31 ++-
 tools/lib/bpf/libbpf.c                             |  53 ++++
 10 files changed, 540 insertions(+), 39 deletions(-)
 create mode 100644 tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
 create mode 100644 tools/bpf/bpftool/cgroup.c

-- 
2.14.3

[PATCH v2 net-next 1/4] libbpf: add ability to guess program type based on section name

[Roman Gushchin]

The bpf_prog_load() function will guess program type if it's not
specified explicitly. This functionality will be used to implement
loading of different programs without asking a user to specify
the program type. In first order it will be used by bpftool.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Quentin Monnet <quentin.monnet@netronome.com>
Cc: David Ahern <dsahern@gmail.com>
---
 tools/lib/bpf/libbpf.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 5aa45f89da93..205b7822fa0a 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1721,6 +1721,45 @@ BPF_PROG_TYPE_FNS(tracepoint, BPF_PROG_TYPE_TRACEPOINT);
 BPF_PROG_TYPE_FNS(xdp, BPF_PROG_TYPE_XDP);
 BPF_PROG_TYPE_FNS(perf_event, BPF_PROG_TYPE_PERF_EVENT);
 
+#define BPF_PROG_SEC(string, type) { string, sizeof(string), type }
+static const struct {
+	const char *sec;
+	size_t len;
+	enum bpf_prog_type prog_type;
+} section_names[] = {
+	BPF_PROG_SEC("socket",		BPF_PROG_TYPE_SOCKET_FILTER),
+	BPF_PROG_SEC("kprobe/",		BPF_PROG_TYPE_KPROBE),
+	BPF_PROG_SEC("kretprobe/",	BPF_PROG_TYPE_KPROBE),
+	BPF_PROG_SEC("tracepoint/",	BPF_PROG_TYPE_TRACEPOINT),
+	BPF_PROG_SEC("xdp",		BPF_PROG_TYPE_XDP),
+	BPF_PROG_SEC("perf_event",	BPF_PROG_TYPE_PERF_EVENT),
+	BPF_PROG_SEC("cgroup/skb",	BPF_PROG_TYPE_CGROUP_SKB),
+	BPF_PROG_SEC("cgroup/sock",	BPF_PROG_TYPE_CGROUP_SOCK),
+	BPF_PROG_SEC("cgroup/dev",	BPF_PROG_TYPE_CGROUP_DEVICE),
+	BPF_PROG_SEC("sockops",		BPF_PROG_TYPE_SOCK_OPS),
+	BPF_PROG_SEC("sk_skb",		BPF_PROG_TYPE_SK_SKB),
+};
+#undef BPF_PROG_SEC
+
+static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
+{
+	int i;
+
+	if (!prog->section_name)
+		goto err;
+
+	for (i = 0; i < ARRAY_SIZE(section_names); i++)
+		if (strncmp(prog->section_name, section_names[i].sec,
+			    section_names[i].len) == 0)
+			return section_names[i].prog_type;
+
+err:
+	pr_warning("failed to guess program type based on section name %s\n",
+		   prog->section_name);
+
+	return BPF_PROG_TYPE_UNSPEC;
+}
+
 int bpf_map__fd(struct bpf_map *map)
 {
 	return map ? map->fd : -EINVAL;
@@ -1832,6 +1871,18 @@ int bpf_prog_load(const char *file, enum bpf_prog_type type,
 		return -ENOENT;
 	}
 
+	/*
+	 * If type is not specified, try to guess it based on
+	 * section name.
+	 */
+	if (type == BPF_PROG_TYPE_UNSPEC) {
+		type = bpf_program__guess_type(prog);
+		if (type == BPF_PROG_TYPE_UNSPEC) {
+			bpf_object__close(obj);
+			return -EINVAL;
+		}
+	}
+
 	bpf_program__set_type(prog, type);
 	err = bpf_object__load(obj);
 	if (err) {
-- 
2.14.3

Re: [PATCH v2 net-next 1/4] libbpf: add ability to guess program type based on section name

[Quentin Monnet]

2017-12-07 18:39 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> The bpf_prog_load() function will guess program type if it's not
> specified explicitly. This functionality will be used to implement
> loading of different programs without asking a user to specify
> the program type. In first order it will be used by bpftool.
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Quentin Monnet <quentin.monnet@netronome.com>
> Cc: David Ahern <dsahern@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 5aa45f89da93..205b7822fa0a 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1721,6 +1721,45 @@ BPF_PROG_TYPE_FNS(tracepoint, BPF_PROG_TYPE_TRACEPOINT);
>  BPF_PROG_TYPE_FNS(xdp, BPF_PROG_TYPE_XDP);
>  BPF_PROG_TYPE_FNS(perf_event, BPF_PROG_TYPE_PERF_EVENT);
>  
> +#define BPF_PROG_SEC(string, type) { string, sizeof(string), type }
> +static const struct {
> +	const char *sec;
> +	size_t len;
> +	enum bpf_prog_type prog_type;
> +} section_names[] = {
> +	BPF_PROG_SEC("socket",		BPF_PROG_TYPE_SOCKET_FILTER),
> +	BPF_PROG_SEC("kprobe/",		BPF_PROG_TYPE_KPROBE),
> +	BPF_PROG_SEC("kretprobe/",	BPF_PROG_TYPE_KPROBE),
> +	BPF_PROG_SEC("tracepoint/",	BPF_PROG_TYPE_TRACEPOINT),
> +	BPF_PROG_SEC("xdp",		BPF_PROG_TYPE_XDP),
> +	BPF_PROG_SEC("perf_event",	BPF_PROG_TYPE_PERF_EVENT),
> +	BPF_PROG_SEC("cgroup/skb",	BPF_PROG_TYPE_CGROUP_SKB),
> +	BPF_PROG_SEC("cgroup/sock",	BPF_PROG_TYPE_CGROUP_SOCK),
> +	BPF_PROG_SEC("cgroup/dev",	BPF_PROG_TYPE_CGROUP_DEVICE),
> +	BPF_PROG_SEC("sockops",		BPF_PROG_TYPE_SOCK_OPS),
> +	BPF_PROG_SEC("sk_skb",		BPF_PROG_TYPE_SK_SKB),
> +};
> +#undef BPF_PROG_SEC
> +
> +static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
> +{
> +	int i;
> +
> +	if (!prog->section_name)
> +		goto err;
> +
> +	for (i = 0; i < ARRAY_SIZE(section_names); i++)
> +		if (strncmp(prog->section_name, section_names[i].sec,
> +			    section_names[i].len) == 0)
> +			return section_names[i].prog_type;
> +
> +err:
> +	pr_warning("failed to guess program type based on section name %s\n",
> +		   prog->section_name);
> +
> +	return BPF_PROG_TYPE_UNSPEC;
> +}
> +
>  int bpf_map__fd(struct bpf_map *map)
>  {
>  	return map ? map->fd : -EINVAL;
> @@ -1832,6 +1871,18 @@ int bpf_prog_load(const char *file, enum bpf_prog_type type,
>  		return -ENOENT;
>  	}
>  
> +	/*
> +	 * If type is not specified, try to guess it based on
> +	 * section name.
> +	 */
> +	if (type == BPF_PROG_TYPE_UNSPEC) {
> +		type = bpf_program__guess_type(prog);
> +		if (type == BPF_PROG_TYPE_UNSPEC) {
> +			bpf_object__close(obj);
> +			return -EINVAL;
> +		}
> +	}
> +
>  	bpf_program__set_type(prog, type);
>  	err = bpf_object__load(obj);
>  	if (err) {
> 

Looks good to me, nice to avoid the hard-coded string lengths :). Thanks
for the changes!

Quentin

[PATCH v2 net-next 2/4] libbpf: prefer global symbols as bpf program name source

[Roman Gushchin]

Libbpf picks the name of the first symbol in the corresponding
elf section to use as a program name. But without taking symbol's
scope into account it may end's up with some local label
as a program name. E.g.:

$ bpftool prog
1: type 15  name LBB0_10    tag 0390a5136ba23f5c
	loaded_at Dec 07/17:22  uid 0
	xlated 456B  not jited  memlock 4096B

Fix this by preferring global symbols as program name.

For instance:
$ bpftool prog
1: type 15  name bpf_prog1  tag 0390a5136ba23f5c
	loaded_at Dec 07/17:26  uid 0
	xlated 456B  not jited  memlock 4096B

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Quentin Monnet <quentin.monnet@netronome.com>
Cc: David Ahern <dsahern@gmail.com>
---
 tools/lib/bpf/libbpf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 205b7822fa0a..65d0d0aff4fa 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -387,6 +387,8 @@ bpf_object__init_prog_names(struct bpf_object *obj)
 				continue;
 			if (sym.st_shndx != prog->idx)
 				continue;
+			if (GELF_ST_BIND(sym.st_info) != STB_GLOBAL)
+				continue;
 
 			name = elf_strptr(obj->efile.elf,
 					  obj->efile.strtabidx,
-- 
2.14.3

[PATCH v2 net-next 3/4] bpftool: implement prog load command

[Roman Gushchin]

Add the prog load command to load a bpf program from a specified
binary file and pin it to bpffs.

Usage description and examples are given in the corresponding man
page.

Syntax:
$ bpftool prog load SOURCE_FILE FILE

FILE is a non-existing file on bpffs.

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Quentin Monnet <quentin.monnet@netronome.com>
Cc: David Ahern <dsahern@gmail.com>
---
 tools/bpf/bpftool/Documentation/bpftool-prog.rst | 10 +++-
 tools/bpf/bpftool/Documentation/bpftool.rst      |  2 +-
 tools/bpf/bpftool/common.c                       | 71 +++++++++++++-----------
 tools/bpf/bpftool/main.h                         |  1 +
 tools/bpf/bpftool/prog.c                         | 31 ++++++++++-
 5 files changed, 81 insertions(+), 34 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 36e8d1c3c40d..827b415f8ab6 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -15,7 +15,7 @@ SYNOPSIS
 	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
 
 	*COMMANDS* :=
-	{ **show** | **dump xlated** | **dump jited** | **pin** | **help** }
+	{ **show** | **dump xlated** | **dump jited** | **pin** | **load** | **help** }
 
 MAP COMMANDS
 =============
@@ -24,6 +24,7 @@ MAP COMMANDS
 |	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes**}]
 |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes**}]
 |	**bpftool** **prog pin** *PROG* *FILE*
+|	**bpftool** **prog load** *SRC* *FILE*
 |	**bpftool** **prog help**
 |
 |	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
@@ -57,6 +58,11 @@ DESCRIPTION
 
 		  Note: *FILE* must be located in *bpffs* mount.
 
+	**bpftool prog load** *SRC* *FILE*
+		  Load bpf program from binary *SRC* and pin as *FILE*.
+
+		  Note: *FILE* must be located in *bpffs* mount.
+
 	**bpftool prog help**
 		  Print short help message.
 
@@ -126,8 +132,10 @@ EXAMPLES
 |
 | **# mount -t bpf none /sys/fs/bpf/**
 | **# bpftool prog pin id 10 /sys/fs/bpf/prog**
+| **# bpftool prog load ./my_prog.o /sys/fs/bpf/prog2**
 | **# ls -l /sys/fs/bpf/**
 |   -rw------- 1 root root 0 Jul 22 01:43 prog
+|   -rw------- 1 root root 0 Dec 07 17:23 prog2
 
 **# bpftool prog dum jited pinned /sys/fs/bpf/prog opcodes**
 
diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
index 926c03d5a8da..f547a0c0aa34 100644
--- a/tools/bpf/bpftool/Documentation/bpftool.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool.rst
@@ -26,7 +26,7 @@ SYNOPSIS
 	| **pin** | **help** }
 
 	*PROG-COMMANDS* := { **show** | **dump jited** | **dump xlated** | **pin**
-	| **help** }
+	| **load** | **help** }
 
 DESCRIPTION
 ===========
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index 2bd3b280e6dd..b62c94e3997a 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -163,13 +163,49 @@ int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type)
 	return fd;
 }
 
-int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32))
+int do_pin_fd(int fd, const char *name)
 {
 	char err_str[ERR_MAX_LEN];
-	unsigned int id;
-	char *endptr;
 	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));
+		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 {
+		err_str[ERR_MAX_LEN - 1] = '\0';
+		p_err("can't mount BPF file system to pin the object (%s): %s",
+		      name, err_str);
+	}
+
+out_free:
+	free(file);
+out:
+	return err;
+}
+
+int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32))
+{
+	unsigned int id;
+	char *endptr;
 	int err;
 	int fd;
 
@@ -195,35 +231,8 @@ int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32))
 		return -1;
 	}
 
-	err = bpf_obj_pin(fd, *argv);
-	if (!err)
-		goto out_close;
-
-	file = malloc(strlen(*argv) + 1);
-	strcpy(file, *argv);
-	dir = dirname(file);
-
-	if (errno != EPERM || is_bpffs(dir)) {
-		p_err("can't pin the object (%s): %s", *argv, strerror(errno));
-		goto out_free;
-	}
+	err = do_pin_fd(fd, *argv);
 
-	/* Attempt to mount bpffs, then retry pinning. */
-	err = mnt_bpffs(dir, err_str, ERR_MAX_LEN);
-	if (!err) {
-		err = bpf_obj_pin(fd, *argv);
-		if (err)
-			p_err("can't pin the object (%s): %s", *argv,
-			      strerror(errno));
-	} else {
-		err_str[ERR_MAX_LEN - 1] = '\0';
-		p_err("can't mount BPF file system to pin the object (%s): %s",
-		      *argv, err_str);
-	}
-
-out_free:
-	free(file);
-out_close:
 	close(fd);
 	return err;
 }
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index bff330b49791..bec1ccbb49c7 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -111,6 +111,7 @@ 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 do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32));
+int do_pin_fd(int fd, const char *name);
 
 int do_prog(int argc, char **arg);
 int do_map(int argc, char **arg);
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index ad619b96c276..bac5d81e2ff0 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -45,6 +45,7 @@
 #include <sys/stat.h>
 
 #include <bpf.h>
+#include <libbpf.h>
 
 #include "main.h"
 #include "disasm.h"
@@ -635,6 +636,32 @@ static int do_pin(int argc, char **argv)
 	return err;
 }
 
+static int do_load(int argc, char **argv)
+{
+	struct bpf_object *obj;
+	int prog_fd;
+
+	if (argc != 2) {
+		usage();
+		return -1;
+	}
+
+	if (bpf_prog_load(argv[0], BPF_PROG_TYPE_UNSPEC, &obj, &prog_fd)) {
+		p_err("failed to load program\n");
+		return -1;
+	}
+
+	if (do_pin_fd(prog_fd, argv[1])) {
+		p_err("failed to pin program\n");
+		return -1;
+	}
+
+	if (json_output)
+		jsonw_null(json_wtr);
+
+	return 0;
+}
+
 static int do_help(int argc, char **argv)
 {
 	if (json_output) {
@@ -647,13 +674,14 @@ static int do_help(int argc, char **argv)
 		"       %s %s dump xlated PROG [{ file FILE | opcodes }]\n"
 		"       %s %s dump jited  PROG [{ file FILE | opcodes }]\n"
 		"       %s %s pin   PROG FILE\n"
+		"       %s %s load  SRC  FILE\n"
 		"       %s %s help\n"
 		"\n"
 		"       " HELP_SPEC_PROGRAM "\n"
 		"       " HELP_SPEC_OPTIONS "\n"
 		"",
 		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
-		bin_name, argv[-2], bin_name, argv[-2]);
+		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
 
 	return 0;
 }
@@ -663,6 +691,7 @@ static const struct cmd cmds[] = {
 	{ "help",	do_help },
 	{ "dump",	do_dump },
 	{ "pin",	do_pin },
+	{ "load",	do_load },
 	{ 0 }
 };
 
-- 
2.14.3

Re: [PATCH v2 net-next 3/4] bpftool: implement prog load command

[Jakub Kicinski]

On Thu, 7 Dec 2017 18:39:08 +0000, Roman Gushchin wrote:
> Add the prog load command to load a bpf program from a specified
> binary file and pin it to bpffs.
> 
> Usage description and examples are given in the corresponding man
> page.
> 
> Syntax:
> $ bpftool prog load SOURCE_FILE FILE
> 
> FILE is a non-existing file on bpffs.
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>

Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

The only reservation I have is that SRC may confuse people, would it
make sense to call the compiled code OBJ?  I'm afraid someone may try
to load a C source...

Re: [PATCH v2 net-next 3/4] bpftool: implement prog load command

[Quentin Monnet]

2017-12-07 18:39 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> Add the prog load command to load a bpf program from a specified
> binary file and pin it to bpffs.
> 
> Usage description and examples are given in the corresponding man
> page.
> 
> Syntax:
> $ bpftool prog load SOURCE_FILE FILE
> 
> FILE is a non-existing file on bpffs.
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Quentin Monnet <quentin.monnet@netronome.com>
> Cc: David Ahern <dsahern@gmail.com>
> ---
>  tools/bpf/bpftool/Documentation/bpftool-prog.rst | 10 +++-
>  tools/bpf/bpftool/Documentation/bpftool.rst      |  2 +-
>  tools/bpf/bpftool/common.c                       | 71 +++++++++++++-----------
>  tools/bpf/bpftool/main.h                         |  1 +
>  tools/bpf/bpftool/prog.c                         | 31 ++++++++++-
>  5 files changed, 81 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index 36e8d1c3c40d..827b415f8ab6 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -15,7 +15,7 @@ SYNOPSIS
>  	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
>  
>  	*COMMANDS* :=
> -	{ **show** | **dump xlated** | **dump jited** | **pin** | **help** }
> +	{ **show** | **dump xlated** | **dump jited** | **pin** | **load** | **help** }
>  
>  MAP COMMANDS
>  =============
> @@ -24,6 +24,7 @@ MAP COMMANDS
>  |	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes**}]
>  |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes**}]
>  |	**bpftool** **prog pin** *PROG* *FILE*
> +|	**bpftool** **prog load** *SRC* *FILE*
>  |	**bpftool** **prog help**
>  |
>  |	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
> @@ -57,6 +58,11 @@ DESCRIPTION
>  
>  		  Note: *FILE* must be located in *bpffs* mount.
>  
> +	**bpftool prog load** *SRC* *FILE*
> +		  Load bpf program from binary *SRC* and pin as *FILE*.
> +
> +		  Note: *FILE* must be located in *bpffs* mount.
> +
>  	**bpftool prog help**
>  		  Print short help message.
>  
> @@ -126,8 +132,10 @@ EXAMPLES
>  |
>  | **# mount -t bpf none /sys/fs/bpf/**
>  | **# bpftool prog pin id 10 /sys/fs/bpf/prog**
> +| **# bpftool prog load ./my_prog.o /sys/fs/bpf/prog2**
>  | **# ls -l /sys/fs/bpf/**
>  |   -rw------- 1 root root 0 Jul 22 01:43 prog
> +|   -rw------- 1 root root 0 Dec 07 17:23 prog2

Nit: would you mind using a date closer to the first one? Just from a
reader's point of view it might look strange to see the files apparently
created with two successive commands having such a difference between
their creation times.

>  
>  **# bpftool prog dum jited pinned /sys/fs/bpf/prog opcodes**
>  
> diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
> index 926c03d5a8da..f547a0c0aa34 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool.rst
> @@ -26,7 +26,7 @@ SYNOPSIS
>  	| **pin** | **help** }
>  
>  	*PROG-COMMANDS* := { **show** | **dump jited** | **dump xlated** | **pin**
> -	| **help** }
> +	| **load** | **help** }
>  
>  DESCRIPTION
>  ===========

[…]

> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index ad619b96c276..bac5d81e2ff0 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -45,6 +45,7 @@
>  #include <sys/stat.h>
>  
>  #include <bpf.h>
> +#include <libbpf.h>
>  
>  #include "main.h"
>  #include "disasm.h"
> @@ -635,6 +636,32 @@ static int do_pin(int argc, char **argv)
>  	return err;
>  }
>  
> +static int do_load(int argc, char **argv)
> +{
> +	struct bpf_object *obj;
> +	int prog_fd;
> +
> +	if (argc != 2) {
> +		usage();
> +		return -1;
> +	}

No need to return here, usage() exits from the program.

> +
> +	if (bpf_prog_load(argv[0], BPF_PROG_TYPE_UNSPEC, &obj, &prog_fd)) {
> +		p_err("failed to load program\n");
> +		return -1;
> +	}
> +
> +	if (do_pin_fd(prog_fd, argv[1])) {
> +		p_err("failed to pin program\n");
> +		return -1;
> +	}
> +
> +	if (json_output)
> +		jsonw_null(json_wtr);
> +
> +	return 0;
> +}
> +
>  static int do_help(int argc, char **argv)
>  {
>  	if (json_output) {
> @@ -647,13 +674,14 @@ static int do_help(int argc, char **argv)
>  		"       %s %s dump xlated PROG [{ file FILE | opcodes }]\n"
>  		"       %s %s dump jited  PROG [{ file FILE | opcodes }]\n"
>  		"       %s %s pin   PROG FILE\n"
> +		"       %s %s load  SRC  FILE\n"
>  		"       %s %s help\n"
>  		"\n"
>  		"       " HELP_SPEC_PROGRAM "\n"
>  		"       " HELP_SPEC_OPTIONS "\n"
>  		"",
>  		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
> -		bin_name, argv[-2], bin_name, argv[-2]);
> +		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
>  
>  	return 0;
>  }
> @@ -663,6 +691,7 @@ static const struct cmd cmds[] = {
>  	{ "help",	do_help },
>  	{ "dump",	do_dump },
>  	{ "pin",	do_pin },
> +	{ "load",	do_load },
>  	{ 0 }
>  };
>  
> 

[PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Roman Gushchin]

This patch adds basic cgroup bpf operations to bpftool:
cgroup list, attach and detach commands.

Usage is described in the corresponding man pages,
and examples are provided.

Syntax:
$ bpftool cgroup list CGROUP
$ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]
$ bpftool cgroup detach CGROUP ATTACH_TYPE PROG

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Quentin Monnet <quentin.monnet@netronome.com>
Cc: David Ahern <dsahern@gmail.com>
---
 tools/bpf/bpftool/Documentation/bpftool-cgroup.rst |  92 +++++++
 tools/bpf/bpftool/Documentation/bpftool-map.rst    |   2 +-
 tools/bpf/bpftool/Documentation/bpftool-prog.rst   |   2 +-
 tools/bpf/bpftool/Documentation/bpftool.rst        |   6 +-
 tools/bpf/bpftool/cgroup.c                         | 305 +++++++++++++++++++++
 tools/bpf/bpftool/main.c                           |   3 +-
 tools/bpf/bpftool/main.h                           |   1 +
 7 files changed, 406 insertions(+), 5 deletions(-)
 create mode 100644 tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
 create mode 100644 tools/bpf/bpftool/cgroup.c

diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
new file mode 100644
index 000000000000..61ded613aee1
--- /dev/null
+++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
@@ -0,0 +1,92 @@
+================
+bpftool-cgroup
+================
+-------------------------------------------------------------------------------
+tool for inspection and simple manipulation of eBPF progs
+-------------------------------------------------------------------------------
+
+:Manual section: 8
+
+SYNOPSIS
+========
+
+	**bpftool** [*OPTIONS*] **cgroup** *COMMAND*
+
+	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
+
+	*COMMANDS* :=
+	{ **list** | **attach** | **detach** | **help** }
+
+MAP COMMANDS
+=============
+
+|	**bpftool** **cgroup list** *CGROUP*
+|	**bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
+|	**bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
+|	**bpftool** **cgroup help**
+|
+|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
+
+DESCRIPTION
+===========
+	**bpftool cgroup list** *CGROUP*
+		  List all programs attached to the cgroup *CGROUP*.
+
+		  Output will start with program ID followed by attach type,
+		  attach flags and program name.
+
+	**bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
+		  Attach program *PROG* to the cgroup *CGROUP* with attach type
+		  *ATTACH_TYPE* and optional *ATTACH_FLAGS*.
+
+	**bpftool cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
+		  Detach *PROG* from the cgroup *CGROUP* and attach type
+		  *ATTACH_TYPE*.
+
+	**bpftool prog help**
+		  Print short help message.
+
+OPTIONS
+=======
+	-h, --help
+		  Print short generic help message (similar to **bpftool help**).
+
+	-v, --version
+		  Print version number (similar to **bpftool version**).
+
+	-j, --json
+		  Generate JSON output. For commands that cannot produce JSON, this
+		  option has no effect.
+
+	-p, --pretty
+		  Generate human-readable JSON output. Implies **-j**.
+
+	-f, --bpffs
+		  Show file names of pinned programs.
+
+EXAMPLES
+========
+|
+| **# mount -t bpf none /sys/fs/bpf/**
+| **# mkdir /sys/fs/cgroup/test.slice**
+| **# bpftool prog load ./device_cgroup.o /sys/fs/bpf/prog**
+| **# bpftool cgroup attach /sys/fs/cgroup/test.slice/ device id 1 allow_multi**
+
+**# bpftool cgroup list /sys/fs/cgroup/test.slice/**
+
+::
+
+    ID       AttachType      AttachFlags     Name
+    1        device          allow_multi     bpf_prog1
+
+|
+| **# bpftool cgroup detach /sys/fs/cgroup/test.slice/ device id 1**
+| **# bpftool cgroup list /sys/fs/cgroup/test.slice/**
+
+::
+
+    ID       AttachType      AttachFlags     Name
+
+SEE ALSO
+========
+	**bpftool**\ (8), **bpftool-prog**\ (8), **bpftool-map**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 9f51a268eb06..421cabc417e6 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -128,4 +128,4 @@ EXAMPLES
 
 SEE ALSO
 ========
-	**bpftool**\ (8), **bpftool-prog**\ (8)
+	**bpftool**\ (8), **bpftool-prog**\ (8), **bpftool-cgroup**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 827b415f8ab6..61229a1779a3 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -155,4 +155,4 @@ EXAMPLES
 
 SEE ALSO
 ========
-	**bpftool**\ (8), **bpftool-map**\ (8)
+	**bpftool**\ (8), **bpftool-map**\ (8), **bpftool-cgroup**\ (8)
diff --git a/tools/bpf/bpftool/Documentation/bpftool.rst b/tools/bpf/bpftool/Documentation/bpftool.rst
index f547a0c0aa34..6732a5a617e4 100644
--- a/tools/bpf/bpftool/Documentation/bpftool.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool.rst
@@ -16,7 +16,7 @@ SYNOPSIS
 
 	**bpftool** **version**
 
-	*OBJECT* := { **map** | **program** }
+	*OBJECT* := { **map** | **program** | **cgroup** }
 
 	*OPTIONS* := { { **-V** | **--version** } | { **-h** | **--help** }
 	| { **-j** | **--json** } [{ **-p** | **--pretty** }] }
@@ -28,6 +28,8 @@ SYNOPSIS
 	*PROG-COMMANDS* := { **show** | **dump jited** | **dump xlated** | **pin**
 	| **load** | **help** }
 
+	*CGROUP-COMMANDS* := { **list** | **attach** | **detach** | **help** }
+
 DESCRIPTION
 ===========
 	*bpftool* allows for inspection and simple modification of BPF objects
@@ -53,4 +55,4 @@ OPTIONS
 
 SEE ALSO
 ========
-	**bpftool-map**\ (8), **bpftool-prog**\ (8)
+	**bpftool-map**\ (8), **bpftool-prog**\ (8), **bpftool-cgroup**\ (8)
diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
new file mode 100644
index 000000000000..88d67f74313f
--- /dev/null
+++ b/tools/bpf/bpftool/cgroup.c
@@ -0,0 +1,305 @@
+/*
+ * Copyright (C) 2017 Facebook
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * Author: Roman Gushchin <guro@fb.com>
+ */
+
+#include <fcntl.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#include <bpf.h>
+
+#include "main.h"
+
+static const char * const attach_type_strings[] = {
+	[BPF_CGROUP_INET_INGRESS] = "ingress",
+	[BPF_CGROUP_INET_EGRESS] = "egress",
+	[BPF_CGROUP_INET_SOCK_CREATE] = "sock_create",
+	[BPF_CGROUP_SOCK_OPS] = "sock_ops",
+	[BPF_CGROUP_DEVICE] = "device",
+	[__MAX_BPF_ATTACH_TYPE] = NULL,
+};
+
+static enum bpf_attach_type parse_attach_type(const char *str)
+{
+	enum bpf_attach_type type;
+
+	for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
+		if (attach_type_strings[type] &&
+		    strcmp(str, attach_type_strings[type]) == 0)
+			return type;
+	}
+
+	return __MAX_BPF_ATTACH_TYPE;
+}
+
+static int list_bpf_prog(int id, const char *attach_type_str,
+			 const char *attach_flags_str)
+{
+	struct bpf_prog_info info = {};
+	__u32 info_len = sizeof(info);
+	int prog_fd;
+
+	prog_fd = bpf_prog_get_fd_by_id(id);
+	if (prog_fd < 0)
+		return -1;
+
+	if (bpf_obj_get_info_by_fd(prog_fd, &info, &info_len)) {
+		close(prog_fd);
+		return -1;
+	}
+
+	if (json_output) {
+		jsonw_start_object(json_wtr);
+		jsonw_uint_field(json_wtr, "id", info.id);
+		jsonw_string_field(json_wtr, "attach_type",
+				   attach_type_str);
+		jsonw_string_field(json_wtr, "attach_flags",
+				   attach_flags_str);
+		jsonw_string_field(json_wtr, "name", info.name);
+		jsonw_end_object(json_wtr);
+	} else {
+		printf("%-8u %-15s %-15s %-15s\n", info.id,
+		       attach_type_str,
+		       attach_flags_str,
+		       info.name);
+	}
+
+	close(prog_fd);
+	return 0;
+}
+
+static int list_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
+{
+	__u32 attach_flags;
+	__u32 prog_ids[1024] = {0};
+	__u32 prog_cnt, iter;
+	char *attach_flags_str;
+	int ret;
+
+	prog_cnt = ARRAY_SIZE(prog_ids);
+	ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids,
+			     &prog_cnt);
+	if (ret)
+		return ret;
+
+	if (prog_cnt == 0)
+		return 0;
+
+	switch (attach_flags) {
+	case BPF_F_ALLOW_MULTI:
+		attach_flags_str = "allow_multi";
+		break;
+	case BPF_F_ALLOW_OVERRIDE:
+		attach_flags_str = "allow_override";
+		break;
+	case 0:
+		attach_flags_str = "";
+		break;
+	default:
+		attach_flags_str = "unknown";
+	}
+
+	for (iter = 0; iter < prog_cnt; iter++)
+		list_bpf_prog(prog_ids[iter], attach_type_strings[type],
+			      attach_flags_str);
+
+	return 0;
+}
+
+static int do_list(int argc, char **argv)
+{
+	enum bpf_attach_type type;
+	int cgroup_fd;
+	int ret = -1;
+
+	if (argc < 1) {
+		p_err("too few parameters for cgroup list\n");
+		goto exit;
+	} else if (argc > 1) {
+		p_err("too many parameters for cgroup list\n");
+		goto exit;
+	}
+
+	cgroup_fd = open(argv[0], O_RDONLY);
+	if (cgroup_fd < 0) {
+		p_err("can't open cgroup %s\n", argv[1]);
+		goto exit;
+	}
+
+	if (json_output)
+		jsonw_start_array(json_wtr);
+	else
+		printf("%-8s %-15s %-15s %-15s\n", "ID", "AttachType",
+		       "AttachFlags", "Name");
+
+	for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
+		/*
+		 * Not all attach types may be supported, so it's expected,
+		 * that some requests will fail.
+		 * If we were able to get the list for at least one
+		 * attach type, let's return 0.
+		 */
+		if (list_attached_bpf_progs(cgroup_fd, type) == 0)
+			ret = 0;
+	}
+
+	if (json_output)
+		jsonw_end_array(json_wtr);
+
+	close(cgroup_fd);
+exit:
+	return ret;
+}
+
+static int do_attach(int argc, char **argv)
+{
+	int cgroup_fd, prog_fd;
+	enum bpf_attach_type attach_type;
+	int attach_flags = 0;
+	int i;
+	int ret = -1;
+
+	if (argc < 4) {
+		p_err("too few parameters for cgroup attach\n");
+		goto exit;
+	}
+
+	cgroup_fd = open(argv[0], O_RDONLY);
+	if (cgroup_fd < 0) {
+		p_err("can't open cgroup %s\n", argv[1]);
+		goto exit;
+	}
+
+	attach_type = parse_attach_type(argv[1]);
+	if (attach_type == __MAX_BPF_ATTACH_TYPE) {
+		p_err("invalid attach type\n");
+		goto exit_cgroup;
+	}
+
+	argc -= 2;
+	argv = &argv[2];
+	prog_fd = prog_parse_fd(&argc, &argv);
+	if (prog_fd < 0)
+		goto exit_cgroup;
+
+	for (i = 0; i < argc; i++) {
+		if (strcmp(argv[i], "allow_multi") == 0) {
+			attach_flags |= BPF_F_ALLOW_MULTI;
+		} else if (strcmp(argv[i], "allow_override") == 0) {
+			attach_flags |= BPF_F_ALLOW_OVERRIDE;
+		} else {
+			p_err("unknown option: %s\n", argv[i]);
+			goto exit_cgroup;
+		}
+	}
+
+	if (bpf_prog_attach(prog_fd, cgroup_fd, attach_type, attach_flags)) {
+		p_err("failed to attach program");
+		goto exit_prog;
+	}
+
+	if (json_output)
+		jsonw_null(json_wtr);
+
+	ret = 0;
+
+exit_prog:
+	close(prog_fd);
+exit_cgroup:
+	close(cgroup_fd);
+exit:
+	return ret;
+}
+
+static int do_detach(int argc, char **argv)
+{
+	int prog_fd, cgroup_fd;
+	enum bpf_attach_type attach_type;
+	int ret = -1;
+
+	if (argc < 4) {
+		p_err("too few parameters for cgroup detach\n");
+		goto exit;
+	}
+
+	cgroup_fd = open(argv[0], O_RDONLY);
+	if (cgroup_fd < 0) {
+		p_err("can't open cgroup %s\n", argv[1]);
+		goto exit;
+	}
+
+	attach_type = parse_attach_type(argv[1]);
+	if (attach_type == __MAX_BPF_ATTACH_TYPE) {
+		p_err("invalid attach type");
+		goto exit_cgroup;
+	}
+
+	argc -= 2;
+	argv = &argv[2];
+	prog_fd = prog_parse_fd(&argc, &argv);
+	if (prog_fd < 0)
+		goto exit_cgroup;
+
+	if (bpf_prog_detach2(prog_fd, cgroup_fd, attach_type)) {
+		p_err("failed to attach program");
+		goto exit_prog;
+	}
+
+	if (json_output)
+		jsonw_null(json_wtr);
+
+	ret = 0;
+
+exit_prog:
+	close(prog_fd);
+exit_cgroup:
+	close(cgroup_fd);
+exit:
+	return ret;
+}
+
+static int do_help(int argc, char **argv)
+{
+	if (json_output) {
+		jsonw_null(json_wtr);
+		return 0;
+	}
+
+	fprintf(stderr,
+		"Usage: %s %s list CGROUP\n"
+		"       %s %s attach CGROUP TYPE PROG [ATTACH_FLAGS]\n"
+		"       %s %s detach CGROUP TYPE PROG\n"
+		"       %s %s help\n"
+		"\n"
+		"       ATTACH_FLAGS := { allow_multi | allow_override }"
+		"       " HELP_SPEC_PROGRAM "\n"
+		"       " HELP_SPEC_OPTIONS "\n"
+		"",
+		bin_name, argv[-2], bin_name, argv[-2],
+		bin_name, argv[-2], bin_name, argv[-2]);
+
+	return 0;
+}
+
+static const struct cmd cmds[] = {
+	{ "list",	do_list },
+	{ "attach",	do_attach },
+	{ "detach",	do_detach },
+	{ "help",	do_help },
+	{ 0 }
+};
+
+int do_cgroup(int argc, char **argv)
+{
+	return cmd_select(cmds, argc, argv, do_help);
+}
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index d294bc8168be..ecd53ccf1239 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -85,7 +85,7 @@ static int do_help(int argc, char **argv)
 		"       %s batch file FILE\n"
 		"       %s version\n"
 		"\n"
-		"       OBJECT := { prog | map }\n"
+		"       OBJECT := { prog | map | cgroup }\n"
 		"       " HELP_SPEC_OPTIONS "\n"
 		"",
 		bin_name, bin_name, bin_name);
@@ -173,6 +173,7 @@ static const struct cmd cmds[] = {
 	{ "batch",	do_batch },
 	{ "prog",	do_prog },
 	{ "map",	do_map },
+	{ "cgroup",	do_cgroup },
 	{ "version",	do_version },
 	{ 0 }
 };
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index bec1ccbb49c7..8f6d3cac0347 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -115,6 +115,7 @@ int do_pin_fd(int fd, const char *name);
 
 int do_prog(int argc, char **arg);
 int do_map(int argc, char **arg);
+int do_cgroup(int argc, char **arg);
 
 int prog_parse_fd(int *argc, char ***argv);
 
-- 
2.14.3

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[David Ahern]

On 12/7/17 11:39 AM, Roman Gushchin wrote:
> This patch adds basic cgroup bpf operations to bpftool:
> cgroup list, attach and detach commands.
> 
> Usage is described in the corresponding man pages,
> and examples are provided.
> 
> Syntax:
> $ bpftool cgroup list CGROUP
> $ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]
> $ bpftool cgroup detach CGROUP ATTACH_TYPE PROG
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Quentin Monnet <quentin.monnet@netronome.com>
> Cc: David Ahern <dsahern@gmail.com>
> ---


LGTM.

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Jakub Kicinski]

On Thu, 7 Dec 2017 18:39:09 +0000, Roman Gushchin wrote:
> This patch adds basic cgroup bpf operations to bpftool:
> cgroup list, attach and detach commands.
> 
> Usage is described in the corresponding man pages,
> and examples are provided.
> 
> Syntax:
> $ bpftool cgroup list CGROUP
> $ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]
> $ bpftool cgroup detach CGROUP ATTACH_TYPE PROG
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>

Looks good, a few very minor nits/questions below.

> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> new file mode 100644
> index 000000000000..88d67f74313f
> --- /dev/null
> +++ b/tools/bpf/bpftool/cgroup.c
> @@ -0,0 +1,305 @@
> +/*
> + * Copyright (C) 2017 Facebook
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * Author: Roman Gushchin <guro@fb.com>
> + */
> +
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <bpf.h>
> +
> +#include "main.h"
> +
> +static const char * const attach_type_strings[] = {
> +	[BPF_CGROUP_INET_INGRESS] = "ingress",
> +	[BPF_CGROUP_INET_EGRESS] = "egress",
> +	[BPF_CGROUP_INET_SOCK_CREATE] = "sock_create",
> +	[BPF_CGROUP_SOCK_OPS] = "sock_ops",
> +	[BPF_CGROUP_DEVICE] = "device",
> +	[__MAX_BPF_ATTACH_TYPE] = NULL,
> +};
> +
> +static enum bpf_attach_type parse_attach_type(const char *str)
> +{
> +	enum bpf_attach_type type;
> +
> +	for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
> +		if (attach_type_strings[type] &&
> +		    strcmp(str, attach_type_strings[type]) == 0)

Here and for matching flags you use straight strcmp(), not our locally
defined is_prefix(), is this intentional?  is_prefix() allows
abbreviations, like in iproute2 commands.  E.g. this would work:

# bpftool cg att /sys/fs/cgroup/test.slice/ dev id 1 allow_multi

> +			return type;
> +	}
> +
> +	return __MAX_BPF_ATTACH_TYPE;
> +}

> +static int list_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
> +{
> +	__u32 attach_flags;
> +	__u32 prog_ids[1024] = {0};
> +	__u32 prog_cnt, iter;
> +	char *attach_flags_str;
> +	int ret;

nit: could you reorder the variables so they're listed longest to
     shortest (reverse christmas tree)?

> +	prog_cnt = ARRAY_SIZE(prog_ids);
> +	ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids,
> +			     &prog_cnt);
> +	if (ret)
> +		return ret;
> +
> +	if (prog_cnt == 0)
> +		return 0;
> +
> +	switch (attach_flags) {
> +	case BPF_F_ALLOW_MULTI:
> +		attach_flags_str = "allow_multi";
> +		break;
> +	case BPF_F_ALLOW_OVERRIDE:
> +		attach_flags_str = "allow_override";
> +		break;
> +	case 0:
> +		attach_flags_str = "";
> +		break;
> +	default:
> +		attach_flags_str = "unknown";

nit: would it make sense to perhaps print flags in hex format in this
     case?

> +	}
> +
> +	for (iter = 0; iter < prog_cnt; iter++)
> +		list_bpf_prog(prog_ids[iter], attach_type_strings[type],
> +			      attach_flags_str);
> +
> +	return 0;
> +}

> +static int do_attach(int argc, char **argv)
> +{
> +	int cgroup_fd, prog_fd;
> +	enum bpf_attach_type attach_type;
> +	int attach_flags = 0;
> +	int i;
> +	int ret = -1;
> +
> +	if (argc < 4) {
> +		p_err("too few parameters for cgroup attach\n");
> +		goto exit;
> +	}
> +
> +	cgroup_fd = open(argv[0], O_RDONLY);
> +	if (cgroup_fd < 0) {
> +		p_err("can't open cgroup %s\n", argv[1]);
> +		goto exit;
> +	}
> +
> +	attach_type = parse_attach_type(argv[1]);
> +	if (attach_type == __MAX_BPF_ATTACH_TYPE) {
> +		p_err("invalid attach type\n");
> +		goto exit_cgroup;
> +	}
> +
> +	argc -= 2;
> +	argv = &argv[2];
> +	prog_fd = prog_parse_fd(&argc, &argv);
> +	if (prog_fd < 0)
> +		goto exit_cgroup;
> +
> +	for (i = 0; i < argc; i++) {
> +		if (strcmp(argv[i], "allow_multi") == 0) {
> +			attach_flags |= BPF_F_ALLOW_MULTI;
> +		} else if (strcmp(argv[i], "allow_override") == 0) {
> +			attach_flags |= BPF_F_ALLOW_OVERRIDE;

This is the other potential place for is_prefix() I referred to.

That reminds me - would you care to also update Quentin's bash
completions (tools/bpf/bpftool/bash-completion/bpftool)?  They 
are _very_ nice to have in day to day use!

> +		} else {
> +			p_err("unknown option: %s\n", argv[i]);
> +			goto exit_cgroup;
> +		}
> +	}
> +
> +	if (bpf_prog_attach(prog_fd, cgroup_fd, attach_type, attach_flags)) {
> +		p_err("failed to attach program");
> +		goto exit_prog;
> +	}
> +
> +	if (json_output)
> +		jsonw_null(json_wtr);
> +
> +	ret = 0;
> +
> +exit_prog:
> +	close(prog_fd);
> +exit_cgroup:
> +	close(cgroup_fd);
> +exit:
> +	return ret;
> +}

Otherwise looks really nice, thanks for working on it!

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Philippe Ombredanne]

Roman,

On Thu, Dec 7, 2017 at 11:23 PM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> On Thu, 7 Dec 2017 18:39:09 +0000, Roman Gushchin wrote:
>> This patch adds basic cgroup bpf operations to bpftool:
>> cgroup list, attach and detach commands.
>>
>> Usage is described in the corresponding man pages,
>> and examples are provided.
>>
>> Syntax:
>> $ bpftool cgroup list CGROUP
>> $ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]
>> $ bpftool cgroup detach CGROUP ATTACH_TYPE PROG
>>
>> Signed-off-by: Roman Gushchin <guro@fb.com>
>
> Looks good, a few very minor nits/questions below.
>
>> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
>> new file mode 100644
>> index 000000000000..88d67f74313f
>> --- /dev/null
>> +++ b/tools/bpf/bpftool/cgroup.c
>> @@ -0,0 +1,305 @@
>> +/*
>> + * Copyright (C) 2017 Facebook
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + *
>> + * Author: Roman Gushchin <guro@fb.com>
>> + */

Have you considered using the new SPDX ids instead?
e.g. may be something like:
// SPDX-License-Identifier: GPL-2.0+
// Copyright (C) 2017 Facebook
// Author: Roman Gushchin <guro@fb.com>

Don't you love it with less boilerplate and a better code/comments ratio?
BTW the comment style may surprise you here: this is a suggestion, but
not just. Check the posts from Linus on this topic and Thomas's doc
patches for the rationale.

Thank you for your kind consideration!

-- 
Cordially
Philippe Ombredanne

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Roman Gushchin]

On Thu, Dec 07, 2017 at 02:23:06PM -0800, Jakub Kicinski wrote:
> On Thu, 7 Dec 2017 18:39:09 +0000, Roman Gushchin wrote:
> > This patch adds basic cgroup bpf operations to bpftool:
> > cgroup list, attach and detach commands.
> > 
> > Usage is described in the corresponding man pages,
> > and examples are provided.
> > 
> > Syntax:
> > $ bpftool cgroup list CGROUP
> > $ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]
> > $ bpftool cgroup detach CGROUP ATTACH_TYPE PROG
> > 
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> 
> Looks good, a few very minor nits/questions below.
> 
> > diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> > new file mode 100644
> > index 000000000000..88d67f74313f
> > --- /dev/null
> > +++ b/tools/bpf/bpftool/cgroup.c
> > @@ -0,0 +1,305 @@
> > +/*
> > + * Copyright (C) 2017 Facebook
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version
> > + * 2 of the License, or (at your option) any later version.
> > + *
> > + * Author: Roman Gushchin <guro@fb.com>
> > + */
> > +
> > +#include <fcntl.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +
> > +#include <bpf.h>
> > +
> > +#include "main.h"
> > +
> > +static const char * const attach_type_strings[] = {
> > +	[BPF_CGROUP_INET_INGRESS] = "ingress",
> > +	[BPF_CGROUP_INET_EGRESS] = "egress",
> > +	[BPF_CGROUP_INET_SOCK_CREATE] = "sock_create",
> > +	[BPF_CGROUP_SOCK_OPS] = "sock_ops",
> > +	[BPF_CGROUP_DEVICE] = "device",
> > +	[__MAX_BPF_ATTACH_TYPE] = NULL,
> > +};
> > +
> > +static enum bpf_attach_type parse_attach_type(const char *str)
> > +{
> > +	enum bpf_attach_type type;
> > +
> > +	for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
> > +		if (attach_type_strings[type] &&
> > +		    strcmp(str, attach_type_strings[type]) == 0)
> 
> Here and for matching flags you use straight strcmp(), not our locally
> defined is_prefix(), is this intentional?  is_prefix() allows
> abbreviations, like in iproute2 commands.  E.g. this would work:

Fixed in v3.

> 
> # bpftool cg att /sys/fs/cgroup/test.slice/ dev id 1 allow_multi
> 
> > +			return type;
> > +	}
> > +
> > +	return __MAX_BPF_ATTACH_TYPE;
> > +}
> 
> > +static int list_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
> > +{
> > +	__u32 attach_flags;
> > +	__u32 prog_ids[1024] = {0};
> > +	__u32 prog_cnt, iter;
> > +	char *attach_flags_str;
> > +	int ret;
> 
> nit: could you reorder the variables so they're listed longest to
>      shortest (reverse christmas tree)?
> 
> > +	prog_cnt = ARRAY_SIZE(prog_ids);
> > +	ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids,
> > +			     &prog_cnt);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (prog_cnt == 0)
> > +		return 0;
> > +
> > +	switch (attach_flags) {
> > +	case BPF_F_ALLOW_MULTI:
> > +		attach_flags_str = "allow_multi";
> > +		break;
> > +	case BPF_F_ALLOW_OVERRIDE:
> > +		attach_flags_str = "allow_override";
> > +		break;
> > +	case 0:
> > +		attach_flags_str = "";
> > +		break;
> > +	default:
> > +		attach_flags_str = "unknown";
> 
> nit: would it make sense to perhaps print flags in hex format in this
>      case?
> 
> > +	}
> > +
> > +	for (iter = 0; iter < prog_cnt; iter++)
> > +		list_bpf_prog(prog_ids[iter], attach_type_strings[type],
> > +			      attach_flags_str);
> > +
> > +	return 0;
> > +}
> 
> > +static int do_attach(int argc, char **argv)
> > +{
> > +	int cgroup_fd, prog_fd;
> > +	enum bpf_attach_type attach_type;
> > +	int attach_flags = 0;
> > +	int i;
> > +	int ret = -1;
> > +
> > +	if (argc < 4) {
> > +		p_err("too few parameters for cgroup attach\n");
> > +		goto exit;
> > +	}
> > +
> > +	cgroup_fd = open(argv[0], O_RDONLY);
> > +	if (cgroup_fd < 0) {
> > +		p_err("can't open cgroup %s\n", argv[1]);
> > +		goto exit;
> > +	}
> > +
> > +	attach_type = parse_attach_type(argv[1]);
> > +	if (attach_type == __MAX_BPF_ATTACH_TYPE) {
> > +		p_err("invalid attach type\n");
> > +		goto exit_cgroup;
> > +	}
> > +
> > +	argc -= 2;
> > +	argv = &argv[2];
> > +	prog_fd = prog_parse_fd(&argc, &argv);
> > +	if (prog_fd < 0)
> > +		goto exit_cgroup;
> > +
> > +	for (i = 0; i < argc; i++) {
> > +		if (strcmp(argv[i], "allow_multi") == 0) {
> > +			attach_flags |= BPF_F_ALLOW_MULTI;
> > +		} else if (strcmp(argv[i], "allow_override") == 0) {
> > +			attach_flags |= BPF_F_ALLOW_OVERRIDE;
> 
> This is the other potential place for is_prefix() I referred to.

Not sure about this case, as allow_multi and allow_override have
a common "allow_" prefix, so it might be confusing.

> 
> That reminds me - would you care to also update Quentin's bash
> completions (tools/bpf/bpftool/bash-completion/bpftool)?  They 
> are _very_ nice to have in day to day use!

Sure, will do separately.

> Otherwise looks really nice, thanks for working on it!

Thank you for reviewing!

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Quentin Monnet]

2017-12-07 18:39 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> This patch adds basic cgroup bpf operations to bpftool:
> cgroup list, attach and detach commands.
> 
> Usage is described in the corresponding man pages,
> and examples are provided.
> 
> Syntax:
> $ bpftool cgroup list CGROUP
> $ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]
> $ bpftool cgroup detach CGROUP ATTACH_TYPE PROG
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Quentin Monnet <quentin.monnet@netronome.com>
> Cc: David Ahern <dsahern@gmail.com>
> ---
>  tools/bpf/bpftool/Documentation/bpftool-cgroup.rst |  92 +++++++
>  tools/bpf/bpftool/Documentation/bpftool-map.rst    |   2 +-
>  tools/bpf/bpftool/Documentation/bpftool-prog.rst   |   2 +-
>  tools/bpf/bpftool/Documentation/bpftool.rst        |   6 +-
>  tools/bpf/bpftool/cgroup.c                         | 305 +++++++++++++++++++++
>  tools/bpf/bpftool/main.c                           |   3 +-
>  tools/bpf/bpftool/main.h                           |   1 +
>  7 files changed, 406 insertions(+), 5 deletions(-)
>  create mode 100644 tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
>  create mode 100644 tools/bpf/bpftool/cgroup.c
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> new file mode 100644
> index 000000000000..61ded613aee1
> --- /dev/null
> +++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
> @@ -0,0 +1,92 @@
> +================
> +bpftool-cgroup
> +================
> +-------------------------------------------------------------------------------
> +tool for inspection and simple manipulation of eBPF progs
> +-------------------------------------------------------------------------------
> +
> +:Manual section: 8
> +
> +SYNOPSIS
> +========
> +
> +	**bpftool** [*OPTIONS*] **cgroup** *COMMAND*
> +
> +	*OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
> +
> +	*COMMANDS* :=
> +	{ **list** | **attach** | **detach** | **help** }
> +
> +MAP COMMANDS
> +=============
> +
> +|	**bpftool** **cgroup list** *CGROUP*
> +|	**bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> +|	**bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
> +|	**bpftool** **cgroup help**
> +|
> +|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }

Could you please give the different possible values for ATTACH_TYPE and
ATTACH_FLAGS, and provide some documentation for the flags?

> +
> +DESCRIPTION
> +===========
> +	**bpftool cgroup list** *CGROUP*
> +		  List all programs attached to the cgroup *CGROUP*.
> +
> +		  Output will start with program ID followed by attach type,
> +		  attach flags and program name.
> +
> +	**bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> +		  Attach program *PROG* to the cgroup *CGROUP* with attach type
> +		  *ATTACH_TYPE* and optional *ATTACH_FLAGS*.
> +
> +	**bpftool cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
> +		  Detach *PROG* from the cgroup *CGROUP* and attach type
> +		  *ATTACH_TYPE*.
> +
> +	**bpftool prog help**
> +		  Print short help message.

[…]

> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> new file mode 100644
> index 000000000000..88d67f74313f
> --- /dev/null
> +++ b/tools/bpf/bpftool/cgroup.c
> @@ -0,0 +1,305 @@
> +/*
> + * Copyright (C) 2017 Facebook
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * Author: Roman Gushchin <guro@fb.com>
> + */
> +

[…]

> +static int do_detach(int argc, char **argv)
> +{
> +	int prog_fd, cgroup_fd;
> +	enum bpf_attach_type attach_type;
> +	int ret = -1;
> +
> +	if (argc < 4) {
> +		p_err("too few parameters for cgroup detach\n");
> +		goto exit;
> +	}
> +
> +	cgroup_fd = open(argv[0], O_RDONLY);
> +	if (cgroup_fd < 0) {
> +		p_err("can't open cgroup %s\n", argv[1]);
> +		goto exit;
> +	}
> +
> +	attach_type = parse_attach_type(argv[1]);
> +	if (attach_type == __MAX_BPF_ATTACH_TYPE) {
> +		p_err("invalid attach type");
> +		goto exit_cgroup;
> +	}
> +
> +	argc -= 2;
> +	argv = &argv[2];
> +	prog_fd = prog_parse_fd(&argc, &argv);
> +	if (prog_fd < 0)
> +		goto exit_cgroup;
> +
> +	if (bpf_prog_detach2(prog_fd, cgroup_fd, attach_type)) {
> +		p_err("failed to attach program");

Failed to *detach* instead of “attach”.

> +		goto exit_prog;
> +	}
> +
> +	if (json_output)
> +		jsonw_null(json_wtr);
> +
> +	ret = 0;
> +
> +exit_prog:
> +	close(prog_fd);
> +exit_cgroup:
> +	close(cgroup_fd);
> +exit:
> +	return ret;
> +}

[…]

Very nice work on this v2, thanks a lot!
Quentin

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Philippe Ombredanne]

On Fri, Dec 8, 2017 at 11:34 AM, Quentin Monnet
<quentin.monnet@netronome.com> wrote:
> 2017-12-07 18:39 UTC+0000 ~ Roman Gushchin <guro@fb.com>
>> This patch adds basic cgroup bpf operations to bpftool:
>> cgroup list, attach and detach commands.

[...]
>> --- /dev/null
>> +++ b/tools/bpf/bpftool/cgroup.c
>> @@ -0,0 +1,305 @@
>> +/*
>> + * Copyright (C) 2017 Facebook
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + *
>> + *
>> + */
>> +

Roman,
Have you considered using the simpler and new SPDX ids instead? e.g.:

// SPDX-License-Identifier: GPL-2.0+
// Copyright (C) 2017 Facebook
// Author: Roman Gushchin <guro@fb.com>

This would boost your code/comments ratio nicely IMHO.

For  reference please check Linus [1][2][3], Thomas [4] and Greg [5]
comments on the topic of C++ style // comments!

Jonathan also wrote a nice background article on the SPDXification
topic at LWN [6]


PS: and if you could spread the word at FB, that would we awesome!

[1] https://lkml.org/lkml/2017/11/2/715
[2] https://lkml.org/lkml/2017/11/25/125
[3] https://lkml.org/lkml/2017/11/25/133
[4] https://lkml.org/lkml/2017/11/2/805
[5] https://lkml.org/lkml/2017/10/19/165
[6] https://lwn.net/Articles/739183/

-- 
Cordially
Philippe Ombredanne

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Roman Gushchin]

On Fri, Dec 08, 2017 at 02:56:15PM +0100, Philippe Ombredanne wrote:
> On Fri, Dec 8, 2017 at 11:34 AM, Quentin Monnet
> <quentin.monnet@netronome.com> wrote:
> > 2017-12-07 18:39 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> >> This patch adds basic cgroup bpf operations to bpftool:
> >> cgroup list, attach and detach commands.
> 
> [...]
> >> --- /dev/null
> >> +++ b/tools/bpf/bpftool/cgroup.c
> >> @@ -0,0 +1,305 @@
> >> +/*
> >> + * Copyright (C) 2017 Facebook
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU General Public License
> >> + * as published by the Free Software Foundation; either version
> >> + * 2 of the License, or (at your option) any later version.
> >> + *
> >> + *
> >> + */
> >> +
> 
> Roman,
> Have you considered using the simpler and new SPDX ids instead? e.g.:
> 
> // SPDX-License-Identifier: GPL-2.0+
> // Copyright (C) 2017 Facebook
> // Author: Roman Gushchin <guro@fb.com>
> 
> This would boost your code/comments ratio nicely IMHO.

Thanks, applied to v3!

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Roman Gushchin]

On Fri, Dec 08, 2017 at 10:34:16AM +0000, Quentin Monnet wrote:
> 2017-12-07 18:39 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> > This patch adds basic cgroup bpf operations to bpftool:
> > cgroup list, attach and detach commands.
> > 
> > Usage is described in the corresponding man pages,
> > and examples are provided.
[...]
> > +MAP COMMANDS
> > +=============
> > +
> > +|	**bpftool** **cgroup list** *CGROUP*
> > +|	**bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> > +|	**bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
> > +|	**bpftool** **cgroup help**
> > +|
> > +|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
> 
> Could you please give the different possible values for ATTACH_TYPE and
> ATTACH_FLAGS, and provide some documentation for the flags?

I intentionally didn't include the list of possible values, as it depends
on the exact kernel version, and other bpftool docs are carefully avoiding
specifying such things.

It would be nice to have a way to ask the kernel about provided bpf program types,
attach types, etc; but I'm not sure that hardcoding it in bpftool docs is
a good idea.

> 
> > +
> > +DESCRIPTION
> > +===========
> > +	**bpftool cgroup list** *CGROUP*
> > +		  List all programs attached to the cgroup *CGROUP*.
> > +
> > +		  Output will start with program ID followed by attach type,
> > +		  attach flags and program name.
> > +
> > +	**bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> > +		  Attach program *PROG* to the cgroup *CGROUP* with attach type
> > +		  *ATTACH_TYPE* and optional *ATTACH_FLAGS*.
[...]
> > +
> > +	attach_type = parse_attach_type(argv[1]);
> > +	if (attach_type == __MAX_BPF_ATTACH_TYPE) {
> > +		p_err("invalid attach type");
> > +		goto exit_cgroup;
> > +	}
> > +
> > +	argc -= 2;
> > +	argv = &argv[2];
> > +	prog_fd = prog_parse_fd(&argc, &argv);
> > +	if (prog_fd < 0)
> > +		goto exit_cgroup;
> > +
> > +	if (bpf_prog_detach2(prog_fd, cgroup_fd, attach_type)) {
> > +		p_err("failed to attach program");
> 
> Failed to *detach* instead of “attach”.

Fixed.

> 
> > +		goto exit_prog;
> > +	}
> > +
> > +	if (json_output)
> > +		jsonw_null(json_wtr);
> > +
> > +	ret = 0;
> > +
> > +exit_prog:
> > +	close(prog_fd);
> > +exit_cgroup:
> > +	close(cgroup_fd);
> > +exit:
> > +	return ret;
> > +}
> 
> […]
> 
> Very nice work on this v2, thanks a lot!
> Quentin

Thank you for reviewing!

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Quentin Monnet]

2017-12-08 14:12 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> On Fri, Dec 08, 2017 at 10:34:16AM +0000, Quentin Monnet wrote:
>> 2017-12-07 18:39 UTC+0000 ~ Roman Gushchin <guro@fb.com>
>>> This patch adds basic cgroup bpf operations to bpftool:
>>> cgroup list, attach and detach commands.
>>>
>>> Usage is described in the corresponding man pages,
>>> and examples are provided.
> [...]
>>> +MAP COMMANDS
>>> +=============
>>> +
>>> +|	**bpftool** **cgroup list** *CGROUP*
>>> +|	**bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
>>> +|	**bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
>>> +|	**bpftool** **cgroup help**
>>> +|
>>> +|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
>>
>> Could you please give the different possible values for ATTACH_TYPE and
>> ATTACH_FLAGS, and provide some documentation for the flags?
> 
> I intentionally didn't include the list of possible values, as it depends
> on the exact kernel version, and other bpftool docs are carefully avoiding
> specifying such things.

Do they? As far as I can tell the only other bpftool command that uses
flags is the `bpftool map update`, and it does specify the possible
values for UPDATE_FLAGS (and document them) in the man page.

I don't believe compatibility is an issue here, since the program and
its documentation come together (so they should stay in sync) and are
part of the kernel tree (so the tool should be compatible with the
kernel sources it comes with). My concern is that there is no way to
guess from the current description what the values for ATTACH_FLAG or
ATTACH_TYPE can be, without reading the source code of the program—which
is not exactly user-friendly.

> 
> It would be nice to have a way to ask the kernel about provided bpf program types,
> attach types, etc; but I'm not sure that hardcoding it in bpftool docs is
> a good idea.

They are coded into the bpftool that comes with the docs anyway :).

Quentin

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[David Ahern]

On 12/8/17 8:39 AM, Quentin Monnet wrote:
> I don't believe compatibility is an issue here, since the program and
> its documentation come together (so they should stay in sync) and are
> part of the kernel tree (so the tool should be compatible with the
> kernel sources it comes with). My concern is that there is no way to
> guess from the current description what the values for ATTACH_FLAG or
> ATTACH_TYPE can be, without reading the source code of the program—which
> is not exactly user-friendly.
> 

The tool should be backward and forward compatible across kernel
versions. Running a newer command on an older kernel should fail in a
deterministic. While the tool is in the kernel tree for ease of
development, that should not be confused with having a direct tie to any
kernel version.

I believe man pages do include kernel version descriptions in flags
(e.g., man 7 socket -- flags are denoted with "since Linux x.y") which
is one way to handle it with the usual caveat that vendors might have
backported support to earlier kernels.

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Jakub Kicinski]

On Fri, 8 Dec 2017 09:52:16 -0700, David Ahern wrote:
> On 12/8/17 8:39 AM, Quentin Monnet wrote:
> > I don't believe compatibility is an issue here, since the program and
> > its documentation come together (so they should stay in sync) and are
> > part of the kernel tree (so the tool should be compatible with the
> > kernel sources it comes with). My concern is that there is no way to
> > guess from the current description what the values for ATTACH_FLAG or
> > ATTACH_TYPE can be, without reading the source code of the program—which
> > is not exactly user-friendly.
> 
> The tool should be backward and forward compatible across kernel
> versions. Running a newer command on an older kernel should fail in a
> deterministic. While the tool is in the kernel tree for ease of
> development, that should not be confused with having a direct tie to any
> kernel version.
>
> I believe man pages do include kernel version descriptions in flags
> (e.g., man 7 socket -- flags are denoted with "since Linux x.y") which
> is one way to handle it with the usual caveat that vendors might have
> backported support to earlier kernels.

Let's see if I understand correctly.  We have a list of hard coded
strings which the tool will recognize (static const char * const
attach_type_strings[]).  We should put that list into the man page and
help so users know what values are possible.  And in the "verbose"
part of the man section mark each flag with kernel version it was
introduced in.

Roman, would you agree this is the best way forward?

Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations

[Roman Gushchin]

On Fri, Dec 08, 2017 at 03:39:43PM +0000, Quentin Monnet wrote:
> 2017-12-08 14:12 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> > On Fri, Dec 08, 2017 at 10:34:16AM +0000, Quentin Monnet wrote:
> >> 2017-12-07 18:39 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> >>> This patch adds basic cgroup bpf operations to bpftool:
> >>> cgroup list, attach and detach commands.
> >>>
> >>> Usage is described in the corresponding man pages,
> >>> and examples are provided.
> > [...]
> >>> +MAP COMMANDS
> >>> +=============
> >>> +
> >>> +|	**bpftool** **cgroup list** *CGROUP*
> >>> +|	**bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
> >>> +|	**bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
> >>> +|	**bpftool** **cgroup help**
> >>> +|
> >>> +|	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
> >>
> >> Could you please give the different possible values for ATTACH_TYPE and
> >> ATTACH_FLAGS, and provide some documentation for the flags?
> > 
> > I intentionally didn't include the list of possible values, as it depends
> > on the exact kernel version, and other bpftool docs are carefully avoiding
> > specifying such things.
> 
> Do they? As far as I can tell the only other bpftool command that uses
> flags is the `bpftool map update`, and it does specify the possible
> values for UPDATE_FLAGS (and document them) in the man page.

You are right about UPDATE_FLAGS, but at the same time we do
not describe bpf program attributes in prog show:
  **bpftool prog show** [*PROG*]
	  Show information about loaded programs.  If *PROG* is
	  specified show information only about given program, otherwise
	  list all programs currently loaded on the system.

	  Output will start with program ID followed by program type and
	  zero or more named attributes (depending on kernel version).

I think, that actually ATTACH_TYPE is similar to PROGRAM_TYPE because
it will likely be extended in the following kernel versions.
So we should probably support specifying it in a numeric form too.

ATTACH_FLAGS are probably less volatile and will unlikely be extended often,
so we can describe them in docs and add a note about the kernel version
next time when a new flag will be added.

Anyway, I don't see any big problem in documenting current ATTACH_FLAG
and ATTACH_TYPE sets, if we think that it's a good way forward.

Thanks!