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

[PATCH net-next 0/5] 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.

Roman Gushchin (5):
  libbpf: add ability to guess program type based on section name
  libbpf: prefer global symbols as bpf program name source
  bpftool: implement cgattach command
  bpftool: implement cgdetach command
  bpftool: implement cglist command

 tools/bpf/bpftool/main.c | 209 ++++++++++++++++++++++++++++++++++++++++++++++-
 tools/lib/bpf/libbpf.c   |  49 +++++++++++
 2 files changed, 257 insertions(+), 1 deletion(-)

-- 
2.14.3

[PATCH net-next 1/5] 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>
---
 tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 5aa45f89da93..9f2410beaa18 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1721,6 +1721,41 @@ 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);
 
+static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
+{
+	if (!prog->section_name)
+		goto err;
+
+	if (strncmp(prog->section_name, "socket", 6) == 0)
+		return BPF_PROG_TYPE_SOCKET_FILTER;
+	if (strncmp(prog->section_name, "kprobe/", 7) == 0)
+		return BPF_PROG_TYPE_KPROBE;
+	if (strncmp(prog->section_name, "kretprobe/", 10) == 0)
+		return BPF_PROG_TYPE_KPROBE;
+	if (strncmp(prog->section_name, "tracepoint/", 11) == 0)
+		return BPF_PROG_TYPE_TRACEPOINT;
+	if (strncmp(prog->section_name, "xdp", 3) == 0)
+		return BPF_PROG_TYPE_XDP;
+	if (strncmp(prog->section_name, "perf_event", 10) == 0)
+		return BPF_PROG_TYPE_PERF_EVENT;
+	if (strncmp(prog->section_name, "cgroup/skb", 10) == 0)
+		return BPF_PROG_TYPE_CGROUP_SKB;
+	if (strncmp(prog->section_name, "cgroup/sock", 11) == 0)
+		return BPF_PROG_TYPE_CGROUP_SOCK;
+	if (strncmp(prog->section_name, "cgroup/dev", 10) == 0)
+		return BPF_PROG_TYPE_CGROUP_DEVICE;
+	if (strncmp(prog->section_name, "sockops", 7) == 0)
+		return BPF_PROG_TYPE_SOCK_OPS;
+	if (strncmp(prog->section_name, "sk_skb", 6) == 0)
+		return BPF_PROG_TYPE_SK_SKB;
+
+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 +1867,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

[PATCH net-next 2/5] 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 cglist /sys/fs/cgroup/system.slice/tmp.mount/
      16 device          LBB0_10

Fix this by preferring global symbols as program name.
For instance:

$ bpftool cglist /sys/fs/cgroup/system.slice/tmp.mount/
      17 device          bpf_prog1

Signed-off-by: Roman Gushchin <guro@fb.com>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Jakub Kicinski <jakub.kicinski@netronome.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 9f2410beaa18..5191afd46556 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 net-next 3/5] bpftool: implement cgattach command

[Roman Gushchin]

This patch add the cgattach command to bpftool.
It allows to load a bpf program from a binary file and attach
it to a given cgroup.

Example:
$ bpftool cgattach ./mybpfprog.o /sys/fs/cgroup/user.slice/ ingress

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>
---
 tools/bpf/bpftool/main.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index d6e4762170a4..8eb3b9bf5bb2 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -41,9 +41,14 @@
 #include <linux/version.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <unistd.h>
 #include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
 
 #include <bpf.h>
+#include <libbpf.h>
 
 #include "main.h"
 
@@ -75,12 +80,13 @@ static int do_help(int argc, char **argv)
 	fprintf(stderr,
 		"Usage: %s [OPTIONS] OBJECT { COMMAND | help }\n"
 		"       %s batch file FILE\n"
+		"       %s cgattach FILE CGROUP TYPE\n"
 		"       %s version\n"
 		"\n"
 		"       OBJECT := { prog | map }\n"
 		"       " HELP_SPEC_OPTIONS "\n"
 		"",
-		bin_name, bin_name, bin_name);
+		bin_name, bin_name, bin_name, bin_name);
 
 	return 0;
 }
@@ -159,12 +165,14 @@ void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
 }
 
 static int do_batch(int argc, char **argv);
+static int do_cgattach(int argc, char **argv);
 
 static const struct cmd cmds[] = {
 	{ "help",	do_help },
 	{ "batch",	do_batch },
 	{ "prog",	do_prog },
 	{ "map",	do_map },
+	{ "cgattach",	do_cgattach },
 	{ "version",	do_version },
 	{ 0 }
 };
@@ -259,6 +267,77 @@ static int do_batch(int argc, char **argv)
 	return err;
 }
 
