[PATCH bpf-next v2 0/6] libbpf: Fix pinning and error message bugs and add new getters

[PATCH bpf-next v2 0/6] libbpf: Fix pinning and error message bugs and add new getters

[Toke Høiland-Jørgensen]

This series fixes a few bugs in libbpf that I discovered while playing around
with the new auto-pinning code, and writing the first utility in xdp-tools[0]:

- If object loading fails, libbpf does not clean up the pinnings created by the
  auto-pinning mechanism.
- EPERM is not propagated to the caller on program load
- Netlink functions write error messages directly to stderr

In addition, libbpf currently only has a somewhat limited getter function for
XDP link info, which makes it impossible to discover whether an attached program
is in SKB mode or not. So the last patch in the series adds a new getter for XDP
link info which returns all the information returned via netlink (and which can
be extended later).

Finally, add a getter for BPF program size, which can be used by the caller to
estimate the amount of locked memory needed to load a program.

A selftest is added for the pinning change, while the other features were tested
in the xdp-filter tool from the xdp-tools repo. The 'new-libbpf-features' branch
contains the commits that make use of the new XDP getter and the corrected EPERM
error code.

[0] https://github.com/xdp-project/xdp-tools

Changelog:

v2:
  - Keep function names in libbpf.map sorted properly

---

Toke Høiland-Jørgensen (6):
      libbpf: Unpin auto-pinned maps if loading fails
      selftests/bpf: Add tests for automatic map unpinning on load failure
      libbpf: Propagate EPERM to caller on program load
      libbpf: Use pr_warn() when printing netlink errors
      libbpf: Add bpf_get_link_xdp_info() function to get more XDP information
      libbpf: Add getter for program size


 tools/lib/bpf/libbpf.c                           |   25 +++++--
 tools/lib/bpf/libbpf.h                           |   11 +++
 tools/lib/bpf/libbpf.map                         |    2 +
 tools/lib/bpf/netlink.c                          |   81 ++++++++++++++--------
 tools/lib/bpf/nlattr.c                           |   10 +--
 tools/testing/selftests/bpf/prog_tests/pinning.c |   20 +++++
 tools/testing/selftests/bpf/progs/test_pinning.c |    2 -
 7 files changed, 109 insertions(+), 42 deletions(-)

[PATCH bpf-next v2 1/6] libbpf: Unpin auto-pinned maps if loading fails

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

Since the automatic map-pinning happens during load, it will leave pinned
maps around if the load fails at a later stage. Fix this by unpinning any
pinned maps on cleanup. To avoid unpinning pinned maps that were reused
rather than newly pinned, add a new boolean property on struct bpf_map to
keep track of whether that map was reused or not; and only unpin those maps
that were not reused.

Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/libbpf.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index be4af95d5a2c..cea61b2ec9d3 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -229,6 +229,7 @@ struct bpf_map {
 	enum libbpf_map_type libbpf_type;
 	char *pin_path;
 	bool pinned;
+	bool was_reused;
 };
 
 struct bpf_secdata {
@@ -1995,6 +1996,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd)
 	map->def.map_flags = info.map_flags;
 	map->btf_key_type_id = info.btf_key_type_id;
 	map->btf_value_type_id = info.btf_value_type_id;
+	map->was_reused = true;
 
 	return 0;
 
@@ -4007,15 +4009,18 @@ bpf_object__open_buffer(const void *obj_buf, size_t obj_buf_sz,
 	return bpf_object__open_mem(obj_buf, obj_buf_sz, &opts);
 }
 
-int bpf_object__unload(struct bpf_object *obj)
+static int __bpf_object__unload(struct bpf_object *obj, bool unpin)
 {
 	size_t i;
 
 	if (!obj)
 		return -EINVAL;
 
-	for (i = 0; i < obj->nr_maps; i++)
+	for (i = 0; i < obj->nr_maps; i++) {
 		zclose(obj->maps[i].fd);
+		if (unpin && obj->maps[i].pinned && !obj->maps[i].was_reused)
+			bpf_map__unpin(&obj->maps[i], NULL);
+	}
 
 	for (i = 0; i < obj->nr_programs; i++)
 		bpf_program__unload(&obj->programs[i]);
@@ -4023,6 +4028,11 @@ int bpf_object__unload(struct bpf_object *obj)
 	return 0;
 }
 
+int bpf_object__unload(struct bpf_object *obj)
+{
+	return __bpf_object__unload(obj, false);
+}
+
 int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 {
 	struct bpf_object *obj;
@@ -4047,7 +4057,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 
 	return 0;
 out:
-	bpf_object__unload(obj);
+	__bpf_object__unload(obj, true);
 	pr_warn("failed to load object '%s'\n", obj->path);
 	return err;
 }

[PATCH bpf-next v2 2/6] selftests/bpf: Add tests for automatic map unpinning on load failure

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

This add tests for the different variations of automatic map unpinning on
load failure.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/testing/selftests/bpf/prog_tests/pinning.c |   20 +++++++++++++++++---
 tools/testing/selftests/bpf/progs/test_pinning.c |    2 +-
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/pinning.c b/tools/testing/selftests/bpf/prog_tests/pinning.c
index 525388971e08..041952524c55 100644
--- a/tools/testing/selftests/bpf/prog_tests/pinning.c
+++ b/tools/testing/selftests/bpf/prog_tests/pinning.c
@@ -163,12 +163,15 @@ void test_pinning(void)
 		goto out;
 	}
 
