Skip to contentSkip to conversation

bpftool: cgroup bpf operations

PATCH Superseded Roman Gushchin 3 participants 7 messages Dec 8, 2017 – Dec 9, 2017

Subsystems (5)
Networking Network drivers BPF libbpf +1 more subsystems

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Roman Gushchin

author
#0 lore raw

v3 cover bpftool: cgroup bpf operations

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.

v3:
- SRC replaced with OBJ in prog load docs
- Output unknown attach type in hex
- License header in SPDX format
- Minor style fixes (e.g. variable reordering)

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                         | 300 +++++++++++++++++++++
 tools/bpf/bpftool/common.c                         |  71 ++---
 tools/bpf/bpftool/main.c                           |   3 +-
 tools/bpf/bpftool/main.h                           |   2 +
 tools/bpf/bpftool/prog.c                           |  29 +-
 tools/lib/bpf/libbpf.c                             |  53 ++++
 10 files changed, 533 insertions(+), 39 deletions(-)
 create mode 100644 tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
 create mode 100644 tools/bpf/bpftool/cgroup.c
Signature · 1 line
2.14.3
author
#1 lore raw

PATCH v3 1/4 libbpf: add ability to guess program type based on section name

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 <[email protected]>
Cc Alexei Starovoitov <[email protected]>
Cc Daniel Borkmann <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Martin KaFai Lau <[email protected]>
Cc Quentin Monnet <[email protected]>
Cc David Ahern <[email protected]>
---
 tools/lib/bpf/libbpf.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
tools/lib/bpf/libbpf.c +51 -0
index 5aa45f89da93..205b7822fa0a 100644
@@ -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) {
Signature · 1 line
2.14.3
author
#2 lore raw

PATCH v3 2/4 libbpf: prefer global symbols as bpf program name source

In reply to 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 <[email protected]>
Cc Alexei Starovoitov <[email protected]>
Cc Daniel Borkmann <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Martin KaFai Lau <[email protected]>
Cc Quentin Monnet <[email protected]>
Cc David Ahern <[email protected]>
---
 tools/lib/bpf/libbpf.c | 2 ++
 1 file changed, 2 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 205b7822fa0a..65d0d0aff4fa 100644
@@ -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,
Signature · 1 line
2.14.3
author
#3 lore raw

PATCH v3 3/4 bpftool: implement prog load command

In reply to 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 OBJ FILE

FILE is a non-existing file on bpffs.

Signed-off-by Roman Gushchin <[email protected]>
Cc Alexei Starovoitov <[email protected]>
Cc Daniel Borkmann <[email protected]>
Reviewed-by Jakub Kicinski <[email protected]>
Cc Martin KaFai Lau <[email protected]>
Cc Quentin Monnet <[email protected]>
Cc David Ahern <[email protected]>
---
 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                         | 29 +++++++++-
 5 files changed, 79 insertions(+), 34 deletions(-)
tools/bpf/bpftool/Documentation/bpftool-prog.rst +9 -1
index 36e8d1c3c40d..ffdb20e8280f 100644
@@ -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** *OBJ* *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** *OBJ* *FILE*
+		  Load bpf program from binary *OBJ* 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 Jul 22 01:44 prog2
 
 **# bpftool prog dum jited pinned /sys/fs/bpf/prog opcodes**
tools/bpf/bpftool/Documentation/bpftool.rst +1 -1
index 926c03d5a8da..f547a0c0aa34 100644
@@ -26,7 +26,7 @@ SYNOPSIS
 	| **pin** | **help** }
 
 	*PROG-COMMANDS* := { **show** | **dump jited** | **dump xlated** | **pin**
-	| **help** }
+	| **load** | **help** }
 
 DESCRIPTION
 ===========
tools/bpf/bpftool/common.c +40 -31

88 lines not on this page · raw message

tools/bpf/bpftool/main.h +1 -0
index bff330b49791..bec1ccbb49c7 100644
@@ -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);
tools/bpf/bpftool/prog.c +28 -1
index ad619b96c276..037484ceaeaf 100644
@@ -45,6 +45,7 @@
 #include <sys/stat.h>
 
 #include <bpf.h>
+#include <libbpf.h>
 
 #include "main.h"
 #include "disasm.h"

@@ -635,6 +636,30 @@ 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();
+
+	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 +672,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  OBJ  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 +689,7 @@ static const struct cmd cmds[] = {
 	{ "help",	do_help },
 	{ "dump",	do_dump },
 	{ "pin",	do_pin },
+	{ "load",	do_load },
 	{ 0 }
 };
Signature · 1 line
2.14.3
author
#4 lore raw

PATCH v3 4/4 bpftool: implement cgroup bpf operations

In reply to 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 <[email protected]>
Cc Alexei Starovoitov <[email protected]>
Cc Daniel Borkmann <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Martin KaFai Lau <[email protected]>
Cc Quentin Monnet <[email protected]>
Reviewed-by David Ahern <[email protected]>
---
 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                         | 300 +++++++++++++++++++++
 tools/bpf/bpftool/main.c                           |   3 +-
 tools/bpf/bpftool/main.h                           |   1 +
 7 files changed, 401 insertions(+), 5 deletions(-)
 create mode 100644 tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
 create mode 100644 tools/bpf/bpftool/cgroup.c
