Skip to contentSkip to conversation

bpftool: support loading flow dissector

PATCH Superseded Stanislav Fomichev 3 participants 15 messages Nov 7, 2018 – Nov 8, 2018

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

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Stanislav Fomichev

author
#0 lore raw

v2 cover bpftool: support loading flow dissector

v2 changes:
* addressed comments/style issues from Jakub Kicinski & Quentin Monnet
* removed logic that populates jump table
* added cleanup for partial failure in bpf_object__pin

This patch series adds support for loading and attaching flow dissector programs from the bpftool:

* first patch fixes flow dissector section name in the selftests (so libbpf auto-detection works)
* second patch adds proper cleanup to bpf_object__pin which is now being used to attach all flow dissector progs/maps
* third patch adds actual support to the bpftool

See third patch for the description/details.

Stanislav Fomichev (3):
selftests/bpf: rename flow dissector section to flow_dissector
libbpf: cleanup after partial failure in bpf_object__pin
bpftool: support loading flow dissector

 .../bpftool/Documentation/bpftool-prog.rst    | 26 +++--
 tools/bpf/bpftool/bash-completion/bpftool     |  2 +-
 tools/bpf/bpftool/common.c                    | 30 +++---
 tools/bpf/bpftool/main.h                      |  1 +
 tools/bpf/bpftool/prog.c                      | 94 ++++++++++++++-----
 tools/lib/bpf/libbpf.c                        | 58 ++++++++++--
 tools/testing/selftests/bpf/bpf_flow.c        |  2 +-
 .../selftests/bpf/test_flow_dissector.sh      |  2 +-
 8 files changed, 151 insertions(+), 64 deletions(-)
Signature · 1 line
2.19.1.930.g4563a0d9d0-goog
author
#1 lore raw

PATCH v2 1/3 selftests/bpf: rename flow dissector section to flow_dissector

Makes it compatible with the logic that derives program type from section name in libbpf_prog_type_by_name.

Signed-off-by Stanislav Fomichev <sdf at google.com>
---
 tools/testing/selftests/bpf/bpf_flow.c             | 2 +-
 tools/testing/selftests/bpf/test_flow_dissector.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
tools/testing/selftests/bpf/bpf_flow.c +1 -1
index 107350a7821d..b9798f558ca7 100644
@@ -116,7 +116,7 @@ static __always_inline int parse_eth_proto(struct __sk_buff *skb, __be16 proto)
 	return BPF_DROP;
 }
 
-SEC("dissect")
+SEC("flow_dissector")
 int _dissect(struct __sk_buff *skb)
 {
 	if (!skb->vlan_present)
tools/testing/selftests/bpf/test_flow_dissector.sh +1 -1
index c0fb073b5eab..d23d4da66b83 100755
@@ -59,7 +59,7 @@ else
 fi
 
 # Attach BPF program
-./flow_dissector_load -p bpf_flow.o -s dissect
+./flow_dissector_load -p bpf_flow.o -s flow_dissector
 
 # Setup
 tc qdisc add dev lo ingress
Signature · 1 line
2.19.1.930.g4563a0d9d0-goog
author
#2 lore raw

PATCH v2 2/3 libbpf: cleanup after partial failure in bpf_object__pin

In reply to Stanislav Fomichev

bpftool will use bpf_object__pin in the next commit to pin all programs and maps from the file; in case of a partial failure, we need to get back to the clean state (undo previous program/map pins).

Signed-off-by Stanislav Fomichev <sdf at google.com>
---
 tools/lib/bpf/libbpf.c | 58 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 10 deletions(-)
tools/lib/bpf/libbpf.c +48 -10
index d6e62e90e8d4..309abe7196f3 100644
@@ -1803,14 +1803,17 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 
 		len = snprintf(buf, PATH_MAX, "%s/%s", path,
 			       bpf_map__name(map));
-		if (len < 0)
-			return -EINVAL;
-		else if (len >= PATH_MAX)
-			return -ENAMETOOLONG;
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin_maps;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin_maps;
+		}
 
 		err = bpf_map__pin(map, buf);
 		if (err)