-	/* swap pin paths of the two maps */
+	/* set pin paths so that nopinmap2 will attempt to reuse the map at
+	 * pinpath (which will fail), but not before pinmap has already been
+	 * reused
+	 */
 	bpf_object__for_each_map(map, obj) {
 		if (!strcmp(bpf_map__name(map), "nopinmap"))
+			err = bpf_map__set_pin_path(map, nopinpath2);
+		else if (!strcmp(bpf_map__name(map), "nopinmap2"))
 			err = bpf_map__set_pin_path(map, pinpath);
-		else if (!strcmp(bpf_map__name(map), "pinmap"))
-			err = bpf_map__set_pin_path(map, NULL);
 		else
 			continue;
 
@@ -181,6 +184,17 @@ void test_pinning(void)
 	if (CHECK(err != -EINVAL, "param mismatch load", "err %d errno %d\n", err, errno))
 		goto out;
 
+	/* nopinmap2 should have been pinned and cleaned up again */
+	err = stat(nopinpath2, &statbuf);
+	if (CHECK(!err || errno != ENOENT, "stat nopinpath2",
+		  "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* pinmap should still be there */
+	err = stat(pinpath, &statbuf);
+	if (CHECK(err, "stat pinpath", "err %d errno %d\n", err, errno))
+		goto out;
+
 	bpf_object__close(obj);
 
 	/* test auto-pinning at custom path with open opt */
diff --git a/tools/testing/selftests/bpf/progs/test_pinning.c b/tools/testing/selftests/bpf/progs/test_pinning.c
index f69a4a50d056..f20e7e00373f 100644
--- a/tools/testing/selftests/bpf/progs/test_pinning.c
+++ b/tools/testing/selftests/bpf/progs/test_pinning.c
@@ -21,7 +21,7 @@ struct {
 } nopinmap SEC(".maps");
 
 struct {
-	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(type, BPF_MAP_TYPE_HASH);
 	__uint(max_entries, 1);
 	__type(key, __u32);
 	__type(value, __u64);

[PATCH bpf-next v2 3/6] libbpf: Propagate EPERM to caller on program load

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

When loading an eBPF program, libbpf overrides the return code for EPERM
errors instead of returning it to the caller. This makes it hard to figure
out what went wrong on load.

In particular, EPERM is returned when the system rlimit is too low to lock
the memory required for the BPF program. Previously, this was somewhat
obscured because the rlimit error would be hit on map creation (which does
return it correctly). However, since maps can now be reused, object load
can proceed all the way to loading programs without hitting the error;
propagating it even in this case makes it possible for the caller to react
appropriately (and, e.g., attempt to raise the rlimit before retrying).

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/libbpf.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index cea61b2ec9d3..582c0fd16697 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -3721,7 +3721,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 		free(log_buf);
 		goto retry_load;
 	}
-	ret = -LIBBPF_ERRNO__LOAD;
+	ret = (errno == EPERM) ? -errno : -LIBBPF_ERRNO__LOAD;
 	cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
 	pr_warn("load bpf program failed: %s\n", cp);
 
@@ -3749,7 +3749,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 			}
 		}
 
-		if (log_buf)
+		if (log_buf && ret != -EPERM)
 			ret = -LIBBPF_ERRNO__KVER;
 	}
 

[PATCH bpf-next v2 4/6] libbpf: Use pr_warn() when printing netlink errors

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

The netlink functions were using fprintf(stderr, ) directly to print out
error messages, instead of going through the usual logging macros. This
makes it impossible for the calling application to silence or redirect
those error messages. Fix this by switching to pr_warn() in nlattr.c and
netlink.c.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/netlink.c |    3 ++-
 tools/lib/bpf/nlattr.c  |   10 +++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
index ce3ec81b71c0..a261df9cb488 100644
--- a/tools/lib/bpf/netlink.c
+++ b/tools/lib/bpf/netlink.c
@@ -12,6 +12,7 @@
 
 #include "bpf.h"
 #include "libbpf.h"
+#include "libbpf_internal.h"
 #include "nlattr.h"
 
 #ifndef SOL_NETLINK
@@ -43,7 +44,7 @@ int libbpf_netlink_open(__u32 *nl_pid)
 
 	if (setsockopt(sock, SOL_NETLINK, NETLINK_EXT_ACK,
 		       &one, sizeof(one)) < 0) {
-		fprintf(stderr, "Netlink error reporting not supported\n");
+		pr_warn("Netlink error reporting not supported\n");
 	}
 
 	if (bind(sock, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
diff --git a/tools/lib/bpf/nlattr.c b/tools/lib/bpf/nlattr.c
index 1e69c0c8d413..8db44bbfc66d 100644
--- a/tools/lib/bpf/nlattr.c
+++ b/tools/lib/bpf/nlattr.c
@@ -8,6 +8,7 @@
 
 #include <errno.h>
 #include "nlattr.h"
+#include "libbpf_internal.h"
 #include <linux/rtnetlink.h>
 #include <string.h>
 #include <stdio.h>
@@ -121,8 +122,8 @@ int libbpf_nla_parse(struct nlattr *tb[], int maxtype, struct nlattr *head,
 		}
 
 		if (tb[type])
-			fprintf(stderr, "Attribute of type %#x found multiple times in message, "
-				  "previous attribute is being ignored.\n", type);
+			pr_warn("Attribute of type %#x found multiple times in message, "
+				"previous attribute is being ignored.\n", type);
 
 		tb[type] = nla;
 	}
@@ -181,15 +182,14 @@ int libbpf_nla_dump_errormsg(struct nlmsghdr *nlh)
 
 	if (libbpf_nla_parse(tb, NLMSGERR_ATTR_MAX, attr, alen,
 			     extack_policy) != 0) {
-		fprintf(stderr,
-			"Failed to parse extended error attributes\n");
+		pr_warn("Failed to parse extended error attributes\n");
 		return 0;
 	}
 
 	if (tb[NLMSGERR_ATTR_MSG])
 		errmsg = (char *) libbpf_nla_data(tb[NLMSGERR_ATTR_MSG]);
 
-	fprintf(stderr, "Kernel error message: %s\n", errmsg);
+	pr_warn("Kernel error message: %s\n", errmsg);
 
 	return 0;
 }

[PATCH bpf-next v2 5/6] libbpf: Add bpf_get_link_xdp_info() function to get more XDP information

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

Currently, libbpf only provides a function to get a single ID for the XDP
program attached to the interface. However, it can be useful to get the
full set of program IDs attached, along with the attachment mode, in one
go. Add a new getter function to support this, using an extendible
structure to carry the information. Express the old bpf_get_link_id()
function in terms of the new function.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/libbpf.h   |   10 ++++++
 tools/lib/bpf/libbpf.map |    1 +
 tools/lib/bpf/netlink.c  |   78 ++++++++++++++++++++++++++++++----------------
 3 files changed, 62 insertions(+), 27 deletions(-)

diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 6ddc0419337b..f0947cc949d2 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -427,8 +427,18 @@ LIBBPF_API int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
 LIBBPF_API int bpf_prog_load(const char *file, enum bpf_prog_type type,
 			     struct bpf_object **pobj, int *prog_fd);
 