+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,
+};
+
+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 do_cgattach(int argc, char **argv)
+{
+	struct bpf_object *obj;
+	int prog_fd, cgroup_fd;
+	enum bpf_attach_type attach_type;
+
+	if (argc < 3) {
+		p_err("too few parameters for cgattach\n");
+		return -1;
+	} else if (argc > 3) {
+		p_err("too many parameters for cgattach\n");
+		return -1;
+	}
+
+	if (bpf_prog_load(argv[0], BPF_PROG_TYPE_UNSPEC, &obj, &prog_fd))
+		return -1;
+
+	cgroup_fd = open(argv[1], O_RDONLY);
+	if (cgroup_fd < 0) {
+		bpf_object__close(obj);
+		close(prog_fd);
+		p_err("can't open cgroup %s\n", argv[1]);
+		return -1;
+	}
+
+	attach_type = parse_attach_type(argv[2]);
+	if (attach_type == __MAX_BPF_ATTACH_TYPE) {
+		bpf_object__close(obj);
+		close(prog_fd);
+		close(cgroup_fd);
+		p_err("Invalid attach type\n");
+		return -1;
+	}
+
+	if (bpf_prog_attach(prog_fd, cgroup_fd, attach_type, 0)) {
+		bpf_object__close(obj);
+		close(prog_fd);
+		close(cgroup_fd);
+		p_err("Failed to attach program");
+		return -1;
+	}
+
+	bpf_object__close(obj);
+	close(prog_fd);
+	close(cgroup_fd);
+
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	static const struct option options[] = {
-- 
2.14.3

[PATCH net-next 4/5] bpftool: implement cgdetach command

[Roman Gushchin]

Implement cgdetach command, which allows to detach the bpf
program from a cgroup. It takes program id and attach type
as arguments.

Example:
$ ./bpftool cgdetach /sys/fs/cgroup/user.slice/ device 1

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>
---
 tools/bpf/bpftool/main.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 8eb3b9bf5bb2..77fcc1a0bd5d 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -81,12 +81,13 @@ static int do_help(int argc, char **argv)
 		"Usage: %s [OPTIONS] OBJECT { COMMAND | help }\n"
 		"       %s batch file FILE\n"
 		"       %s cgattach FILE CGROUP TYPE\n"
+		"       %s cgdetach CGROUP TYPE ID\n"
 		"       %s version\n"
 		"\n"
 		"       OBJECT := { prog | map }\n"
 		"       " HELP_SPEC_OPTIONS "\n"
 		"",
-		bin_name, bin_name, bin_name, bin_name);
+		bin_name, bin_name, bin_name, bin_name, bin_name);
 
 	return 0;
 }
@@ -166,6 +167,7 @@ void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
 
 static int do_batch(int argc, char **argv);
 static int do_cgattach(int argc, char **argv);
+static int do_cgdetach(int argc, char **argv);
 
 static const struct cmd cmds[] = {
 	{ "help",	do_help },
@@ -173,6 +175,7 @@ static const struct cmd cmds[] = {
 	{ "prog",	do_prog },
 	{ "map",	do_map },
 	{ "cgattach",	do_cgattach },
+	{ "cgdetach",	do_cgdetach },
 	{ "version",	do_version },
 	{ 0 }
 };
@@ -338,6 +341,51 @@ static int do_cgattach(int argc, char **argv)
 	return 0;
 }
 
+static int do_cgdetach(int argc, char **argv)
+{
+	int prog_fd, cgroup_fd;
+	enum bpf_attach_type attach_type;
+
+	if (argc < 3) {
+		p_err("too few parameters for cgdetach\n");
+		return -1;
+	} else if (argc > 3) {
+		p_err("too many parameters for cgdetach\n");
+		return -1;
+	}
+
+	cgroup_fd = open(argv[0], O_RDONLY);
+	if (cgroup_fd < 0) {
+		p_err("can't open cgroup %s\n", argv[1]);
+		return -1;
+	}
+
+	attach_type = parse_attach_type(argv[1]);
+	if (attach_type == __MAX_BPF_ATTACH_TYPE) {
+		close(cgroup_fd);
+		p_err("Invalid attach type");
+		return -1;
+	}
+
+	prog_fd = bpf_prog_get_fd_by_id(atoi(argv[2]));
+	if (prog_fd < 0) {
+		p_err("invalid program id\n");
+		return -1;
+	}
+
+	if (bpf_prog_detach2(prog_fd, cgroup_fd, attach_type)) {
+		close(prog_fd);
+		close(cgroup_fd);
+		p_err("Failed to attach program");
+		return -1;
+	}
+
+	close(prog_fd);
+	close(cgroup_fd);
+
+	return 0;
+}
+
 int main(int argc, char **argv)
 {
 	static const struct option options[] = {
-- 
2.14.3

[PATCH net-next 5/5] bpftool: implement cglist command

[Roman Gushchin]

Implement cgattach command to list bpf progrrams attached
to the given cgroup:

Example:
$ ./bpftool cgattach dev_cgroup.o /sys/fs/cgroup/user.slice/ device
$ ./bpftool cglist /sys/fs/cgroup/user.slice/
ID       AttachType      Name
1        device          bpf_prog1

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>
---
 tools/bpf/bpftool/main.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 77fcc1a0bd5d..8a48f6a32adc 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -82,12 +82,13 @@ static int do_help(int argc, char **argv)
 		"       %s batch file FILE\n"
 		"       %s cgattach FILE CGROUP TYPE\n"
 		"       %s cgdetach CGROUP TYPE ID\n"
+		"       %s cglist CGROUP TYPE\n"
 		"       %s version\n"
 		"\n"
 		"       OBJECT := { prog | map }\n"
 		"       " HELP_SPEC_OPTIONS "\n"
 		"",
-		bin_name, bin_name, bin_name, bin_name, bin_name);
+		bin_name, bin_name, bin_name, bin_name, bin_name, bin_name);
 
 	return 0;
 }
@@ -168,6 +169,7 @@ void fprint_hex(FILE *f, void *arg, unsigned int n, const char *sep)
 static int do_batch(int argc, char **argv);
 static int do_cgattach(int argc, char **argv);
 static int do_cgdetach(int argc, char **argv);