-			return err;
+			goto err_unpin_maps;
 	}
 
 	bpf_object__for_each_program(prog, obj) {

@@ -1819,17 +1822,52 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 
 		len = snprintf(buf, PATH_MAX, "%s/%s", path,
 			       prog->section_name);
-		if (len < 0)
-			return -EINVAL;
-		else if (len >= PATH_MAX)
-			return -ENAMETOOLONG;
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin_programs;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin_programs;
+		}
 
 		err = bpf_program__pin(prog, buf);
 		if (err)
-			return err;
+			goto err_unpin_programs;
 	}
 
 	return 0;
+
+err_unpin_programs:
+	bpf_object__for_each_program(prog, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       prog->section_name);
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		unlink(buf);
+	}
+
+err_unpin_maps:
+	bpf_map__for_each(map, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       bpf_map__name(map));
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		unlink(buf);
+	}
+
+	return err;
 }
 
 void bpf_object__close(struct bpf_object *obj)
Signature · 1 line
2.19.1.930.g4563a0d9d0-goog
#3 lore raw

Re: [PATCH bpf-next 2/3] libbpf: cleanup after partial failure in bpf_object__pin

Quoted 63 lines · Stanislav Fomichev

bpftool will use bpf_object__pin in the next commit to pin all programs and maps from the file; in case of a partial failure, we need to get back to the clean state (undo previous program/map pins).

Signed-off-by Stanislav Fomichev <sdf at google.com>
---
 tools/lib/bpf/libbpf.c | 58 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 10 deletions(-)
tools/lib/bpf/libbpf.c +20 -10
index d6e62e90e8d4..309abe7196f3 100644
@@ -1803,14 +1803,17 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 
 		len = snprintf(buf, PATH_MAX, "%s/%s", path,
 			       bpf_map__name(map));
-		if (len < 0)
-			return -EINVAL;
-		else if (len >= PATH_MAX)
-			return -ENAMETOOLONG;
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin_maps;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin_maps;
+		}
 
 		err = bpf_map__pin(map, buf);
 		if (err)
-			return err;
+			goto err_unpin_maps;
 	}
 
 	bpf_object__for_each_program(prog, obj) {

@@ -1819,17 +1822,52 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 
 		len = snprintf(buf, PATH_MAX, "%s/%s", path,
 			       prog->section_name);
-		if (len < 0)
-			return -EINVAL;
-		else if (len >= PATH_MAX)
-			return -ENAMETOOLONG;
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin_programs;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin_programs;
+		}
 
 		err = bpf_program__pin(prog, buf);
 		if (err)
-			return err;
+			goto err_unpin_programs;
 	}
 
 	return 0;
+
+err_unpin_programs:
+	bpf_object__for_each_program(prog, obj) {
+		char buf[PATH_MAX];

tools/lib/bpf/libbpf.c int bpf_object__pin(struct bpf_object *obj, const char *path) · line 1852

Quoted 10 lines
tools/lib/bpf/libbpf.c +10 -0
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       prog->section_name);
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		unlink(buf);

I think that's no bueno, if pin failed because the file already exists you'll now remove that already existing file.

Quoted 21 lines
tools/lib/bpf/libbpf.c +18 -0
+	}
+
+err_unpin_maps:
+	bpf_map__for_each(map, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       bpf_map__name(map));
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		unlink(buf);
+	}
+
+	return err;
 }
 
 void bpf_object__close(struct bpf_object *obj)
#4 lore raw

Re: [PATCH bpf-next 2/3] libbpf: cleanup after partial failure in bpf_object__pin

Quoted 69 lines · Jakub Kicinski
Quoted 68 lines · Stanislav Fomichev

bpftool will use bpf_object__pin in the next commit to pin all programs and maps from the file; in case of a partial failure, we need to get back to the clean state (undo previous program/map pins).

Signed-off-by Stanislav Fomichev <sdf at google.com>
---
 tools/lib/bpf/libbpf.c | 58 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 10 deletions(-)
tools/lib/bpf/libbpf.c +25 -10
index d6e62e90e8d4..309abe7196f3 100644
@@ -1803,14 +1803,17 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 
 		len = snprintf(buf, PATH_MAX, "%s/%s", path,
 			       bpf_map__name(map));