+struct xdp_link_info {
+	__u32 prog_id;
+	__u32 drv_prog_id;
+	__u32 hw_prog_id;
+	__u32 skb_prog_id;
+	__u8 attach_mode;
+};
+
 LIBBPF_API int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags);
 LIBBPF_API int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags);
+LIBBPF_API int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
+				     size_t info_size, __u32 flags);
 
 struct perf_buffer;
 
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 86173cbb159d..d1a782a3a58d 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -193,6 +193,7 @@ LIBBPF_0.0.5 {
 
 LIBBPF_0.0.6 {
 	global:
+		bpf_get_link_xdp_info;
 		bpf_map__get_pin_path;
 		bpf_map__is_pinned;
 		bpf_map__set_pin_path;
diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
index a261df9cb488..85019da01d3b 100644
--- a/tools/lib/bpf/netlink.c
+++ b/tools/lib/bpf/netlink.c
@@ -25,7 +25,7 @@ typedef int (*__dump_nlmsg_t)(struct nlmsghdr *nlmsg, libbpf_dump_nlmsg_t,
 struct xdp_id_md {
 	int ifindex;
 	__u32 flags;
-	__u32 id;
+	struct xdp_link_info info;
 };
 
 int libbpf_netlink_open(__u32 *nl_pid)
@@ -203,26 +203,11 @@ static int __dump_link_nlmsg(struct nlmsghdr *nlh,
 	return dump_link_nlmsg(cookie, ifi, tb);
 }
 
-static unsigned char get_xdp_id_attr(unsigned char mode, __u32 flags)
-{
-	if (mode != XDP_ATTACHED_MULTI)
-		return IFLA_XDP_PROG_ID;
-	if (flags & XDP_FLAGS_DRV_MODE)
-		return IFLA_XDP_DRV_PROG_ID;
-	if (flags & XDP_FLAGS_HW_MODE)
-		return IFLA_XDP_HW_PROG_ID;
-	if (flags & XDP_FLAGS_SKB_MODE)
-		return IFLA_XDP_SKB_PROG_ID;
-
-	return IFLA_XDP_UNSPEC;
-}
-
-static int get_xdp_id(void *cookie, void *msg, struct nlattr **tb)
+static int get_xdp_info(void *cookie, void *msg, struct nlattr **tb)
 {
 	struct nlattr *xdp_tb[IFLA_XDP_MAX + 1];
 	struct xdp_id_md *xdp_id = cookie;
 	struct ifinfomsg *ifinfo = msg;
-	unsigned char mode, xdp_attr;
 	int ret;
 
 	if (xdp_id->ifindex && xdp_id->ifindex != ifinfo->ifi_index)
@@ -238,27 +223,40 @@ static int get_xdp_id(void *cookie, void *msg, struct nlattr **tb)
 	if (!xdp_tb[IFLA_XDP_ATTACHED])
 		return 0;
 
-	mode = libbpf_nla_getattr_u8(xdp_tb[IFLA_XDP_ATTACHED]);
-	if (mode == XDP_ATTACHED_NONE)
-		return 0;
+	xdp_id->info.attach_mode = libbpf_nla_getattr_u8(
+		xdp_tb[IFLA_XDP_ATTACHED]);
 
-	xdp_attr = get_xdp_id_attr(mode, xdp_id->flags);
-	if (!xdp_attr || !xdp_tb[xdp_attr])
+	if (xdp_id->info.attach_mode == XDP_ATTACHED_NONE)
 		return 0;
 
-	xdp_id->id = libbpf_nla_getattr_u32(xdp_tb[xdp_attr]);
+	if (xdp_tb[IFLA_XDP_PROG_ID])
+		xdp_id->info.prog_id = libbpf_nla_getattr_u32(
+			xdp_tb[IFLA_XDP_PROG_ID]);
+
+	if (xdp_tb[IFLA_XDP_SKB_PROG_ID])
+		xdp_id->info.skb_prog_id = libbpf_nla_getattr_u32(
+			xdp_tb[IFLA_XDP_SKB_PROG_ID]);
+
+	if (xdp_tb[IFLA_XDP_DRV_PROG_ID])
+		xdp_id->info.drv_prog_id = libbpf_nla_getattr_u32(
+			xdp_tb[IFLA_XDP_DRV_PROG_ID]);
+
+	if (xdp_tb[IFLA_XDP_HW_PROG_ID])
+		xdp_id->info.hw_prog_id = libbpf_nla_getattr_u32(
+			xdp_tb[IFLA_XDP_HW_PROG_ID]);
 
 	return 0;
 }
 
-int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
+int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
+			  size_t info_size, __u32 flags)
 {
 	struct xdp_id_md xdp_id = {};
 	int sock, ret;
 	__u32 nl_pid;
 	__u32 mask;
 
-	if (flags & ~XDP_FLAGS_MASK)
+	if (flags & ~XDP_FLAGS_MASK || info_size != sizeof(*info))
 		return -EINVAL;
 
 	/* Check whether the single {HW,DRV,SKB} mode is set */
@@ -274,14 +272,40 @@ int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
 	xdp_id.ifindex = ifindex;
 	xdp_id.flags = flags;
 
-	ret = libbpf_nl_get_link(sock, nl_pid, get_xdp_id, &xdp_id);
+	ret = libbpf_nl_get_link(sock, nl_pid, get_xdp_info, &xdp_id);
 	if (!ret)
-		*prog_id = xdp_id.id;
+		memcpy(info, &xdp_id.info, sizeof(*info));
 
 	close(sock);
 	return ret;
 }
 
+static __u32 get_xdp_id(struct xdp_link_info *info, __u32 flags)
+{
+	if (info->attach_mode != XDP_ATTACHED_MULTI)
+		return info->prog_id;
+	if (flags & XDP_FLAGS_DRV_MODE)
+		return info->drv_prog_id;
+	if (flags & XDP_FLAGS_HW_MODE)
+		return info->hw_prog_id;
+	if (flags & XDP_FLAGS_SKB_MODE)
+		return info->skb_prog_id;
+
+	return 0;
+}
+
+int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
+{
+	struct xdp_link_info info = {};
+	int ret;
+
+	ret = bpf_get_link_xdp_info(ifindex, &info, sizeof(info), flags);
+	if (!ret)
+		*prog_id = get_xdp_id(&info, flags);
+
+	return ret;
+}
+
 int libbpf_nl_get_link(int sock, unsigned int nl_pid,
 		       libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie)
 {

[PATCH bpf-next v2 6/6] libbpf: Add getter for program size

[Toke Høiland-Jørgensen]

From: Toke Høiland-Jørgensen <toke@redhat.com>

This adds a new getter for the BPF program size (in bytes). This is useful
for a caller that is trying to predict how much memory will be locked by
loading a BPF object into the kernel.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/libbpf.c   |    5 +++++
 tools/lib/bpf/libbpf.h   |    1 +
 tools/lib/bpf/libbpf.map |    1 +
 3 files changed, 7 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 582c0fd16697..facd5e1a3a0b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4790,6 +4790,11 @@ int bpf_program__fd(const struct bpf_program *prog)
 	return bpf_program__nth_fd(prog, 0);
 }
 
+size_t bpf_program__size(const struct bpf_program *prog)
+{
+	return prog->insns_cnt * sizeof(struct bpf_insn);
+}
+
 int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
 			  bpf_program_prep_t prep)
 {
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index f0947cc949d2..10875dc68ec8 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -213,6 +213,7 @@ LIBBPF_API void bpf_program__set_ifindex(struct bpf_program *prog,
 
 LIBBPF_API const char *bpf_program__title(const struct bpf_program *prog,
 					  bool needs_copy);
+LIBBPF_API size_t bpf_program__size(const struct bpf_program *prog);
 
 LIBBPF_API int bpf_program__load(struct bpf_program *prog, char *license,
 				 __u32 kern_version);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index d1a782a3a58d..9f39ee06b2d4 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -203,4 +203,5 @@ LIBBPF_0.0.6 {
 		bpf_program__get_type;
 		bpf_program__is_tracing;
 		bpf_program__set_tracing;
+		bpf_program__size;
 } LIBBPF_0.0.5;

Re: [PATCH bpf-next v2 1/6] libbpf: Unpin auto-pinned maps if loading fails

[David Miller]

From: Toke Høiland-Jørgensen <toke@redhat.com>
Date: Fri, 08 Nov 2019 22:33:06 +0100

> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> Since the automatic map-pinning happens during load, it will leave pinned
> maps around if the load fails at a later stage. Fix this by unpinning any
> pinned maps on cleanup. To avoid unpinning pinned maps that were reused
> rather than newly pinned, add a new boolean property on struct bpf_map to
> keep track of whether that map was reused or not; and only unpin those maps
> that were not reused.
> 
> Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH bpf-next v2 2/6] selftests/bpf: Add tests for automatic map unpinning on load failure

[David Miller]

From: Toke Høiland-Jørgensen <toke@redhat.com>
Date: Fri, 08 Nov 2019 22:33:07 +0100

> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> This add tests for the different variations of automatic map unpinning on
> load failure.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH bpf-next v2 3/6] libbpf: Propagate EPERM to caller on program load

[David Miller]

From: Toke Høiland-Jørgensen <toke@redhat.com>
Date: Fri, 08 Nov 2019 22:33:08 +0100

> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> When loading an eBPF program, libbpf overrides the return code for EPERM
> errors instead of returning it to the caller. This makes it hard to figure
> out what went wrong on load.
> 
> In particular, EPERM is returned when the system rlimit is too low to lock
> the memory required for the BPF program. Previously, this was somewhat
> obscured because the rlimit error would be hit on map creation (which does
> return it correctly). However, since maps can now be reused, object load
> can proceed all the way to loading programs without hitting the error;
> propagating it even in this case makes it possible for the caller to react
> appropriately (and, e.g., attempt to raise the rlimit before retrying).
> 
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH bpf-next v2 4/6] libbpf: Use pr_warn() when printing netlink errors