tools/bpf/bpftool/Documentation/bpftool-cgroup.rst new file +92 -0
new file mode 100644
index 000000000000..61ded613aee1
@@ -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)
tools/bpf/bpftool/Documentation/bpftool-map.rst +1 -1
index 9f51a268eb06..421cabc417e6 100644
@@ -128,4 +128,4 @@ EXAMPLES
 
 SEE ALSO
 ========
-	**bpftool**\ (8), **bpftool-prog**\ (8)
+	**bpftool**\ (8), **bpftool-prog**\ (8), **bpftool-cgroup**\ (8)
tools/bpf/bpftool/Documentation/bpftool-prog.rst +1 -1
index ffdb20e8280f..81c97c0e9b67 100644
@@ -155,4 +155,4 @@ EXAMPLES
 
 SEE ALSO
 ========
-	**bpftool**\ (8), **bpftool-map**\ (8)
+	**bpftool**\ (8), **bpftool-map**\ (8), **bpftool-cgroup**\ (8)
tools/bpf/bpftool/Documentation/bpftool.rst +4 -2
index f547a0c0aa34..6732a5a617e4 100644
@@ -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)
tools/bpf/bpftool/cgroup.c new file +300 -0

300 lines not on this page · raw message

tools/bpf/bpftool/main.c +2 -1
index d294bc8168be..ecd53ccf1239 100644
@@ -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 }
 };
tools/bpf/bpftool/main.h +1 -0
index bec1ccbb49c7..8f6d3cac0347 100644
@@ -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);
Signature · 1 line
2.14.3
#5 lore raw

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

Quoted 9 lines · Roman Gushchin
tools/bpf/bpftool/cgroup.c new file +9 -0
+static int list_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
+{
+	__u32 prog_ids[1024] = {0};
+	char *attach_flags_str;
+	__u32 prog_cnt, iter;
+	__u32 attach_flags;
+	char buf[16];
+	int ret;
+

...

tools/bpf/bpftool/cgroup.c line 104

Quoted 9 lines
tools/bpf/bpftool/cgroup.c new file +9 -0
+	case BPF_F_ALLOW_OVERRIDE:
+		attach_flags_str = "allow_override";
+		break;
+	case 0:
+		attach_flags_str = "";
+		break;
+	default:
+		snprintf(buf, sizeof(buf), "unknown(%x)", attach_flags);
+		attach_flags_str = buf;

nit: theoretically speaking strlen("unknown()") == 9 + 8 + 1 > sizeof(buf)
so if we ever use all flags this may get truncated. I personally also like using %x without 0x in front, but I restrained myself in bpftool to avoid potential confusion (unknown(10) could be 10 or 16). Map flags do put the prefix in, perhaps it would be good to keep those consistent?

Quoted 7 lines
tools/bpf/bpftool/cgroup.c new file +7 -0
+	}
+
+	for (iter = 0; iter < prog_cnt; iter++)
+		list_bpf_prog(prog_ids[iter], attach_type_strings[type],
+			      attach_flags_str);
+
+	return 0;

tools/bpf/bpftool/cgroup.c line 165

Quoted 10 lines
tools/bpf/bpftool/cgroup.c new file +8 -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;

nit: I was hoping you'd fix the order of variables in all functions..

Quoted 18 lines
tools/bpf/bpftool/cgroup.c new file +18 -0
+	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;

tools/bpf/bpftool/cgroup.c line 194

Quoted 10 lines
tools/bpf/bpftool/cgroup.c new file +10 -0
+	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;

I don't feel about this strongly but as I said I was trying to follow iproute2's conventions, and it allows aliasing. So if you type "ip a" it will give you the first thing that starts with a, not necessarily alphabetically, more likely in order of usefulness or order in which things were added. IOW if "allow_" selects "allow_mutli" that's what I would actually expect it to do..

Maybe others disagree?

Quoted 19 lines
tools/bpf/bpftool/cgroup.c new file +19 -0
+		} 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:

tools/bpf/bpftool/cgroup.c line 223

Quoted 10 lines
tools/bpf/bpftool/cgroup.c new file +10 -0
+	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;

nit: order here too..

Quoted 40 lines
tools/bpf/bpftool/cgroup.c new file +40 -0
+	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 detach 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;
+}
#6 lore raw

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

Quoted 19 lines · Jakub Kicinski

tools/bpf/bpftool/cgroup.c line 194

Quoted 11 lines
tools/bpf/bpftool/cgroup.c new file +11 -0
+	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;

I don't feel about this strongly but as I said I was trying to follow iproute2's conventions, and it allows aliasing. So if you type "ip a" it will give you the first thing that starts with a, not necessarily alphabetically, more likely in order of usefulness or order in which things were added. IOW if "allow_" selects "allow_mutli" that's what I would actually expect it to do..

Quoted 1 line

Maybe others disagree?

The iproute2 syntax is very user friendly, and I agree with following the conventions.

With respect to the attach flags, allow_yyyyy is a lot to type, but having 'a .. allow_' mean one flag over the other is going to be confusing. Perhaps dropping the 'allow_' prefix in favor of just 'multi' and 'override' and doing prefix match on it? User commands do not need to follow flag names precisely.