-		if (len < 0)
-			return -EINVAL;
-		else if (len >= PATH_MAX)
-			return -ENAMETOOLONG;
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin_maps;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin_maps;
+		}
 
 		err = bpf_map__pin(map, buf);
 		if (err)
-			return err;
+			goto err_unpin_maps;
 	}
 
 	bpf_object__for_each_program(prog, obj) {

@@ -1819,17 +1822,52 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
 
 		len = snprintf(buf, PATH_MAX, "%s/%s", path,
 			       prog->section_name);
-		if (len < 0)
-			return -EINVAL;
-		else if (len >= PATH_MAX)
-			return -ENAMETOOLONG;
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin_programs;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin_programs;
+		}
 
 		err = bpf_program__pin(prog, buf);
 		if (err)
-			return err;
+			goto err_unpin_programs;
 	}
 
 	return 0;
+
+err_unpin_programs:
+	bpf_object__for_each_program(prog, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       prog->section_name);
+		if (len < 0)
Quoted 8 lines

tools/lib/bpf/libbpf.c int bpf_object__pin(struct bpf_object *obj, const char *path) · line 1852

Quoted 5 lines
tools/lib/bpf/libbpf.c +5 -0
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		unlink(buf);

I think that's no bueno, if pin failed because the file already exists you'll now remove that already existing file.

How about we check beforehand and bail early if we are going to overwrite something?

Quoted 22 lines
Quoted 21 lines
tools/lib/bpf/libbpf.c +18 -0
+	}
+
+err_unpin_maps:
+	bpf_map__for_each(map, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       bpf_map__name(map));
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		unlink(buf);
+	}
+
+	return err;
 }
 
 void bpf_object__close(struct bpf_object *obj)
#5 lore raw

Re: [PATCH bpf-next 2/3] libbpf: cleanup after partial failure in bpf_object__pin

Quoted 17 lines · Stanislav Fomichev
Quoted 8 lines
Quoted 8 lines
tools/lib/bpf/libbpf.c +8 -0
+err_unpin_programs:
+	bpf_object__for_each_program(prog, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       prog->section_name);
+		if (len < 0)
Quoted 8 lines

tools/lib/bpf/libbpf.c int bpf_object__pin(struct bpf_object *obj, const char *path) · line 1852

Quoted 5 lines
tools/lib/bpf/libbpf.c +5 -0
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		unlink(buf);

I think that's no bueno, if pin failed because the file already exists you'll now remove that already existing file.

Quoted 2 lines

How about we check beforehand and bail early if we are going to overwrite something?

Possible, although the most common way to handle situation like this in the kernel is to "continue the iteration in reverse" over the list. I.e. walk the list back. I think the objects are on a double linked list. You may need to add the appropriate foreach macro and helper..

#6 lore raw

Re: [PATCH bpf-next 2/3] libbpf: cleanup after partial failure in bpf_object__pin

Quoted 18 lines · Jakub Kicinski
Quoted 17 lines · Stanislav Fomichev
Quoted 16 lines

tools/lib/bpf/libbpf.c int bpf_object__pin(struct bpf_object *obj, const char *path) · line 1852

Quoted 13 lines
tools/lib/bpf/libbpf.c +13 -0
+err_unpin_programs:
+	bpf_object__for_each_program(prog, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       prog->section_name);
+		if (len < 0)
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		unlink(buf);

I think that's no bueno, if pin failed because the file already exists you'll now remove that already existing file.

Quoted 7 lines
Quoted 2 lines

How about we check beforehand and bail early if we are going to overwrite something?

Possible, although the most common way to handle situation like this in the kernel is to "continue the iteration in reverse" over the list. I.e. walk the list back. I think the objects are on a double linked list. You may need to add the appropriate foreach macro and helper..

That sounds more complicated than just ensuring that the top directory for the pins doesn't exist and then rm -rf it on failure. I'm thinking about copy-pasting rm_rf from perf
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/util.c#n119).
Thoughts?

Btw, current patch won't work because of those /0 added by bpf_program__pin.

#7 lore raw

Re: [PATCH bpf-next 2/3] libbpf: cleanup after partial failure in bpf_object__pin