[David Miller]

From: Toke Høiland-Jørgensen <toke@redhat.com>
Date: Fri, 08 Nov 2019 22:33:09 +0100

> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> The netlink functions were using fprintf(stderr, ) directly to print out
> error messages, instead of going through the usual logging macros. This
> makes it impossible for the calling application to silence or redirect
> those error messages. Fix this by switching to pr_warn() in nlattr.c and
> netlink.c.
> 
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH bpf-next v2 5/6] libbpf: Add bpf_get_link_xdp_info() function to get more XDP information

[David Miller]

From: Toke Høiland-Jørgensen <toke@redhat.com>
Date: Fri, 08 Nov 2019 22:33:10 +0100

> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> Currently, libbpf only provides a function to get a single ID for the XDP
> program attached to the interface. However, it can be useful to get the
> full set of program IDs attached, along with the attachment mode, in one
> go. Add a new getter function to support this, using an extendible
> structure to carry the information. Express the old bpf_get_link_id()
> function in terms of the new function.
> 
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH bpf-next v2 6/6] libbpf: Add getter for program size

[David Miller]

From: Toke Høiland-Jørgensen <toke@redhat.com>
Date: Fri, 08 Nov 2019 22:33:11 +0100

> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> This adds a new getter for the BPF program size (in bytes). This is useful
> for a caller that is trying to predict how much memory will be locked by
> loading a BPF object into the kernel.
> 
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH bpf-next v2 2/6] selftests/bpf: Add tests for automatic map unpinning on load failure

[Song Liu]

> On Nov 8, 2019, at 1:41 PM, David Miller <davem@davemloft.net> wrote:
> 
> From: Toke Høiland-Jørgensen <toke@redhat.com>
> Date: Fri, 08 Nov 2019 22:33:07 +0100
> 
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> This add tests for the different variations of automatic map unpinning on
>> load failure.
>> 
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> Acked-by: David S. Miller <davem@davemloft.net>

Acked-by: Song Liu <songliubraving@fb.com>

Re: [PATCH bpf-next v2 1/6] libbpf: Unpin auto-pinned maps if loading fails

[Andrii Nakryiko]