+static int do_cglist(int argc, char **argv);
 
 static const struct cmd cmds[] = {
 	{ "help",	do_help },
@@ -176,6 +178,7 @@ static const struct cmd cmds[] = {
 	{ "map",	do_map },
 	{ "cgattach",	do_cgattach },
 	{ "cgdetach",	do_cgdetach },
+	{ "cglist",	do_cglist },
 	{ "version",	do_version },
 	{ 0 }
 };
@@ -386,6 +389,83 @@ static int do_cgdetach(int argc, char **argv)
 	return 0;
 }
 
+static int do_cglist(int argc, char **argv)
+{
+	enum bpf_attach_type type;
+	int prog_fd, cgroup_fd;
+	__u32 attach_flags;
+	__u32 prog_ids[1024] = {0};
+	__u32 prog_cnt, iter;
+	int ret = -1;
+	struct bpf_prog_info info = {};
+	__u32 info_len = sizeof(info);
+
+	if (argc < 1) {
+		p_err("too few parameters for cglist\n");
+		return -1;
+	} else if (argc > 1) {
+		p_err("too many parameters for cglist\n");
+		return -1;
+	}
+
+	cgroup_fd = open(argv[0], O_RDONLY);
+	if (cgroup_fd < 0) {
+		p_err("can't open cgroup %s\n", argv[1]);
+		return -1;
+	}
+
+	if (json_output)
+		jsonw_start_array(json_wtr);
+
+	for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
+		prog_cnt = ARRAY_SIZE(prog_ids);
+		if (bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids,
+				   &prog_cnt))
+			continue;
+
+		ret = 0;
+
+		if (prog_cnt == 0)
+			continue;
+
+		if (!json_output)
+			printf("%-8s %-15s %-15s\n", "ID", "AttachType",
+			       "Name");
+
+		for (iter = 0; iter < prog_cnt; iter++) {
+			prog_fd = bpf_prog_get_fd_by_id(prog_ids[iter]);
+			if (prog_fd < 0)
+				continue;
+
+			if (bpf_obj_get_info_by_fd(prog_fd, &info, &info_len)) {
+				close(prog_fd);
+				continue;
+			}
+
+			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_strings[type]);
+				jsonw_string_field(json_wtr, "name", info.name);
+				jsonw_end_object(json_wtr);
+			} else {
+				printf("%-8u %-15s %-15s\n", info.id,
+				       attach_type_strings[type], info.name);
+			}
+
+			close(prog_fd);
+		}
+	}
+
+	if (json_output)
+		jsonw_end_array(json_wtr);
+
+	close(cgroup_fd);
+
+	return ret;
+}
+
 int main(int argc, char **argv)
 {
 	static const struct option options[] = {
-- 
2.14.3

Re: [PATCH net-next 3/5] bpftool: implement cgattach command

[David Ahern]

On 11/30/17 6:43 AM, Roman Gushchin wrote:
> @@ -75,12 +80,13 @@ static int do_help(int argc, char **argv)
>  	fprintf(stderr,
>  		"Usage: %s [OPTIONS] OBJECT { COMMAND | help }\n"
>  		"       %s batch file FILE\n"
> +		"       %s cgattach FILE CGROUP TYPE\n"

Can you change the order to:
+		"       %s cgattach CGROUP TYPE FILE\n"

Makes for better consistency with the detach command in the next patch:
+		"       %s cgdetach CGROUP TYPE ID\n"

Re: [PATCH net-next 3/5] bpftool: implement cgattach command

[David Ahern]

On 11/30/17 6:43 AM, Roman Gushchin wrote:
> +	if (bpf_prog_attach(prog_fd, cgroup_fd, attach_type, 0)) {
> +		bpf_object__close(obj);
> +		close(prog_fd);
> +		close(cgroup_fd);
> +		p_err("Failed to attach program");
> +		return -1;
> +	}


Also, what about support for BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI
flags?

Re: [PATCH net-next 3/5] bpftool: implement cgattach command

[Roman Gushchin]

On Thu, Nov 30, 2017 at 09:17:17AM -0700, David Ahern wrote:
> On 11/30/17 6:43 AM, Roman Gushchin wrote:
> > @@ -75,12 +80,13 @@ static int do_help(int argc, char **argv)
> >  	fprintf(stderr,
> >  		"Usage: %s [OPTIONS] OBJECT { COMMAND | help }\n"
> >  		"       %s batch file FILE\n"
> > +		"       %s cgattach FILE CGROUP TYPE\n"
> 
> Can you change the order to:
> +		"       %s cgattach CGROUP TYPE FILE\n"
> 
> Makes for better consistency with the detach command in the next patch:
> +		"       %s cgdetach CGROUP TYPE ID\n"
> 
> 

Good point.

I'll fix this and will add support for attach_flags in v2.

Thanks!

Re: [PATCH net-next 0/5] bpftool: cgroup bpf operations

[Jakub Kicinski]

Hi Roman!

On Thu, 30 Nov 2017 13:42:57 +0000, Roman Gushchin wrote:
> 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.

Could you place your code in a new file and add a new "object level"?
I.e. 
bpftool cgroup list 
bpftool cgroup attach ...
bpftool cgroup help
etc?  Note that you probably want the list to be first, so if someone
types "bpftool cg" it runs list by default.

Does it make sense to support pinned files and specifying programs by
id?  I used the "id"/"pinned" keywords so that users can choose to use
either.  Perhaps you should at least prefix the file to with "file"?
So:
$ bpftool cgattach file ./mybpfprog.o /sys/fs/cgroup/user.slice/ ingress
$ bpftool cgattach id 19 /sys/fs/cgroup/user.slice/ ingress
$ bpftool cgattach pin /bpf/prog /sys/fs/cgroup/user.slice/ ingress
Would this make sense?

Smaller nits on the coding style:
 - please try to run checkpatch, perhaps you did, but some people
   forget tools are in the kernel tree :)
 - please keep includes in alphabetical order;
 - please keep variable declarations in functions ordered longest to
   shortest, if that's impossible because of dependency between
   initializers - move the initializers to the code.

Please also don't forget to update/create new man page.

Thanks! :)

Re: [PATCH net-next 3/5] bpftool: implement cgattach command

[Jakub Kicinski]

On Thu, 30 Nov 2017 13:43:00 +0000, Roman Gushchin wrote:
> +	attach_type = parse_attach_type(argv[2]);
> +	if (attach_type == __MAX_BPF_ATTACH_TYPE) {
> +		bpf_object__close(obj);
> +		close(prog_fd);
> +		close(cgroup_fd);
> +		p_err("Invalid attach type\n");
> +		return -1;
> +	}
> +
> +	if (bpf_prog_attach(prog_fd, cgroup_fd, attach_type, 0)) {
> +		bpf_object__close(obj);
> +		close(prog_fd);
> +		close(cgroup_fd);
> +		p_err("Failed to attach program");
> +		return -1;
> +	}
> +
> +	bpf_object__close(obj);
> +	close(prog_fd);
> +	close(cgroup_fd);
> +
> +	return 0;
> +}

Could you try to consolidate the error paths into a one larger handler
and use gotos to jump to it?  You can see it done in number of places,
grep for e.g. exit_free.

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

[Quentin Monnet]

Thanks Roman!
One comment in-line.

2017-11-30 13:42 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>
> ---
>  tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 5aa45f89da93..9f2410beaa18 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1721,6 +1721,41 @@ 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);
>  
> +static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
> +{
> +	if (!prog->section_name)
> +		goto err;
> +
> +	if (strncmp(prog->section_name, "socket", 6) == 0)
> +		return BPF_PROG_TYPE_SOCKET_FILTER;
> +	if (strncmp(prog->section_name, "kprobe/", 7) == 0)
> +		return BPF_PROG_TYPE_KPROBE;
> +	if (strncmp(prog->section_name, "kretprobe/", 10) == 0)
> +		return BPF_PROG_TYPE_KPROBE;
> +	if (strncmp(prog->section_name, "tracepoint/", 11) == 0)
> +		return BPF_PROG_TYPE_TRACEPOINT;
> +	if (strncmp(prog->section_name, "xdp", 3) == 0)
> +		return BPF_PROG_TYPE_XDP;
> +	if (strncmp(prog->section_name, "perf_event", 10) == 0)
> +		return BPF_PROG_TYPE_PERF_EVENT;
> +	if (strncmp(prog->section_name, "cgroup/skb", 10) == 0)
> +		return BPF_PROG_TYPE_CGROUP_SKB;
> +	if (strncmp(prog->section_name, "cgroup/sock", 11) == 0)
> +		return BPF_PROG_TYPE_CGROUP_SOCK;
> +	if (strncmp(prog->section_name, "cgroup/dev", 10) == 0)
> +		return BPF_PROG_TYPE_CGROUP_DEVICE;
> +	if (strncmp(prog->section_name, "sockops", 7) == 0)
> +		return BPF_PROG_TYPE_SOCK_OPS;
> +	if (strncmp(prog->section_name, "sk_skb", 6) == 0)
> +		return BPF_PROG_TYPE_SK_SKB;

I do not really like these hard-coded lengths, maybe we could work out
something nicer with a bit of pre-processing work? Perhaps something like:

#define SOCKET_FILTER_SEC_PREFIX "socket"
#define KPROBE_SEC_PREFIX "kprobe/"
[…]

#define TRY_TYPE(string, __TYPE)				\
	do {							\
		if (!strncmp(string, __TYPE ## _SEC_PREFIX,	\
			     sizeof(__TYPE ## _SEC_PREFIX)))	\
			return BPF_PROG_TYPE_ ## __TYPE;	\
	} while(0);

static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
{
	if (!prog->section_name)
		goto err;

	TRY_TYPE(prog->section_name, SOCKET_FILTER);
	TRY_TYPE(prog->section_name, KPROBE);
	[…]

err:
	pr_warning("…",
	           prog->section_name);

	return BPF_PROG_TYPE_UNSPEC;
}

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

Re: [PATCH net-next 4/5] bpftool: implement cgdetach command

[Quentin Monnet]

2017-11-30 13:43 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> Implement cgdetach command, which allows to detach the bpf
> program from a cgroup. It takes program id and attach type
> as arguments.
> 
> Example:
> $ ./bpftool cgdetach /sys/fs/cgroup/user.slice/ device 1
> 
> 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>
> ---
>  tools/bpf/bpftool/main.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 8eb3b9bf5bb2..77fcc1a0bd5d 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c

[…]

> @@ -338,6 +341,51 @@ static int do_cgattach(int argc, char **argv)
>  	return 0;
>  }
>  
> +static int do_cgdetach(int argc, char **argv)
> +{
> +	int prog_fd, cgroup_fd;
> +	enum bpf_attach_type attach_type;
> +
> +	if (argc < 3) {
> +		p_err("too few parameters for cgdetach\n");
> +		return -1;
> +	} else if (argc > 3) {
> +		p_err("too many parameters for cgdetach\n");
> +		return -1;
> +	}
> +
> +	cgroup_fd = open(argv[0], O_RDONLY);
> +	if (cgroup_fd < 0) {
> +		p_err("can't open cgroup %s\n", argv[1]);
> +		return -1;
> +	}
> +
> +	attach_type = parse_attach_type(argv[1]);
> +	if (attach_type == __MAX_BPF_ATTACH_TYPE) {
> +		close(cgroup_fd);
> +		p_err("Invalid attach type");
> +		return -1;
> +	}
> +
> +	prog_fd = bpf_prog_get_fd_by_id(atoi(argv[2]));
> +	if (prog_fd < 0) {
> +		p_err("invalid program id\n");
> +		return -1;
> +	}
> +
> +	if (bpf_prog_detach2(prog_fd, cgroup_fd, attach_type)) {
> +		close(prog_fd);
> +		close(cgroup_fd);
> +		p_err("Failed to attach program");
> +		return -1;
> +	}
> +
> +	close(prog_fd);
> +	close(cgroup_fd);

Could you please make the function print a "null" string for JSON here?
So that it does not break JSON for batched commands. Should be as simple as:

	jsonw_null(json_wtr);

This is also valid for `do_cgattach()` in patch 3.
Thanks!

> +
> +	return 0;
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	static const struct option options[] = {
> 

Re: [PATCH net-next 0/5] bpftool: cgroup bpf operations

[Roman Gushchin]

On Thu, Nov 30, 2017 at 07:04:54PM -0800, Jakub Kicinski wrote:
> Hi Roman!
> 
> On Thu, 30 Nov 2017 13:42:57 +0000, Roman Gushchin wrote:
> > 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.
> 
> Could you place your code in a new file and add a new "object level"?
> I.e. 
> bpftool cgroup list 
> bpftool cgroup attach ...
> bpftool cgroup help
> etc?  Note that you probably want the list to be first, so if someone
> types "bpftool cg" it runs list by default.
> 
> Does it make sense to support pinned files and specifying programs by
> id?  I used the "id"/"pinned" keywords so that users can choose to use
> either.  Perhaps you should at least prefix the file to with "file"?
> So:
> $ bpftool cgattach file ./mybpfprog.o /sys/fs/cgroup/user.slice/ ingress
> $ bpftool cgattach id 19 /sys/fs/cgroup/user.slice/ ingress
> $ bpftool cgattach pin /bpf/prog /sys/fs/cgroup/user.slice/ ingress
> Would this make sense?
> 
> Smaller nits on the coding style:
>  - please try to run checkpatch, perhaps you did, but some people
>    forget tools are in the kernel tree :)
>  - please keep includes in alphabetical order;
>  - please keep variable declarations in functions ordered longest to
>    shortest, if that's impossible because of dependency between
>    initializers - move the initializers to the code.
> 
> Please also don't forget to update/create new man page.

Ok, I'll try to address these comments in v2.

Thank you!

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

[Jakub Kicinski]

On Fri, 1 Dec 2017 10:22:57 +0000, Quentin Monnet wrote:
> Thanks Roman!
> One comment in-line.
> 
> 2017-11-30 13:42 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>
> > ---
> >  tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 5aa45f89da93..9f2410beaa18 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -1721,6 +1721,41 @@ 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);
> >  
> > +static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
> > +{
> > +	if (!prog->section_name)
> > +		goto err;
> > +
> > +	if (strncmp(prog->section_name, "socket", 6) == 0)
> > +		return BPF_PROG_TYPE_SOCKET_FILTER;
> > +	if (strncmp(prog->section_name, "kprobe/", 7) == 0)
> > +		return BPF_PROG_TYPE_KPROBE;
> > +	if (strncmp(prog->section_name, "kretprobe/", 10) == 0)
> > +		return BPF_PROG_TYPE_KPROBE;
> > +	if (strncmp(prog->section_name, "tracepoint/", 11) == 0)
> > +		return BPF_PROG_TYPE_TRACEPOINT;
> > +	if (strncmp(prog->section_name, "xdp", 3) == 0)
> > +		return BPF_PROG_TYPE_XDP;
> > +	if (strncmp(prog->section_name, "perf_event", 10) == 0)
> > +		return BPF_PROG_TYPE_PERF_EVENT;
> > +	if (strncmp(prog->section_name, "cgroup/skb", 10) == 0)
> > +		return BPF_PROG_TYPE_CGROUP_SKB;
> > +	if (strncmp(prog->section_name, "cgroup/sock", 11) == 0)
> > +		return BPF_PROG_TYPE_CGROUP_SOCK;
> > +	if (strncmp(prog->section_name, "cgroup/dev", 10) == 0)
> > +		return BPF_PROG_TYPE_CGROUP_DEVICE;
> > +	if (strncmp(prog->section_name, "sockops", 7) == 0)
> > +		return BPF_PROG_TYPE_SOCK_OPS;
> > +	if (strncmp(prog->section_name, "sk_skb", 6) == 0)
> > +		return BPF_PROG_TYPE_SK_SKB;  
> 
> I do not really like these hard-coded lengths, maybe we could work out
> something nicer with a bit of pre-processing work? Perhaps something like:
> 
> #define SOCKET_FILTER_SEC_PREFIX "socket"
> #define KPROBE_SEC_PREFIX "kprobe/"
> […]
> 
> #define TRY_TYPE(string, __TYPE)				\
> 	do {							\
> 		if (!strncmp(string, __TYPE ## _SEC_PREFIX,	\
> 			     sizeof(__TYPE ## _SEC_PREFIX)))	\
> 			return BPF_PROG_TYPE_ ## __TYPE;	\
> 	} while(0);

I like the suggestion, but I think return and goto statements hiding
inside macros are slightly frowned upon in the netdev.  Perhaps just 
a macro that wraps the strncmp() with sizeof would be enough?  Without
the return inside?

> static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
> {
> 	if (!prog->section_name)
> 		goto err;
> 
> 	TRY_TYPE(prog->section_name, SOCKET_FILTER);
> 	TRY_TYPE(prog->section_name, KPROBE);
> 	[…]
> 
> err:
> 	pr_warning("…",
> 	           prog->section_name);
> 
> 	return BPF_PROG_TYPE_UNSPEC;
> }

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

[Roman Gushchin]

On Fri, Dec 01, 2017 at 02:46:06PM -0800, Jakub Kicinski wrote:
> On Fri, 1 Dec 2017 10:22:57 +0000, Quentin Monnet wrote:
> > Thanks Roman!
> > One comment in-line.
> > 
> > 2017-11-30 13:42 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>
> > > ---
> > >  tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 47 insertions(+)
> > > 
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 5aa45f89da93..9f2410beaa18 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -1721,6 +1721,41 @@ 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);
> > >  
> > > +static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
> > > +{
> > > +	if (!prog->section_name)
> > > +		goto err;
> > > +
> > > +	if (strncmp(prog->section_name, "socket", 6) == 0)
> > > +		return BPF_PROG_TYPE_SOCKET_FILTER;
> > > +	if (strncmp(prog->section_name, "kprobe/", 7) == 0)
> > > +		return BPF_PROG_TYPE_KPROBE;
> > > +	if (strncmp(prog->section_name, "kretprobe/", 10) == 0)
> > > +		return BPF_PROG_TYPE_KPROBE;
> > > +	if (strncmp(prog->section_name, "tracepoint/", 11) == 0)
> > > +		return BPF_PROG_TYPE_TRACEPOINT;
> > > +	if (strncmp(prog->section_name, "xdp", 3) == 0)
> > > +		return BPF_PROG_TYPE_XDP;
> > > +	if (strncmp(prog->section_name, "perf_event", 10) == 0)
> > > +		return BPF_PROG_TYPE_PERF_EVENT;
> > > +	if (strncmp(prog->section_name, "cgroup/skb", 10) == 0)
> > > +		return BPF_PROG_TYPE_CGROUP_SKB;
> > > +	if (strncmp(prog->section_name, "cgroup/sock", 11) == 0)
> > > +		return BPF_PROG_TYPE_CGROUP_SOCK;
> > > +	if (strncmp(prog->section_name, "cgroup/dev", 10) == 0)
> > > +		return BPF_PROG_TYPE_CGROUP_DEVICE;
> > > +	if (strncmp(prog->section_name, "sockops", 7) == 0)
> > > +		return BPF_PROG_TYPE_SOCK_OPS;
> > > +	if (strncmp(prog->section_name, "sk_skb", 6) == 0)
> > > +		return BPF_PROG_TYPE_SK_SKB;  
> > 
> > I do not really like these hard-coded lengths, maybe we could work out
> > something nicer with a bit of pre-processing work? Perhaps something like:
> > 
> > #define SOCKET_FILTER_SEC_PREFIX "socket"
> > #define KPROBE_SEC_PREFIX "kprobe/"
> > […]
> > 
> > #define TRY_TYPE(string, __TYPE)				\
> > 	do {							\
> > 		if (!strncmp(string, __TYPE ## _SEC_PREFIX,	\
> > 			     sizeof(__TYPE ## _SEC_PREFIX)))	\
> > 			return BPF_PROG_TYPE_ ## __TYPE;	\
> > 	} while(0);
> 
> I like the suggestion, but I think return and goto statements hiding
> inside macros are slightly frowned upon in the netdev.  Perhaps just 
> a macro that wraps the strncmp() with sizeof would be enough?  Without
> the return inside?

Hm, I'm not sure that using macroses here makes the code much easier to read.
Maybe we can use just strcmp() instead?
As we compare with hardcoded strings, there is no real difference.
Something like this:

if (!strcmp("socket", prog->section_name))
	return BPF_PROG_TYPE_SOCKET_FILTER;
if (!strcmp("kprobe/", prog->section_name))
	return BPF_PROG_TYPE_KPROBE;
if (!strcmp("kretprobe/", prog->section_name))
	return BPF_PROG_TYPE_KPROBE;
if (!strcmp("tracepoint/", prog->section_name))
	return BPF_PROG_TYPE_TRACEPOINT;
if (!strcmp("xdp", prog->section_name))
	return BPF_PROG_TYPE_XDP;
if (!strcmp("perf_event", prog->section_name))
	return BPF_PROG_TYPE_PERF_EVENT;
if (!strcmp("cgroup/skb", prog->section_name))
	return BPF_PROG_TYPE_CGROUP_SKB;
if (!strcmp("cgroup/sock", prog->section_name))
	return BPF_PROG_TYPE_CGROUP_SOCK;
if (!strcmp("cgroup/dev", prog->section_name))
	return BPF_PROG_TYPE_CGROUP_DEVICE;
if (!strcmp("sockops", prog->section_name))
	return BPF_PROG_TYPE_SOCK_OPS;
if (!strcmp("sk_skb", prog->section_name))
	return BPF_PROG_TYPE_SK_SKB;

Thanks!

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

[Quentin Monnet]

2017-12-04 12:34 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> On Fri, Dec 01, 2017 at 02:46:06PM -0800, Jakub Kicinski wrote:
>> On Fri, 1 Dec 2017 10:22:57 +0000, Quentin Monnet wrote:
>>> Thanks Roman!
>>> One comment in-line.
>>>
>>> 2017-11-30 13:42 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>
>>>> ---
>>>>  tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 47 insertions(+)
>>>>
>>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>>> index 5aa45f89da93..9f2410beaa18 100644
>>>> --- a/tools/lib/bpf/libbpf.c
>>>> +++ b/tools/lib/bpf/libbpf.c
>>>> @@ -1721,6 +1721,41 @@ 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);
>>>>  
>>>> +static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
>>>> +{
>>>> +	if (!prog->section_name)
>>>> +		goto err;
>>>> +
>>>> +	if (strncmp(prog->section_name, "socket", 6) == 0)
>>>> +		return BPF_PROG_TYPE_SOCKET_FILTER;
>>>> +	if (strncmp(prog->section_name, "kprobe/", 7) == 0)
>>>> +		return BPF_PROG_TYPE_KPROBE;
>>>> +	if (strncmp(prog->section_name, "kretprobe/", 10) == 0)
>>>> +		return BPF_PROG_TYPE_KPROBE;
>>>> +	if (strncmp(prog->section_name, "tracepoint/", 11) == 0)
>>>> +		return BPF_PROG_TYPE_TRACEPOINT;
>>>> +	if (strncmp(prog->section_name, "xdp", 3) == 0)
>>>> +		return BPF_PROG_TYPE_XDP;
>>>> +	if (strncmp(prog->section_name, "perf_event", 10) == 0)
>>>> +		return BPF_PROG_TYPE_PERF_EVENT;
>>>> +	if (strncmp(prog->section_name, "cgroup/skb", 10) == 0)
>>>> +		return BPF_PROG_TYPE_CGROUP_SKB;
>>>> +	if (strncmp(prog->section_name, "cgroup/sock", 11) == 0)
>>>> +		return BPF_PROG_TYPE_CGROUP_SOCK;
>>>> +	if (strncmp(prog->section_name, "cgroup/dev", 10) == 0)
>>>> +		return BPF_PROG_TYPE_CGROUP_DEVICE;
>>>> +	if (strncmp(prog->section_name, "sockops", 7) == 0)
>>>> +		return BPF_PROG_TYPE_SOCK_OPS;
>>>> +	if (strncmp(prog->section_name, "sk_skb", 6) == 0)
>>>> +		return BPF_PROG_TYPE_SK_SKB;  
>>>
>>> I do not really like these hard-coded lengths, maybe we could work out
>>> something nicer with a bit of pre-processing work? Perhaps something like:
>>>
>>> #define SOCKET_FILTER_SEC_PREFIX "socket"
>>> #define KPROBE_SEC_PREFIX "kprobe/"
>>> […]
>>>
>>> #define TRY_TYPE(string, __TYPE)				\
>>> 	do {							\
>>> 		if (!strncmp(string, __TYPE ## _SEC_PREFIX,	\
>>> 			     sizeof(__TYPE ## _SEC_PREFIX)))	\
>>> 			return BPF_PROG_TYPE_ ## __TYPE;	\
>>> 	} while(0);
>>
>> I like the suggestion, but I think return and goto statements hiding
>> inside macros are slightly frowned upon in the netdev.  Perhaps just 
>> a macro that wraps the strncmp() with sizeof would be enough?  Without
>> the return inside?
> 
> Hm, I'm not sure that using macroses here makes the code much easier to read.
> Maybe we can use just strcmp() instead?
> As we compare with hardcoded strings, there is no real difference.
> Something like this:
> 
> if (!strcmp("socket", prog->section_name))
> 	return BPF_PROG_TYPE_SOCKET_FILTER;
> if (!strcmp("kprobe/", prog->section_name))
> 	return BPF_PROG_TYPE_KPROBE;
> if (!strcmp("kretprobe/", prog->section_name))
> 	return BPF_PROG_TYPE_KPROBE;
> if (!strcmp("tracepoint/", prog->section_name))
> 	return BPF_PROG_TYPE_TRACEPOINT;
> if (!strcmp("xdp", prog->section_name))
> 	return BPF_PROG_TYPE_XDP;
> if (!strcmp("perf_event", prog->section_name))
> 	return BPF_PROG_TYPE_PERF_EVENT;
> if (!strcmp("cgroup/skb", prog->section_name))
> 	return BPF_PROG_TYPE_CGROUP_SKB;
> if (!strcmp("cgroup/sock", prog->section_name))
> 	return BPF_PROG_TYPE_CGROUP_SOCK;
> if (!strcmp("cgroup/dev", prog->section_name))
> 	return BPF_PROG_TYPE_CGROUP_DEVICE;
> if (!strcmp("sockops", prog->section_name))
> 	return BPF_PROG_TYPE_SOCK_OPS;
> if (!strcmp("sk_skb", prog->section_name))
> 	return BPF_PROG_TYPE_SK_SKB;
> 
> Thanks!
> 

I do not believe this works. For kprobes for example, the idea was to
compare "kprobe/" with only the beginning of prog->section_name, right?
The string prog->section_name is supposed to be longer than this (e.g.
"kprobe/sys_write"), and strcmp() will not detect a match in this case.

Quentin

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

[Roman Gushchin]

On Mon, Dec 04, 2017 at 01:12:33PM +0000, Quentin Monnet wrote:
> 2017-12-04 12:34 UTC+0000 ~ Roman Gushchin <guro@fb.com>
> > On Fri, Dec 01, 2017 at 02:46:06PM -0800, Jakub Kicinski wrote:
> >> On Fri, 1 Dec 2017 10:22:57 +0000, Quentin Monnet wrote:
> >>> Thanks Roman!
> >>> One comment in-line.
> >>>
> >>> 2017-11-30 13:42 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>
> >>>> ---
> >>>>  tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 47 insertions(+)
> >>>>
> >>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >>>> index 5aa45f89da93..9f2410beaa18 100644
> >>>> --- a/tools/lib/bpf/libbpf.c
> >>>> +++ b/tools/lib/bpf/libbpf.c
> >>>> @@ -1721,6 +1721,41 @@ 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);
> >>>>  
> >>>> +static enum bpf_prog_type bpf_program__guess_type(struct bpf_program *prog)
> >>>> +{
> >>>> +	if (!prog->section_name)
> >>>> +		goto err;
> >>>> +
> >>>> +	if (strncmp(prog->section_name, "socket", 6) == 0)
> >>>> +		return BPF_PROG_TYPE_SOCKET_FILTER;
> >>>> +	if (strncmp(prog->section_name, "kprobe/", 7) == 0)
> >>>> +		return BPF_PROG_TYPE_KPROBE;
> >>>> +	if (strncmp(prog->section_name, "kretprobe/", 10) == 0)
> >>>> +		return BPF_PROG_TYPE_KPROBE;
> >>>> +	if (strncmp(prog->section_name, "tracepoint/", 11) == 0)
> >>>> +		return BPF_PROG_TYPE_TRACEPOINT;
> >>>> +	if (strncmp(prog->section_name, "xdp", 3) == 0)
> >>>> +		return BPF_PROG_TYPE_XDP;
> >>>> +	if (strncmp(prog->section_name, "perf_event", 10) == 0)
> >>>> +		return BPF_PROG_TYPE_PERF_EVENT;
> >>>> +	if (strncmp(prog->section_name, "cgroup/skb", 10) == 0)
> >>>> +		return BPF_PROG_TYPE_CGROUP_SKB;
> >>>> +	if (strncmp(prog->section_name, "cgroup/sock", 11) == 0)
> >>>> +		return BPF_PROG_TYPE_CGROUP_SOCK;
> >>>> +	if (strncmp(prog->section_name, "cgroup/dev", 10) == 0)
> >>>> +		return BPF_PROG_TYPE_CGROUP_DEVICE;
> >>>> +	if (strncmp(prog->section_name, "sockops", 7) == 0)
> >>>> +		return BPF_PROG_TYPE_SOCK_OPS;
> >>>> +	if (strncmp(prog->section_name, "sk_skb", 6) == 0)
> >>>> +		return BPF_PROG_TYPE_SK_SKB;  
> >>>
> >>> I do not really like these hard-coded lengths, maybe we could work out
> >>> something nicer with a bit of pre-processing work? Perhaps something like:
> >>>
> >>> #define SOCKET_FILTER_SEC_PREFIX "socket"
> >>> #define KPROBE_SEC_PREFIX "kprobe/"
> >>> […]
> >>>
> >>> #define TRY_TYPE(string, __TYPE)				\
> >>> 	do {							\
> >>> 		if (!strncmp(string, __TYPE ## _SEC_PREFIX,	\
> >>> 			     sizeof(__TYPE ## _SEC_PREFIX)))	\
> >>> 			return BPF_PROG_TYPE_ ## __TYPE;	\
> >>> 	} while(0);
> >>
> >> I like the suggestion, but I think return and goto statements hiding
> >> inside macros are slightly frowned upon in the netdev.  Perhaps just 
> >> a macro that wraps the strncmp() with sizeof would be enough?  Without
> >> the return inside?
> > 
> > Hm, I'm not sure that using macroses here makes the code much easier to read.
> > Maybe we can use just strcmp() instead?
> > As we compare with hardcoded strings, there is no real difference.
> > Something like this:
> > 
> > if (!strcmp("socket", prog->section_name))
> > 	return BPF_PROG_TYPE_SOCKET_FILTER;
> > if (!strcmp("kprobe/", prog->section_name))
> > 	return BPF_PROG_TYPE_KPROBE;
> > if (!strcmp("kretprobe/", prog->section_name))
> > 	return BPF_PROG_TYPE_KPROBE;
> > if (!strcmp("tracepoint/", prog->section_name))
> > 	return BPF_PROG_TYPE_TRACEPOINT;
> > if (!strcmp("xdp", prog->section_name))
> > 	return BPF_PROG_TYPE_XDP;
> > if (!strcmp("perf_event", prog->section_name))
> > 	return BPF_PROG_TYPE_PERF_EVENT;
> > if (!strcmp("cgroup/skb", prog->section_name))
> > 	return BPF_PROG_TYPE_CGROUP_SKB;
> > if (!strcmp("cgroup/sock", prog->section_name))
> > 	return BPF_PROG_TYPE_CGROUP_SOCK;
> > if (!strcmp("cgroup/dev", prog->section_name))
> > 	return BPF_PROG_TYPE_CGROUP_DEVICE;
> > if (!strcmp("sockops", prog->section_name))
> > 	return BPF_PROG_TYPE_SOCK_OPS;
> > if (!strcmp("sk_skb", prog->section_name))
> > 	return BPF_PROG_TYPE_SK_SKB;
> > 
> > Thanks!
> > 
> 
> I do not believe this works. For kprobes for example, the idea was to
> compare "kprobe/" with only the beginning of prog->section_name, right?
> The string prog->section_name is supposed to be longer than this (e.g.
> "kprobe/sys_write"), and strcmp() will not detect a match in this case.

Ah, true, I've missed this part.
Ok, I'll try to master something with macroses.

Thanks!