Quoted 22 lines · Stanislav Fomichev
Quoted 21 lines · Jakub Kicinski
Quoted 19 lines · Stanislav Fomichev
Quoted 8 lines
Quoted 8 lines
tools/lib/bpf/libbpf.c +8 -0
+err_unpin_programs:
+	bpf_object__for_each_program(prog, obj) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path,
+			       prog->section_name);
+		if (len < 0)
Quoted 8 lines

tools/lib/bpf/libbpf.c int bpf_object__pin(struct bpf_object *obj, const char *path) · line 1852

Quoted 5 lines
tools/lib/bpf/libbpf.c +5 -0
+			continue;
+		else if (len >= PATH_MAX)
+			continue;
+
+		unlink(buf);

I think that's no bueno, if pin failed because the file already exists you'll now remove that already existing file.

How about we check beforehand and bail early if we are going to overwrite something?

Quoted 7 lines
Quoted 4 lines

Possible, although the most common way to handle situation like this in the kernel is to "continue the iteration in reverse" over the list. I.e. walk the list back. I think the objects are on a double linked list. You may need to add the appropriate foreach macro and helper..

That sounds more complicated than just ensuring that the top directory for the pins doesn't exist and then rm -rf it on failure.

Why would we require that the directory does not exist? We can check if it exists and then either create or just pin all in an existing one.

I don't think it should be that much effort to write a reverse for loop - it could actually be less LoC than that rm_rf function :)

Quoted 5 lines

I'm thinking about copy-pasting rm_rf from perf
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/util.c#n119).
Thoughts?

Btw, current patch won't work because of those /0 added by bpf_program__pin.

author
#8 lore raw

PATCH v2 3/3 bpftool: support loading flow dissector

In reply to Stanislav Fomichev

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

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

When bpftool load is called with a flow_dissector prog (i.e. when the first section is flow_dissector of 'type flow_dissector' argument is passed), we load and pin all the programs/maps. User is responsible to construct the jump table for the tail calls.

The last argument of bpftool attach is made optional for this use case.

Example:
bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
/sys/fs/bpf/flow type flow_dissector

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 0 0 0 0 \
value pinned /sys/fs/bpf/flow/IP/0

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 1 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6/0

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 2 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6OP/0

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 3 0 0 0 \
value pinned /sys/fs/bpf/flow/IPV6FR/0

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 4 0 0 0 \
value pinned /sys/fs/bpf/flow/MPLS/0

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 5 0 0 0 \
value pinned /sys/fs/bpf/flow/VLAN/0

bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector

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

Signed-off-by Stanislav Fomichev <sdf at google.com>
---
 .../bpftool/Documentation/bpftool-prog.rst    | 26 +++--
 tools/bpf/bpftool/bash-completion/bpftool     |  2 +-
 tools/bpf/bpftool/common.c                    | 30 +++---
 tools/bpf/bpftool/main.h                      |  1 +
 tools/bpf/bpftool/prog.c                      | 94 ++++++++++++++-----
 5 files changed, 101 insertions(+), 52 deletions(-)
tools/bpf/bpftool/Documentation/bpftool-prog.rst +16 -10
index ac4e904b10fb..a7b6934a8ac9 100644
@@ -25,8 +25,8 @@ MAP COMMANDS
 |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes**}]
 |	**bpftool** **prog pin** *PROG* *FILE*
 |	**bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
-|       **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
-|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
+|       **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
+|       **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
 |	**bpftool** **prog help**
 |
 |	*MAP* := { **id** *MAP_ID* | **pinned** *FILE* }

@@ -39,7 +39,9 @@ MAP COMMANDS
 |		**cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** | **cgroup/post_bind6** |
 |		**cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4** | **cgroup/sendmsg6**
 |	}
-|       *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** | **skb_parse** }
+|       *ATTACH_TYPE* := {
+|		**msg_verdict** | **skb_verdict** | **skb_parse** | **flow_dissector**
+|	}
 
 
 DESCRIPTION

@@ -97,13 +99,17 @@ DESCRIPTION
 		  contain a dot character ('.'), which is reserved for future
 		  extensions of *bpffs*.
 