On Fri, Nov 8, 2019 at 1:33 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Since the automatic map-pinning happens during load, it will leave pinned
> maps around if the load fails at a later stage. Fix this by unpinning any
> pinned maps on cleanup. To avoid unpinning pinned maps that were reused
> rather than newly pinned, add a new boolean property on struct bpf_map to
> keep track of whether that map was reused or not; and only unpin those maps
> that were not reused.
>
> Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  tools/lib/bpf/libbpf.c |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index be4af95d5a2c..cea61b2ec9d3 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -229,6 +229,7 @@ struct bpf_map {
>         enum libbpf_map_type libbpf_type;
>         char *pin_path;
>         bool pinned;
> +       bool was_reused;

nit: just reused, similar to pinned?

>  };
>
>  struct bpf_secdata {
> @@ -1995,6 +1996,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd)
>         map->def.map_flags = info.map_flags;
>         map->btf_key_type_id = info.btf_key_type_id;
>         map->btf_value_type_id = info.btf_value_type_id;
> +       map->was_reused = true;
>
>         return 0;
>
> @@ -4007,15 +4009,18 @@ bpf_object__open_buffer(const void *obj_buf, size_t obj_buf_sz,
>         return bpf_object__open_mem(obj_buf, obj_buf_sz, &opts);
>  }
>
> -int bpf_object__unload(struct bpf_object *obj)
> +static int __bpf_object__unload(struct bpf_object *obj, bool unpin)
>  {
>         size_t i;
>
>         if (!obj)
>                 return -EINVAL;
>
> -       for (i = 0; i < obj->nr_maps; i++)
> +       for (i = 0; i < obj->nr_maps; i++) {
>                 zclose(obj->maps[i].fd);
> +               if (unpin && obj->maps[i].pinned && !obj->maps[i].was_reused)
> +                       bpf_map__unpin(&obj->maps[i], NULL);
> +       }
>
>         for (i = 0; i < obj->nr_programs; i++)
>                 bpf_program__unload(&obj->programs[i]);
> @@ -4023,6 +4028,11 @@ int bpf_object__unload(struct bpf_object *obj)
>         return 0;
>  }
>
> +int bpf_object__unload(struct bpf_object *obj)
> +{
> +       return __bpf_object__unload(obj, false);
> +}
> +
>  int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>  {
>         struct bpf_object *obj;
> @@ -4047,7 +4057,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>
>         return 0;
>  out:
> -       bpf_object__unload(obj);
> +       __bpf_object__unload(obj, true);

giving this is the only (special) case of auto-unpinning auto-pinned
maps, why not do a trivial loop here, instead of having this extra
unpin flag and extra __bpf_object__unload function?

>         pr_warn("failed to load object '%s'\n", obj->path);
>         return err;
>  }
>

Re: [PATCH bpf-next v2 2/6] selftests/bpf: Add tests for automatic map unpinning on load failure

[Andrii Nakryiko]

On Fri, Nov 8, 2019 at 1:33 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> This add tests for the different variations of automatic map unpinning on
> load failure.
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/testing/selftests/bpf/prog_tests/pinning.c |   20 +++++++++++++++++---
>  tools/testing/selftests/bpf/progs/test_pinning.c |    2 +-
>  2 files changed, 18 insertions(+), 4 deletions(-)
>

[...]

Re: [PATCH bpf-next v2 3/6] libbpf: Propagate EPERM to caller on program load

[Andrii Nakryiko]

On Fri, Nov 8, 2019 at 1:33 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> When loading an eBPF program, libbpf overrides the return code for EPERM
> errors instead of returning it to the caller. This makes it hard to figure
> out what went wrong on load.
>
> In particular, EPERM is returned when the system rlimit is too low to lock
> the memory required for the BPF program. Previously, this was somewhat
> obscured because the rlimit error would be hit on map creation (which does
> return it correctly). However, since maps can now be reused, object load
> can proceed all the way to loading programs without hitting the error;
> propagating it even in this case makes it possible for the caller to react
> appropriately (and, e.g., attempt to raise the rlimit before retrying).
>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  tools/lib/bpf/libbpf.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index cea61b2ec9d3..582c0fd16697 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -3721,7 +3721,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
>                 free(log_buf);
>                 goto retry_load;
>         }
> -       ret = -LIBBPF_ERRNO__LOAD;
> +       ret = (errno == EPERM) ? -errno : -LIBBPF_ERRNO__LOAD;
>         cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
>         pr_warn("load bpf program failed: %s\n", cp);
>
> @@ -3749,7 +3749,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
>                         }
>                 }
>
> -               if (log_buf)
> +               if (log_buf && ret != -EPERM)
>                         ret = -LIBBPF_ERRNO__KVER;

This whole special casing of EPERM looks weird. Should we just pass
through all the errors instead?

But also, I don't think you can assume that if you get EPERM, then it
must be setrlimit problem...


>         }
>
>

Re: [PATCH bpf-next v2 4/6] libbpf: Use pr_warn() when printing netlink errors

[Andrii Nakryiko]

On Fri, Nov 8, 2019 at 1:33 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> The netlink functions were using fprintf(stderr, ) directly to print out
> error messages, instead of going through the usual logging macros. This
> makes it impossible for the calling application to silence or redirect
> those error messages. Fix this by switching to pr_warn() in nlattr.c and
> netlink.c.
>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/lib/bpf/netlink.c |    3 ++-
>  tools/lib/bpf/nlattr.c  |   10 +++++-----
>  2 files changed, 7 insertions(+), 6 deletions(-)
>

[...]

Re: [PATCH bpf-next v2 5/6] libbpf: Add bpf_get_link_xdp_info() function to get more XDP information

[Andrii Nakryiko]