-        **bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
-                  Attach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
-                  to the map *MAP*.
-
-        **bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
-                  Detach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
-                  from the map *MAP*.
+        **bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
+                  Attach bpf program *PROG* (with type specified by
+                  *ATTACH_TYPE*). Most *ATTACH_TYPEs* require a *MAP*
+                  parameter, with the exception of *flow_dissector* which is
+                  attached to current networking name space.
+
+        **bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
+                  Detach bpf program *PROG* (with type specified by
+                  *ATTACH_TYPE*). Most *ATTACH_TYPEs* require a *MAP*
+                  parameter, with the exception of *flow_dissector* which is
+                  detached from the current networking name space.
 
 	**bpftool prog help**
 		  Print short help message.
tools/bpf/bpftool/bash-completion/bpftool +1 -1
index 3f78e6404589..4ab892adfa9f 100644
@@ -299,7 +299,7 @@ _bpftool()
                     fi
 
                     if [[ ${#words[@]} == 6 ]]; then
-                        COMPREPLY=( $( compgen -W "msg_verdict skb_verdict skb_parse" -- "$cur" ) )
+                        COMPREPLY=( $( compgen -W "msg_verdict skb_verdict skb_parse flow_dissector" -- "$cur" ) )
                         return 0
                     fi
tools/bpf/bpftool/common.c +15 -15
index 25af85304ebe..f671a921dec5 100644
@@ -169,34 +169,24 @@ int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type)
 	return fd;
 }
 
-int do_pin_fd(int fd, const char *name)
+int mount_bpffs_for_pin(const char *name)
 {
 	char err_str[ERR_MAX_LEN];
 	char *file;
 	char *dir;
 	int err = 0;
 
-	err = bpf_obj_pin(fd, name);
-	if (!err)
-		goto out;
-
 	file = malloc(strlen(name) + 1);
 	strcpy(file, name);
 	dir = dirname(file);
 
-	if (errno != EPERM || is_bpffs(dir)) {
-		p_err("can't pin the object (%s): %s", name, strerror(errno));
+	if (is_bpffs(dir)) {
+		/* nothing to do if already mounted */
 		goto out_free;
 	}
 
-	/* Attempt to mount bpffs, then retry pinning. */
 	err = mnt_bpffs(dir, err_str, ERR_MAX_LEN);
-	if (!err) {
-		err = bpf_obj_pin(fd, name);
-		if (err)
-			p_err("can't pin the object (%s): %s", name,
-			      strerror(errno));
-	} else {
+	if (err) {
 		err_str[ERR_MAX_LEN - 1] = '\0';
 		p_err("can't mount BPF file system to pin the object (%s): %s",
 		      name, err_str);

@@ -204,10 +194,20 @@ int do_pin_fd(int fd, const char *name)
 
 out_free:
 	free(file);
-out:
 	return err;
 }
 
+int do_pin_fd(int fd, const char *name)
+{
+	int err;
+
+	err = mount_bpffs_for_pin(name);
+	if (err)
+		return err;
+
+	return bpf_obj_pin(fd, name);
+}
+
 int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32))
 {
 	unsigned int id;
tools/bpf/bpftool/main.h +1 -0
index 28322ace2856..1383824c9baf 100644
@@ -129,6 +129,7 @@ const char *get_fd_type_name(enum bpf_obj_type type);
 char *get_fdinfo(int fd, const char *key);
 int open_obj_pinned(char *path);
 int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type);
+int mount_bpffs_for_pin(const char *name);
 int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32));
 int do_pin_fd(int fd, const char *name);
tools/bpf/bpftool/prog.c +68 -26

175 lines not on this page · raw message

Signature · 1 line
2.19.1.930.g4563a0d9d0-goog
#9 lore raw
Quoted 3 lines · Stanislav Fomichev

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 0 0 0 0 \
value pinned /sys/fs/bpf/flow/IP/0

Where is that /0 coming from ? Is that in source code? I don't see libbpf adding it, maybe I'm missing something.

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

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 0 0 0 0 \
value pinned /sys/fs/bpf/flow/IP/0

Where is that /0 coming from ? Is that in source code? I don't see libbpf adding it, maybe I'm missing something.

libbpf adds that, that's a program instance:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n1744

Ugh, I was looking at bpf_object__pin() which uses names :(

We never use this multi-instance thing, and I don't think bpftool ever will, so IMHO it'd be good if we just re-did the pinning loop in bpftool.

#12 lore raw
Quoted 9 lines · Jakub Kicinski
Quoted 8 lines · Stanislav Fomichev
Quoted 7 lines · Jakub Kicinski
Quoted 3 lines · Stanislav Fomichev

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 0 0 0 0 \
value pinned /sys/fs/bpf/flow/IP/0

Where is that /0 coming from ? Is that in source code? I don't see libbpf adding it, maybe I'm missing something.

Quoted 8 lines
Quoted 2 lines

libbpf adds that, that's a program instance:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n1744

Ugh, I was looking at bpf_object__pin() which uses names :(

We never use this multi-instance thing, and I don't think bpftool ever will, so IMHO it'd be good if we just re-did the pinning loop in bpftool.

I wonder whether I should just add special case to bpf_program__pin: don't create a subdir when instances.nr == 1 (and just create a file pin for single instance)? In that case I can continue to use libbpf and don't reinvent the wheel. Any objections?

#13 lore raw
Quoted 15 lines · Stanislav Fomichev
Quoted 14 lines · Jakub Kicinski
Quoted 10 lines · Stanislav Fomichev
Quoted 7 lines · Jakub Kicinski
Quoted 3 lines · Stanislav Fomichev

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 0 0 0 0 \
value pinned /sys/fs/bpf/flow/IP/0

Where is that /0 coming from ? Is that in source code? I don't see libbpf adding it, maybe I'm missing something.

libbpf adds that, that's a program instance:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n1744

Ugh, I was looking at bpf_object__pin() which uses names :(

Quoted 7 lines
Quoted 3 lines

We never use this multi-instance thing, and I don't think bpftool ever will, so IMHO it'd be good if we just re-did the pinning loop in bpftool.

I wonder whether I should just add special case to bpf_program__pin: don't create a subdir when instances.nr == 1 (and just create a file pin for single instance)? In that case I can continue to use libbpf and don't reinvent the wheel. Any objections?

Mm.. I'm afraid libbpf needs to keep backward compatibility. We'd have to add some way for the user (bpftool code) to request the instance ID does not appear, but (potential) existing users should keep seeing them. Perhaps others disagree.

#14 lore raw
Quoted 24 lines · Jakub Kicinski
Quoted 15 lines · Stanislav Fomichev
Quoted 14 lines · Jakub Kicinski
Quoted 10 lines · Stanislav Fomichev
Quoted 7 lines · Jakub Kicinski
Quoted 3 lines · Stanislav Fomichev

bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
key 0 0 0 0 \
value pinned /sys/fs/bpf/flow/IP/0

Where is that /0 coming from ? Is that in source code? I don't see libbpf adding it, maybe I'm missing something.

libbpf adds that, that's a program instance:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n1744

Ugh, I was looking at bpf_object__pin() which uses names :(

Quoted 7 lines
Quoted 3 lines

We never use this multi-instance thing, and I don't think bpftool ever will, so IMHO it'd be good if we just re-did the pinning loop in bpftool.

I wonder whether I should just add special case to bpf_program__pin: don't create a subdir when instances.nr == 1 (and just create a file pin for single instance)? In that case I can continue to use libbpf and don't reinvent the wheel. Any objections?

Quoted 4 lines

Mm.. I'm afraid libbpf needs to keep backward compatibility. We'd have to add some way for the user (bpftool code) to request the instance ID does not appear, but (potential) existing users should keep seeing them. Perhaps others disagree.

AFAICT, nobody (seriously) uses bpf_object__pin in the kernel tree and I have a feeling that the situation is the same outside of the kernel tree. We can revert/work around if we break somebody, I just don't want to reimplement the same code in bpftool while there is a possibility that nobody is using that.

I'll post my proposal as v3, let's see whether other people have the same objections.

Btw, did we officially commit to the libbpf api/abi somewhere? It always felt to me like an internal and work-in-progress library.