On Fri, Nov 8, 2019 at 1:33 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Currently, libbpf only provides a function to get a single ID for the XDP
> program attached to the interface. However, it can be useful to get the
> full set of program IDs attached, along with the attachment mode, in one
> go. Add a new getter function to support this, using an extendible
> structure to carry the information. Express the old bpf_get_link_id()
> function in terms of the new function.
>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  tools/lib/bpf/libbpf.h   |   10 ++++++
>  tools/lib/bpf/libbpf.map |    1 +
>  tools/lib/bpf/netlink.c  |   78 ++++++++++++++++++++++++++++++----------------
>  3 files changed, 62 insertions(+), 27 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 6ddc0419337b..f0947cc949d2 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -427,8 +427,18 @@ LIBBPF_API int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
>  LIBBPF_API int bpf_prog_load(const char *file, enum bpf_prog_type type,
>                              struct bpf_object **pobj, int *prog_fd);
>
> +struct xdp_link_info {
> +       __u32 prog_id;
> +       __u32 drv_prog_id;
> +       __u32 hw_prog_id;
> +       __u32 skb_prog_id;
> +       __u8 attach_mode;
> +};
> +
>  LIBBPF_API int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags);
>  LIBBPF_API int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags);
> +LIBBPF_API int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
> +                                    size_t info_size, __u32 flags);
>
>  struct perf_buffer;
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 86173cbb159d..d1a782a3a58d 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -193,6 +193,7 @@ LIBBPF_0.0.5 {
>
>  LIBBPF_0.0.6 {
>         global:
> +               bpf_get_link_xdp_info;
>                 bpf_map__get_pin_path;
>                 bpf_map__is_pinned;
>                 bpf_map__set_pin_path;
> diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
> index a261df9cb488..85019da01d3b 100644
> --- a/tools/lib/bpf/netlink.c
> +++ b/tools/lib/bpf/netlink.c
> @@ -25,7 +25,7 @@ typedef int (*__dump_nlmsg_t)(struct nlmsghdr *nlmsg, libbpf_dump_nlmsg_t,
>  struct xdp_id_md {
>         int ifindex;
>         __u32 flags;
> -       __u32 id;
> +       struct xdp_link_info info;
>  };
>
>  int libbpf_netlink_open(__u32 *nl_pid)
> @@ -203,26 +203,11 @@ static int __dump_link_nlmsg(struct nlmsghdr *nlh,
>         return dump_link_nlmsg(cookie, ifi, tb);
>  }
>
> -static unsigned char get_xdp_id_attr(unsigned char mode, __u32 flags)
> -{
> -       if (mode != XDP_ATTACHED_MULTI)
> -               return IFLA_XDP_PROG_ID;
> -       if (flags & XDP_FLAGS_DRV_MODE)
> -               return IFLA_XDP_DRV_PROG_ID;
> -       if (flags & XDP_FLAGS_HW_MODE)
> -               return IFLA_XDP_HW_PROG_ID;
> -       if (flags & XDP_FLAGS_SKB_MODE)
> -               return IFLA_XDP_SKB_PROG_ID;
> -
> -       return IFLA_XDP_UNSPEC;
> -}
> -
> -static int get_xdp_id(void *cookie, void *msg, struct nlattr **tb)
> +static int get_xdp_info(void *cookie, void *msg, struct nlattr **tb)
>  {
>         struct nlattr *xdp_tb[IFLA_XDP_MAX + 1];
>         struct xdp_id_md *xdp_id = cookie;
>         struct ifinfomsg *ifinfo = msg;
> -       unsigned char mode, xdp_attr;
>         int ret;
>
>         if (xdp_id->ifindex && xdp_id->ifindex != ifinfo->ifi_index)
> @@ -238,27 +223,40 @@ static int get_xdp_id(void *cookie, void *msg, struct nlattr **tb)
>         if (!xdp_tb[IFLA_XDP_ATTACHED])
>                 return 0;
>
> -       mode = libbpf_nla_getattr_u8(xdp_tb[IFLA_XDP_ATTACHED]);
> -       if (mode == XDP_ATTACHED_NONE)
> -               return 0;
> +       xdp_id->info.attach_mode = libbpf_nla_getattr_u8(
> +               xdp_tb[IFLA_XDP_ATTACHED]);
>
> -       xdp_attr = get_xdp_id_attr(mode, xdp_id->flags);
> -       if (!xdp_attr || !xdp_tb[xdp_attr])
> +       if (xdp_id->info.attach_mode == XDP_ATTACHED_NONE)
>                 return 0;
>
> -       xdp_id->id = libbpf_nla_getattr_u32(xdp_tb[xdp_attr]);
> +       if (xdp_tb[IFLA_XDP_PROG_ID])
> +               xdp_id->info.prog_id = libbpf_nla_getattr_u32(
> +                       xdp_tb[IFLA_XDP_PROG_ID]);
> +
> +       if (xdp_tb[IFLA_XDP_SKB_PROG_ID])
> +               xdp_id->info.skb_prog_id = libbpf_nla_getattr_u32(
> +                       xdp_tb[IFLA_XDP_SKB_PROG_ID]);
> +
> +       if (xdp_tb[IFLA_XDP_DRV_PROG_ID])
> +               xdp_id->info.drv_prog_id = libbpf_nla_getattr_u32(
> +                       xdp_tb[IFLA_XDP_DRV_PROG_ID]);
> +
> +       if (xdp_tb[IFLA_XDP_HW_PROG_ID])
> +               xdp_id->info.hw_prog_id = libbpf_nla_getattr_u32(
> +                       xdp_tb[IFLA_XDP_HW_PROG_ID]);
>
>         return 0;
>  }
>
> -int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
> +int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
> +                         size_t info_size, __u32 flags)
>  {
>         struct xdp_id_md xdp_id = {};
>         int sock, ret;
>         __u32 nl_pid;
>         __u32 mask;
>
> -       if (flags & ~XDP_FLAGS_MASK)
> +       if (flags & ~XDP_FLAGS_MASK || info_size != sizeof(*info))

This is not forward-compatible. Newer application can pass bigger
info_size, if xdp_link_info gets extended. We should probably just
zero-fill the part we don't understand.

>                 return -EINVAL;
>
>         /* Check whether the single {HW,DRV,SKB} mode is set */
> @@ -274,14 +272,40 @@ int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
>         xdp_id.ifindex = ifindex;
>         xdp_id.flags = flags;
>
> -       ret = libbpf_nl_get_link(sock, nl_pid, get_xdp_id, &xdp_id);
> +       ret = libbpf_nl_get_link(sock, nl_pid, get_xdp_info, &xdp_id);
>         if (!ret)
> -               *prog_id = xdp_id.id;
> +               memcpy(info, &xdp_id.info, sizeof(*info));
>
>         close(sock);
>         return ret;
>  }
>
> +static __u32 get_xdp_id(struct xdp_link_info *info, __u32 flags)
> +{
> +       if (info->attach_mode != XDP_ATTACHED_MULTI)
> +               return info->prog_id;
> +       if (flags & XDP_FLAGS_DRV_MODE)
> +               return info->drv_prog_id;
> +       if (flags & XDP_FLAGS_HW_MODE)
> +               return info->hw_prog_id;
> +       if (flags & XDP_FLAGS_SKB_MODE)
> +               return info->skb_prog_id;
> +
> +       return 0;
> +}
> +
> +int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
> +{
> +       struct xdp_link_info info = {};

seems like there is no need to pre-initialize info, it should be
initialized (on success) by bpf_get_link_xdp_info?

> +       int ret;
> +
> +       ret = bpf_get_link_xdp_info(ifindex, &info, sizeof(info), flags);
> +       if (!ret)
> +               *prog_id = get_xdp_id(&info, flags);
> +
> +       return ret;
> +}
> +
>  int libbpf_nl_get_link(int sock, unsigned int nl_pid,
>                        libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie)
>  {
>

Re: [PATCH bpf-next v2 6/6] libbpf: Add getter for program size

[Andrii Nakryiko]

On Fri, Nov 8, 2019 at 1:33 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> This adds a new getter for the BPF program size (in bytes). This is useful
> for a caller that is trying to predict how much memory will be locked by
> loading a BPF object into the kernel.
>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---

Can you add comment mentioning that this is size in bytes, not in
number of instructions? It's certainly will be a first question anyone
using this will ask.

I think it's good to have this, but I don't think you can really
predict how much memory will be used. I'd expect memory used by maps
(and not just based on element size and count, but some internal
bookkeeping stuff) would be much bigger factor and not easy to guess.
So beyond just stats dumping, I think this won't be that helpful.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  tools/lib/bpf/libbpf.c   |    5 +++++
>  tools/lib/bpf/libbpf.h   |    1 +
>  tools/lib/bpf/libbpf.map |    1 +
>  3 files changed, 7 insertions(+)
>

[...]

Re: [PATCH bpf-next v2 3/6] libbpf: Propagate EPERM to caller on program load

[Alexei Starovoitov]

On Fri, Nov 08, 2019 at 02:50:43PM -0800, Andrii Nakryiko wrote:
> On Fri, Nov 8, 2019 at 1:33 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >
> > From: Toke Høiland-Jørgensen <toke@redhat.com>
> >
> > When loading an eBPF program, libbpf overrides the return code for EPERM
> > errors instead of returning it to the caller. This makes it hard to figure
> > out what went wrong on load.
> >
> > In particular, EPERM is returned when the system rlimit is too low to lock
> > the memory required for the BPF program. Previously, this was somewhat
> > obscured because the rlimit error would be hit on map creation (which does
> > return it correctly). However, since maps can now be reused, object load
> > can proceed all the way to loading programs without hitting the error;
> > propagating it even in this case makes it possible for the caller to react
> > appropriately (and, e.g., attempt to raise the rlimit before retrying).
> >
> > Acked-by: Song Liu <songliubraving@fb.com>
> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > ---
> >  tools/lib/bpf/libbpf.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index cea61b2ec9d3..582c0fd16697 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -3721,7 +3721,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> >                 free(log_buf);
> >                 goto retry_load;
> >         }
> > -       ret = -LIBBPF_ERRNO__LOAD;
> > +       ret = (errno == EPERM) ? -errno : -LIBBPF_ERRNO__LOAD;

ouch. so libbpf was supressing all errnos for loading and that was a commit
from 2015. No wonder it's hard to debug. I grepped every where I could and it
doesn't look like anyone is using this code. There are other codes that can
come from sys_bpf(prog_load). Not sure why such decision was made back then. I
guess noone was really paying attention. I think we better propagate all codes.
I don't see why EPERM should be special.

Re: [PATCH bpf-next v2 3/6] libbpf: Propagate EPERM to caller on program load

[Toke Høiland-Jørgensen]

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Fri, Nov 08, 2019 at 02:50:43PM -0800, Andrii Nakryiko wrote:
>> On Fri, Nov 8, 2019 at 1:33 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >
>> > From: Toke Høiland-Jørgensen <toke@redhat.com>
>> >
>> > When loading an eBPF program, libbpf overrides the return code for EPERM
>> > errors instead of returning it to the caller. This makes it hard to figure
>> > out what went wrong on load.
>> >
>> > In particular, EPERM is returned when the system rlimit is too low to lock
>> > the memory required for the BPF program. Previously, this was somewhat
>> > obscured because the rlimit error would be hit on map creation (which does
>> > return it correctly). However, since maps can now be reused, object load
>> > can proceed all the way to loading programs without hitting the error;
>> > propagating it even in this case makes it possible for the caller to react
>> > appropriately (and, e.g., attempt to raise the rlimit before retrying).
>> >
>> > Acked-by: Song Liu <songliubraving@fb.com>
>> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> > ---
>> >  tools/lib/bpf/libbpf.c |    4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> > index cea61b2ec9d3..582c0fd16697 100644
>> > --- a/tools/lib/bpf/libbpf.c
>> > +++ b/tools/lib/bpf/libbpf.c
>> > @@ -3721,7 +3721,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
>> >                 free(log_buf);
>> >                 goto retry_load;
>> >         }
>> > -       ret = -LIBBPF_ERRNO__LOAD;
>> > +       ret = (errno == EPERM) ? -errno : -LIBBPF_ERRNO__LOAD;
>
> ouch. so libbpf was supressing all errnos for loading and that was a commit
> from 2015. No wonder it's hard to debug. I grepped every where I could and it
> doesn't look like anyone is using this code. There are other codes that can
> come from sys_bpf(prog_load). Not sure why such decision was made back then. I
> guess noone was really paying attention. I think we better propagate all codes.
> I don't see why EPERM should be special.

Fine with me; I just assumed there was a good reason for the current
behaviour :)

Will do a v3 that just passes through the errors from the kernel.

-Toke

Re: [PATCH bpf-next v2 1/6] libbpf: Unpin auto-pinned maps if loading fails

[Toke Høiland-Jørgensen]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Fri, Nov 8, 2019 at 1:33 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>>
>> Since the automatic map-pinning happens during load, it will leave pinned
>> maps around if the load fails at a later stage. Fix this by unpinning any
>> pinned maps on cleanup. To avoid unpinning pinned maps that were reused
>> rather than newly pinned, add a new boolean property on struct bpf_map to
>> keep track of whether that map was reused or not; and only unpin those maps
>> that were not reused.
>>
>> Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
>> Acked-by: Song Liu <songliubraving@fb.com>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>>  tools/lib/bpf/libbpf.c |   16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index be4af95d5a2c..cea61b2ec9d3 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -229,6 +229,7 @@ struct bpf_map {
>>         enum libbpf_map_type libbpf_type;
>>         char *pin_path;
>>         bool pinned;
>> +       bool was_reused;
>
> nit: just reused, similar to pinned?
>
>>  };
>>
>>  struct bpf_secdata {
>> @@ -1995,6 +1996,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd)
>>         map->def.map_flags = info.map_flags;
>>         map->btf_key_type_id = info.btf_key_type_id;
>>         map->btf_value_type_id = info.btf_value_type_id;
>> +       map->was_reused = true;
>>
>>         return 0;
>>
>> @@ -4007,15 +4009,18 @@ bpf_object__open_buffer(const void *obj_buf, size_t obj_buf_sz,
>>         return bpf_object__open_mem(obj_buf, obj_buf_sz, &opts);
>>  }
>>
>> -int bpf_object__unload(struct bpf_object *obj)
>> +static int __bpf_object__unload(struct bpf_object *obj, bool unpin)
>>  {
>>         size_t i;
>>
>>         if (!obj)
>>                 return -EINVAL;
>>
>> -       for (i = 0; i < obj->nr_maps; i++)
>> +       for (i = 0; i < obj->nr_maps; i++) {
>>                 zclose(obj->maps[i].fd);
>> +               if (unpin && obj->maps[i].pinned && !obj->maps[i].was_reused)
>> +                       bpf_map__unpin(&obj->maps[i], NULL);
>> +       }
>>
>>         for (i = 0; i < obj->nr_programs; i++)
>>                 bpf_program__unload(&obj->programs[i]);
>> @@ -4023,6 +4028,11 @@ int bpf_object__unload(struct bpf_object *obj)
>>         return 0;
>>  }
>>
>> +int bpf_object__unload(struct bpf_object *obj)
>> +{
>> +       return __bpf_object__unload(obj, false);
>> +}
>> +
>>  int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>>  {
>>         struct bpf_object *obj;
>> @@ -4047,7 +4057,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
>>
>>         return 0;
>>  out:
>> -       bpf_object__unload(obj);
>> +       __bpf_object__unload(obj, true);
>
> giving this is the only (special) case of auto-unpinning auto-pinned
> maps, why not do a trivial loop here, instead of having this extra
> unpin flag and extra __bpf_object__unload function?

Oh, you mean just do a loop in addition to the call to __unload? Sure, I
guess we can do that instead...

-Toke

Re: [PATCH bpf-next v2 1/6] libbpf: Unpin auto-pinned maps if loading fails

[Andrii Nakryiko]

On Fri, Nov 8, 2019 at 3:33 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Fri, Nov 8, 2019 at 1:33 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> From: Toke Høiland-Jørgensen <toke@redhat.com>
> >>
> >> Since the automatic map-pinning happens during load, it will leave pinned
> >> maps around if the load fails at a later stage. Fix this by unpinning any
> >> pinned maps on cleanup. To avoid unpinning pinned maps that were reused
> >> rather than newly pinned, add a new boolean property on struct bpf_map to
> >> keep track of whether that map was reused or not; and only unpin those maps
> >> that were not reused.
> >>
> >> Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
> >> Acked-by: Song Liu <songliubraving@fb.com>
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> ---
> >>  tools/lib/bpf/libbpf.c |   16 +++++++++++++---
> >>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index be4af95d5a2c..cea61b2ec9d3 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -229,6 +229,7 @@ struct bpf_map {
> >>         enum libbpf_map_type libbpf_type;
> >>         char *pin_path;
> >>         bool pinned;
> >> +       bool was_reused;
> >
> > nit: just reused, similar to pinned?
> >
> >>  };
> >>
> >>  struct bpf_secdata {
> >> @@ -1995,6 +1996,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd)
> >>         map->def.map_flags = info.map_flags;
> >>         map->btf_key_type_id = info.btf_key_type_id;
> >>         map->btf_value_type_id = info.btf_value_type_id;
> >> +       map->was_reused = true;
> >>
> >>         return 0;
> >>
> >> @@ -4007,15 +4009,18 @@ bpf_object__open_buffer(const void *obj_buf, size_t obj_buf_sz,
> >>         return bpf_object__open_mem(obj_buf, obj_buf_sz, &opts);
> >>  }
> >>
> >> -int bpf_object__unload(struct bpf_object *obj)
> >> +static int __bpf_object__unload(struct bpf_object *obj, bool unpin)
> >>  {
> >>         size_t i;
> >>
> >>         if (!obj)
> >>                 return -EINVAL;
> >>
> >> -       for (i = 0; i < obj->nr_maps; i++)
> >> +       for (i = 0; i < obj->nr_maps; i++) {
> >>                 zclose(obj->maps[i].fd);
> >> +               if (unpin && obj->maps[i].pinned && !obj->maps[i].was_reused)
> >> +                       bpf_map__unpin(&obj->maps[i], NULL);
> >> +       }
> >>
> >>         for (i = 0; i < obj->nr_programs; i++)
> >>                 bpf_program__unload(&obj->programs[i]);
> >> @@ -4023,6 +4028,11 @@ int bpf_object__unload(struct bpf_object *obj)
> >>         return 0;
> >>  }
> >>
> >> +int bpf_object__unload(struct bpf_object *obj)
> >> +{
> >> +       return __bpf_object__unload(obj, false);
> >> +}
> >> +
> >>  int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> >>  {
> >>         struct bpf_object *obj;
> >> @@ -4047,7 +4057,7 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> >>
> >>         return 0;
> >>  out:
> >> -       bpf_object__unload(obj);
> >> +       __bpf_object__unload(obj, true);
> >
> > giving this is the only (special) case of auto-unpinning auto-pinned
> > maps, why not do a trivial loop here, instead of having this extra
> > unpin flag and extra __bpf_object__unload function?
>
> Oh, you mean just do a loop in addition to the call to __unload? Sure, I
> guess we can do that instead...

I think that's cleaner, because it's custom clean up logic in one
place, rather than supported feature of unload.

>
> -Toke