netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 bpf-next 0/4] Add new-style bpf_object__open APIs
@ 2019-10-04 22:40 Andrii Nakryiko
  2019-10-04 22:40 ` [PATCH v3 bpf-next 1/4] libbpf: stop enforcing kern_version, populate it for users Andrii Nakryiko
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2019-10-04 22:40 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Add bpf_object__open_file() and bpf_object__open_mem() APIs that use a new
approach to providing future-proof non-ABI-breaking API changes. It relies on
APIs accepting optional self-describing "opts" struct, containing its own
size, filled out and provided by potentially outdated (as well as
newer-than-libbpf) user application. A set of internal helper macros
(OPTS_VALID, OPTS_HAS, and OPTS_GET) streamline and simplify a graceful
handling forward and backward compatibility for user applications dynamically
linked against different versions of libbpf shared library.

Users of libbpf are provided with convenience macro LIBBPF_OPTS that takes
care of populating correct structure size and zero-initializes options struct,
which helps avoid obscure issues of unitialized padding. Uninitialized padding
in a struct might turn into garbage-populated new fields understood by future
versions of libbpf.

Patch #1 removes enforcement of kern_version in libbpf and always populates
correct one on behalf of users.
Patch #2 defines necessary infrastructure for options and two new open APIs
relying on it.
Patch #3 fixes bug in bpf_object__name().
Patch #4 switches two of test_progs' tests to use new APIs as a validation
that they work as expected.

v2->v3:
- fix LIBBPF_OPTS() to ensure zero-initialization of padded bytes;
- pass through name override and relaxed maps flag for open_file() (Toke);
- fix bpf_object__name() to actually return object name;
- don't bother parsing and verifying version section (John);

v1->v2:
- use better approach for tracking last field in opts struct;
- convert few tests to new APIs for validation;
- fix bug with using offsetof(last_field) instead of offsetofend(last_field).

Andrii Nakryiko (4):
  libbpf: stop enforcing kern_version, populate it for users
  libbpf: add bpf_object__open_{file,mem} w/ extensible opts
  libbpf: fix bpf_object__name() to actually return object name
  selftests/bpf: switch tests to new bpf_object__open_{file,mem}() APIs

 tools/lib/bpf/libbpf.c                        | 183 +++++++++---------
 tools/lib/bpf/libbpf.h                        |  48 ++++-
 tools/lib/bpf/libbpf.map                      |   3 +
 tools/lib/bpf/libbpf_internal.h               |  32 +++
 tools/testing/selftests/bpf/Makefile          |   2 +-
 .../selftests/bpf/prog_tests/attach_probe.c   |  49 ++++-
 .../bpf/prog_tests/reference_tracking.c       |  16 +-
 .../selftests/bpf/progs/test_attach_probe.c   |   1 -
 .../bpf/progs/test_get_stack_rawtp.c          |   1 -
 .../selftests/bpf/progs/test_perf_buffer.c    |   1 -
 .../selftests/bpf/progs/test_stacktrace_map.c |   1 -
 11 files changed, 226 insertions(+), 111 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 bpf-next 1/4] libbpf: stop enforcing kern_version, populate it for users
  2019-10-04 22:40 [PATCH v3 bpf-next 0/4] Add new-style bpf_object__open APIs Andrii Nakryiko
@ 2019-10-04 22:40 ` Andrii Nakryiko
  2019-10-07 16:14   ` Stanislav Fomichev
  2019-10-04 22:40 ` [PATCH v3 bpf-next 2/4] libbpf: add bpf_object__open_{file,mem} w/ extensible opts Andrii Nakryiko
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2019-10-04 22:40 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Kernel version enforcement for kprobes/kretprobes was removed from
5.0 kernel in 6c4fc209fcf9 ("bpf: remove useless version check for prog load").
Since then, BPF programs were specifying SEC("version") just to please
libbpf. We should stop enforcing this in libbpf, if even kernel doesn't
care. Furthermore, libbpf now will pre-populate current kernel version
of the host system, in case we are still running on old kernel.

This patch also removes __bpf_object__open_xattr from libbpf.h, as
nothing in libbpf is relying on having it in that header. That function
was never exported as LIBBPF_API and even name suggests its internal
version. So this should be safe to remove, as it doesn't break ABI.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c                        | 100 ++++--------------
 tools/lib/bpf/libbpf.h                        |   2 -
 .../selftests/bpf/progs/test_attach_probe.c   |   1 -
 .../bpf/progs/test_get_stack_rawtp.c          |   1 -
 .../selftests/bpf/progs/test_perf_buffer.c    |   1 -
 .../selftests/bpf/progs/test_stacktrace_map.c |   1 -
 6 files changed, 23 insertions(+), 83 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index e0276520171b..024334b29b54 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -33,6 +33,7 @@
 #include <linux/limits.h>
 #include <linux/perf_event.h>
 #include <linux/ring_buffer.h>
+#include <linux/version.h>
 #include <sys/epoll.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
@@ -255,7 +256,7 @@ struct bpf_object {
 	 */
 	struct {
 		int fd;
-		void *obj_buf;
+		const void *obj_buf;
 		size_t obj_buf_sz;
 		Elf *elf;
 		GElf_Ehdr ehdr;
@@ -491,8 +492,19 @@ bpf_object__init_prog_names(struct bpf_object *obj)
 	return 0;
 }
 
+static __u32 get_kernel_version(void)
+{
+	__u32 major, minor, patch;
+	struct utsname info;
+
+	uname(&info);
+	if (sscanf(info.release, "%u.%u.%u", &major, &minor, &patch) != 3)
+		return 0;
+	return KERNEL_VERSION(major, minor, patch);
+}
+
 static struct bpf_object *bpf_object__new(const char *path,
-					  void *obj_buf,
+					  const void *obj_buf,
 					  size_t obj_buf_sz)
 {
 	struct bpf_object *obj;
@@ -526,6 +538,7 @@ static struct bpf_object *bpf_object__new(const char *path,
 	obj->efile.rodata_shndx = -1;
 	obj->efile.bss_shndx = -1;
 
+	obj->kern_version = get_kernel_version();
 	obj->loaded = false;
 
 	INIT_LIST_HEAD(&obj->list);
@@ -569,7 +582,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
 		 * obj_buf should have been validated by
 		 * bpf_object__open_buffer().
 		 */
-		obj->efile.elf = elf_memory(obj->efile.obj_buf,
+		obj->efile.elf = elf_memory((char *)obj->efile.obj_buf,
 					    obj->efile.obj_buf_sz);
 	} else {
 		obj->efile.fd = open(obj->path, O_RDONLY);
@@ -636,21 +649,6 @@ bpf_object__init_license(struct bpf_object *obj, void *data, size_t size)
 	return 0;
 }
 
-static int
-bpf_object__init_kversion(struct bpf_object *obj, void *data, size_t size)
-{
-	__u32 kver;
-
-	if (size != sizeof(kver)) {
-		pr_warning("invalid kver section in %s\n", obj->path);
-		return -LIBBPF_ERRNO__FORMAT;
-	}
-	memcpy(&kver, data, sizeof(kver));
-	obj->kern_version = kver;
-	pr_debug("kernel version of %s is %x\n", obj->path, obj->kern_version);
-	return 0;
-}
-
 static int compare_bpf_map(const void *_a, const void *_b)
 {
 	const struct bpf_map *a = _a;
@@ -1568,11 +1566,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
 			if (err)
 				return err;
 		} else if (strcmp(name, "version") == 0) {
-			err = bpf_object__init_kversion(obj,
-							data->d_buf,
-							data->d_size);
-			if (err)
-				return err;
+			/* skip, we don't need it anymore */
 		} else if (strcmp(name, "maps") == 0) {
 			obj->efile.maps_shndx = idx;
 		} else if (strcmp(name, MAPS_ELF_SEC) == 0) {
@@ -3551,54 +3545,9 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
 	return 0;
 }
 
-static bool bpf_prog_type__needs_kver(enum bpf_prog_type type)
-{
-	switch (type) {
-	case BPF_PROG_TYPE_SOCKET_FILTER:
-	case BPF_PROG_TYPE_SCHED_CLS:
-	case BPF_PROG_TYPE_SCHED_ACT:
-	case BPF_PROG_TYPE_XDP:
-	case BPF_PROG_TYPE_CGROUP_SKB:
-	case BPF_PROG_TYPE_CGROUP_SOCK:
-	case BPF_PROG_TYPE_LWT_IN:
-	case BPF_PROG_TYPE_LWT_OUT:
-	case BPF_PROG_TYPE_LWT_XMIT:
-	case BPF_PROG_TYPE_LWT_SEG6LOCAL:
-	case BPF_PROG_TYPE_SOCK_OPS:
-	case BPF_PROG_TYPE_SK_SKB:
-	case BPF_PROG_TYPE_CGROUP_DEVICE:
-	case BPF_PROG_TYPE_SK_MSG:
-	case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
-	case BPF_PROG_TYPE_LIRC_MODE2:
-	case BPF_PROG_TYPE_SK_REUSEPORT:
-	case BPF_PROG_TYPE_FLOW_DISSECTOR:
-	case BPF_PROG_TYPE_UNSPEC:
-	case BPF_PROG_TYPE_TRACEPOINT:
-	case BPF_PROG_TYPE_RAW_TRACEPOINT:
-	case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
-	case BPF_PROG_TYPE_PERF_EVENT:
-	case BPF_PROG_TYPE_CGROUP_SYSCTL:
-	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
-		return false;
-	case BPF_PROG_TYPE_KPROBE:
-	default:
-		return true;
-	}
-}
-
-static int bpf_object__validate(struct bpf_object *obj, bool needs_kver)
-{
-	if (needs_kver && obj->kern_version == 0) {
-		pr_warning("%s doesn't provide kernel version\n",
-			   obj->path);
-		return -LIBBPF_ERRNO__KVERSION;
-	}
-	return 0;
-}
-
 static struct bpf_object *
-__bpf_object__open(const char *path, void *obj_buf, size_t obj_buf_sz,
-		   bool needs_kver, int flags)
+__bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
+		   int flags)
 {
 	struct bpf_object *obj;
 	int err;
@@ -3617,7 +3566,6 @@ __bpf_object__open(const char *path, void *obj_buf, size_t obj_buf_sz,
 	CHECK_ERR(bpf_object__probe_caps(obj), err, out);
 	CHECK_ERR(bpf_object__elf_collect(obj, flags), err, out);
 	CHECK_ERR(bpf_object__collect_reloc(obj), err, out);
-	CHECK_ERR(bpf_object__validate(obj, needs_kver), err, out);
 
 	bpf_object__elf_finish(obj);
 	return obj;
@@ -3626,8 +3574,8 @@ __bpf_object__open(const char *path, void *obj_buf, size_t obj_buf_sz,
 	return ERR_PTR(err);
 }
 
-struct bpf_object *__bpf_object__open_xattr(struct bpf_object_open_attr *attr,
-					    int flags)
+static struct bpf_object *
+__bpf_object__open_xattr(struct bpf_object_open_attr *attr, int flags)
 {
 	/* param validation */
 	if (!attr->file)
@@ -3635,9 +3583,7 @@ struct bpf_object *__bpf_object__open_xattr(struct bpf_object_open_attr *attr,
 
 	pr_debug("loading %s\n", attr->file);
 
-	return __bpf_object__open(attr->file, NULL, 0,
-				  bpf_prog_type__needs_kver(attr->prog_type),
-				  flags);
+	return __bpf_object__open(attr->file, NULL, 0, flags);
 }
 
 struct bpf_object *bpf_object__open_xattr(struct bpf_object_open_attr *attr)
@@ -3673,7 +3619,7 @@ struct bpf_object *bpf_object__open_buffer(void *obj_buf,
 	}
 	pr_debug("loading object '%s' from buffer\n", name);
 
-	return __bpf_object__open(name, obj_buf, obj_buf_sz, true, true);
+	return __bpf_object__open(name, obj_buf, obj_buf_sz, true);
 }
 
 int bpf_object__unload(struct bpf_object *obj)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index e8f70977d137..2905dffd70b2 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -70,8 +70,6 @@ struct bpf_object_open_attr {
 LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
 LIBBPF_API struct bpf_object *
 bpf_object__open_xattr(struct bpf_object_open_attr *attr);
-struct bpf_object *__bpf_object__open_xattr(struct bpf_object_open_attr *attr,
-					    int flags);
 LIBBPF_API struct bpf_object *bpf_object__open_buffer(void *obj_buf,
 						      size_t obj_buf_sz,
 						      const char *name);
diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
index 63a8dfef893b..534621e38906 100644
--- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
+++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
@@ -49,4 +49,3 @@ int handle_uprobe_return(struct pt_regs *ctx)
 }
 
 char _license[] SEC("license") = "GPL";
-__u32 _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c b/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c
index f8ffa3f3d44b..736b6955bba7 100644
--- a/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c
+++ b/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c
@@ -100,4 +100,3 @@ int bpf_prog1(void *ctx)
 }
 
 char _license[] SEC("license") = "GPL";
-__u32 _version SEC("version") = 1; /* ignored by tracepoints, required by libbpf.a */
diff --git a/tools/testing/selftests/bpf/progs/test_perf_buffer.c b/tools/testing/selftests/bpf/progs/test_perf_buffer.c
index 876c27deb65a..07c09ca5546a 100644
--- a/tools/testing/selftests/bpf/progs/test_perf_buffer.c
+++ b/tools/testing/selftests/bpf/progs/test_perf_buffer.c
@@ -22,4 +22,3 @@ int handle_sys_nanosleep_entry(struct pt_regs *ctx)
 }
 
 char _license[] SEC("license") = "GPL";
-__u32 _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
index fa0be3e10a10..3b7e1dca8829 100644
--- a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
+++ b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
@@ -74,4 +74,3 @@ int oncpu(struct sched_switch_args *ctx)
 }
 
 char _license[] SEC("license") = "GPL";
-__u32 _version SEC("version") = 1; /* ignored by tracepoints, required by libbpf.a */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 bpf-next 2/4] libbpf: add bpf_object__open_{file,mem} w/ extensible opts
  2019-10-04 22:40 [PATCH v3 bpf-next 0/4] Add new-style bpf_object__open APIs Andrii Nakryiko
  2019-10-04 22:40 ` [PATCH v3 bpf-next 1/4] libbpf: stop enforcing kern_version, populate it for users Andrii Nakryiko
@ 2019-10-04 22:40 ` Andrii Nakryiko
  2019-10-06  1:24   ` Alexei Starovoitov
  2019-10-04 22:40 ` [PATCH v3 bpf-next 3/4] libbpf: fix bpf_object__name() to actually return object name Andrii Nakryiko
  2019-10-04 22:40 ` [PATCH v3 bpf-next 4/4] selftests/bpf: switch tests to new bpf_object__open_{file,mem}() APIs Andrii Nakryiko
  3 siblings, 1 reply; 9+ messages in thread
From: Andrii Nakryiko @ 2019-10-04 22:40 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Add new set of bpf_object__open APIs using new approach to optional
parameters extensibility allowing simpler ABI compatibility approach.

This patch demonstrates an approach to implementing libbpf APIs that
makes it easy to extend existing APIs with extra optional parameters in
such a way, that ABI compatibility is preserved without having to do
symbol versioning and generating lots of boilerplate code to handle it.
To facilitate succinct code for working with options, add OPTS_VALID,
OPTS_HAS, and OPTS_GET macros that hide all the NULL, size, and zero
checks.

Additionally, newly added libbpf APIs are encouraged to follow similar
pattern of having all mandatory parameters as formal function parameters
and always have optional (NULL-able) xxx_opts struct, which should
always have real struct size as a first field and the rest would be
optional parameters added over time, which tune the behavior of existing
API, if specified by user.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c          | 87 ++++++++++++++++++++++++++-------
 tools/lib/bpf/libbpf.h          | 46 +++++++++++++++--
 tools/lib/bpf/libbpf.map        |  3 ++
 tools/lib/bpf/libbpf_internal.h | 32 ++++++++++++
 4 files changed, 146 insertions(+), 22 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 024334b29b54..d471d33400ae 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -505,7 +505,8 @@ static __u32 get_kernel_version(void)
 
 static struct bpf_object *bpf_object__new(const char *path,
 					  const void *obj_buf,
-					  size_t obj_buf_sz)
+					  size_t obj_buf_sz,
+					  const char *obj_name)
 {
 	struct bpf_object *obj;
 	char *end;
@@ -517,11 +518,17 @@ static struct bpf_object *bpf_object__new(const char *path,
 	}
 
 	strcpy(obj->path, path);
-	/* Using basename() GNU version which doesn't modify arg. */
-	strncpy(obj->name, basename((void *)path), sizeof(obj->name) - 1);
-	end = strchr(obj->name, '.');
-	if (end)
-		*end = 0;
+	if (obj_name) {
+		strncpy(obj->name, obj_name, sizeof(obj->name) - 1);
+		obj->name[sizeof(obj->name) - 1] = 0;
+	} else {
+		/* Using basename() GNU version which doesn't modify arg. */
+		strncpy(obj->name, basename((void *)path),
+			sizeof(obj->name) - 1);
+		end = strchr(obj->name, '.');
+		if (end)
+			*end = 0;
+	}
 
 	obj->efile.fd = -1;
 	/*
@@ -3547,7 +3554,7 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
 
 static struct bpf_object *
 __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
-		   int flags)
+		   const char *obj_name, int flags)
 {
 	struct bpf_object *obj;
 	int err;
@@ -3557,7 +3564,7 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
 		return ERR_PTR(-LIBBPF_ERRNO__LIBELF);
 	}
 
-	obj = bpf_object__new(path, obj_buf, obj_buf_sz);
+	obj = bpf_object__new(path, obj_buf, obj_buf_sz, obj_name);
 	if (IS_ERR(obj))
 		return obj;
 
@@ -3583,7 +3590,7 @@ __bpf_object__open_xattr(struct bpf_object_open_attr *attr, int flags)
 
 	pr_debug("loading %s\n", attr->file);
 
-	return __bpf_object__open(attr->file, NULL, 0, flags);
+	return __bpf_object__open(attr->file, NULL, 0, NULL, flags);
 }
 
 struct bpf_object *bpf_object__open_xattr(struct bpf_object_open_attr *attr)
@@ -3601,25 +3608,67 @@ struct bpf_object *bpf_object__open(const char *path)
 	return bpf_object__open_xattr(&attr);
 }
 
-struct bpf_object *bpf_object__open_buffer(void *obj_buf,
-					   size_t obj_buf_sz,
-					   const char *name)
+struct bpf_object *
+bpf_object__open_file(const char *path, struct bpf_object_open_opts *opts)
+{
+	const char *obj_name;
+	bool relaxed_maps;
+
+	if (!OPTS_VALID(opts, bpf_object_open_opts))
+		return ERR_PTR(-EINVAL);
+	if (!path)
+		return ERR_PTR(-EINVAL);
+
+	pr_debug("loading %s\n", path);
+
+	obj_name = OPTS_GET(opts, object_name, path);
+	relaxed_maps = OPTS_GET(opts, relaxed_maps, false);
+	return __bpf_object__open(path, NULL, 0, obj_name,
+				  relaxed_maps ? MAPS_RELAX_COMPAT : 0);
+}
+
+struct bpf_object *
+bpf_object__open_mem(const void *obj_buf, size_t obj_buf_sz,
+		     struct bpf_object_open_opts *opts)
 {
 	char tmp_name[64];
+	const char *obj_name;
+	bool relaxed_maps;
 
-	/* param validation */
-	if (!obj_buf || obj_buf_sz <= 0)
-		return NULL;
+	if (!OPTS_VALID(opts, bpf_object_open_opts))
+		return ERR_PTR(-EINVAL);
+	if (!obj_buf || obj_buf_sz == 0)
+		return ERR_PTR(-EINVAL);
 
-	if (!name) {
+	obj_name = OPTS_GET(opts, object_name, NULL);
+	if (!obj_name) {
 		snprintf(tmp_name, sizeof(tmp_name), "%lx-%lx",
 			 (unsigned long)obj_buf,
 			 (unsigned long)obj_buf_sz);
-		name = tmp_name;
+		obj_name = tmp_name;
 	}
-	pr_debug("loading object '%s' from buffer\n", name);
+	pr_debug("loading object '%s' from buffer\n", obj_name);
+
+	relaxed_maps = OPTS_GET(opts, relaxed_maps, false);
+	return __bpf_object__open(obj_name, obj_buf, obj_buf_sz, obj_name,
+				  relaxed_maps ? MAPS_RELAX_COMPAT : 0);
+}
+
+struct bpf_object *
+bpf_object__open_buffer(const void *obj_buf, size_t obj_buf_sz,
+			const char *name)
+{
+	LIBBPF_OPTS(bpf_object_open_opts, opts,
+		.object_name = name,
+		/* wrong default, but backwards-compatible */
+		.relaxed_maps = true,
+	);
+
+	/* returning NULL is wrong, but backwards-compatible */
+	if (!obj_buf || obj_buf_sz == 0)
+		return NULL;
 
-	return __bpf_object__open(name, obj_buf, obj_buf_sz, true);
+	return bpf_object__open_mem(obj_buf, obj_buf_sz, &opts);
 }
 
 int bpf_object__unload(struct bpf_object *obj)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 2905dffd70b2..667e6853e51f 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -67,12 +67,52 @@ struct bpf_object_open_attr {
 	enum bpf_prog_type prog_type;
 };
 
+/* Helper macro to declare and initialize libbpf options struct
+ *
+ * This dance with uninitialized declaration, followed by memset to zero,
+ * followed by assignment using compound literal syntax is done to preserve
+ * ability to use a nice struct field initialization syntax and **hopefully**
+ * have all the padding bytes initialized to zero. It's not guaranteed though,
+ * when copying literal, that compiler won't copy garbage in literal's padding
+ * bytes, but that's the best way I've found and it seems to work in practice.
+ */
+#define LIBBPF_OPTS(TYPE, NAME, ...)					    \
+	struct TYPE NAME;						    \
+	memset(&NAME, 0, sizeof(struct TYPE));				    \
+	NAME = (struct TYPE) {						    \
+		.sz = sizeof(struct TYPE),				    \
+		__VA_ARGS__						    \
+	}
+
+struct bpf_object_open_opts {
+	/* size of this struct, for forward/backward compatiblity */
+	size_t sz;
+	/* object name override, if provided:
+	 * - for object open from file, this will override setting object
+	 *   name from file path's base name;
+	 * - for object open from memory buffer, this will specify an object
+	 *   name and will override default "<addr>-<buf-size>" name;
+	 */
+	const char *object_name;
+	/* parse map definitions non-strictly, allowing extra attributes/data */
+	bool relaxed_maps;
+};
+#define bpf_object_open_opts__last_field relaxed_maps
+
 LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
 LIBBPF_API struct bpf_object *
+bpf_object__open_file(const char *path, struct bpf_object_open_opts *opts);
+LIBBPF_API struct bpf_object *
+bpf_object__open_mem(const void *obj_buf, size_t obj_buf_sz,
+		     struct bpf_object_open_opts *opts);
+
+/* deprecated bpf_object__open variants */
+LIBBPF_API struct bpf_object *
+bpf_object__open_buffer(const void *obj_buf, size_t obj_buf_sz,
+			const char *name);
+LIBBPF_API struct bpf_object *
 bpf_object__open_xattr(struct bpf_object_open_attr *attr);
-LIBBPF_API struct bpf_object *bpf_object__open_buffer(void *obj_buf,
-						      size_t obj_buf_sz,
-						      const char *name);
+
 int bpf_object__section_size(const struct bpf_object *obj, const char *name,
 			     __u32 *size);
 int bpf_object__variable_offset(const struct bpf_object *obj, const char *name,
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 8d10ca03d78d..4d241fd92dd4 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -192,4 +192,7 @@ LIBBPF_0.0.5 {
 } LIBBPF_0.0.4;
 
 LIBBPF_0.0.6 {
+	global:
+		bpf_object__open_file;
+		bpf_object__open_mem;
 } LIBBPF_0.0.5;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 2e83a34f8c79..f51444fc7eb7 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -47,6 +47,38 @@ do {				\
 #define pr_info(fmt, ...)	__pr(LIBBPF_INFO, fmt, ##__VA_ARGS__)
 #define pr_debug(fmt, ...)	__pr(LIBBPF_DEBUG, fmt, ##__VA_ARGS__)
 
+static inline bool libbpf_validate_opts(const char *opts,
+					size_t opts_sz, size_t user_sz,
+					const char *type_name)
+{
+	if (user_sz < sizeof(size_t)) {
+		pr_warning("%s size (%zu) is too small\n", type_name, user_sz);
+		return false;
+	}
+	if (user_sz > opts_sz) {
+		size_t i;
+
+		for (i = opts_sz; i < user_sz; i++) {
+			if (opts[i]) {
+				pr_warning("%s has non-zero extra bytes",
+					   type_name);
+				return false;
+			}
+		}
+	}
+	return true;
+}
+
+#define OPTS_VALID(opts, type)						      \
+	(!(opts) || libbpf_validate_opts((const char *)opts,		      \
+					 offsetofend(struct type,	      \
+						     type##__last_field),     \
+					 (opts)->sz, #type))
+#define OPTS_HAS(opts, field) \
+	((opts) && opts->sz >= offsetofend(typeof(*(opts)), field))
+#define OPTS_GET(opts, field, fallback_value) \
+	(OPTS_HAS(opts, field) ? (opts)->field : fallback_value)
+
 int libbpf__load_raw_btf(const char *raw_types, size_t types_len,
 			 const char *str_sec, size_t str_len);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 bpf-next 3/4] libbpf: fix bpf_object__name() to actually return object name
  2019-10-04 22:40 [PATCH v3 bpf-next 0/4] Add new-style bpf_object__open APIs Andrii Nakryiko
  2019-10-04 22:40 ` [PATCH v3 bpf-next 1/4] libbpf: stop enforcing kern_version, populate it for users Andrii Nakryiko
  2019-10-04 22:40 ` [PATCH v3 bpf-next 2/4] libbpf: add bpf_object__open_{file,mem} w/ extensible opts Andrii Nakryiko
@ 2019-10-04 22:40 ` Andrii Nakryiko
  2019-10-04 22:40 ` [PATCH v3 bpf-next 4/4] selftests/bpf: switch tests to new bpf_object__open_{file,mem}() APIs Andrii Nakryiko
  3 siblings, 0 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2019-10-04 22:40 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

bpf_object__name() was returning file path, not name. Fix this.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/lib/bpf/libbpf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d471d33400ae..a02cdedc4e3f 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4231,7 +4231,7 @@ bpf_object__next(struct bpf_object *prev)
 
 const char *bpf_object__name(const struct bpf_object *obj)
 {
-	return obj ? obj->path : ERR_PTR(-EINVAL);
+	return obj ? obj->name : ERR_PTR(-EINVAL);
 }
 
 unsigned int bpf_object__kversion(const struct bpf_object *obj)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 bpf-next 4/4] selftests/bpf: switch tests to new bpf_object__open_{file,mem}() APIs
  2019-10-04 22:40 [PATCH v3 bpf-next 0/4] Add new-style bpf_object__open APIs Andrii Nakryiko
                   ` (2 preceding siblings ...)
  2019-10-04 22:40 ` [PATCH v3 bpf-next 3/4] libbpf: fix bpf_object__name() to actually return object name Andrii Nakryiko
@ 2019-10-04 22:40 ` Andrii Nakryiko
  3 siblings, 0 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2019-10-04 22:40 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Verify new bpf_object__open_mem() and bpf_object__open_file() APIs work
as expected by switching test_attach_probe test to use embedded BPF
object and bpf_object__open_mem() and test_reference_tracking to
bpf_object__open_file().

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/testing/selftests/bpf/Makefile          |  2 +-
 .../selftests/bpf/prog_tests/attach_probe.c   | 49 +++++++++++++++++--
 .../bpf/prog_tests/reference_tracking.c       | 16 +++++-
 3 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 6889c19a628c..294d7472dad7 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -160,7 +160,7 @@ $(OUTPUT)/test_queue_map.o: test_queue_stack_map.h
 $(OUTPUT)/test_stack_map.o: test_queue_stack_map.h
 
 $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h
-$(OUTPUT)/test_progs.o: flow_dissector_load.h
+$(OUTPUT)/test_progs.o: flow_dissector_load.h $(OUTPUT)/test_attach_probe.o
 
 BTF_LLC_PROBE := $(shell $(LLC) -march=bpf -mattr=help 2>&1 | grep dwarfris)
 BTF_PAHOLE_PROBE := $(shell $(BTF_PAHOLE) --help 2>&1 | grep BTF)
diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
index 5ecc267d98b0..4f50d32c4abb 100644
--- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
+++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
@@ -1,6 +1,24 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
 
+#define EMBED_FILE(NAME, PATH)						    \
+asm (									    \
+"      .pushsection \".rodata\", \"a\", @progbits              \n"	    \
+"      .global "#NAME"_data                                    \n"	    \
+#NAME"_data:                                                   \n"	    \
+"      .incbin \"" PATH "\"                                    \n"	    \
+#NAME"_data_end:                                               \n"	    \
+"      .global "#NAME"_size                                    \n"	    \
+"      .type "#NAME"_size, @object                             \n"	    \
+"      .size "#NAME"_size, 4                                   \n"	    \
+"      .align 4,                                               \n"	    \
+#NAME"_size:                                                   \n"	    \
+"      .int "#NAME"_data_end - "#NAME"_data                    \n"	    \
+"      .popsection                                             \n"	    \
+);									    \
+extern char NAME##_data[];						    \
+extern int NAME##_size;
+
 ssize_t get_base_addr() {
 	size_t start;
 	char buf[256];
@@ -21,6 +39,8 @@ ssize_t get_base_addr() {
 	return -EINVAL;
 }
 
+EMBED_FILE(probe, "test_attach_probe.o");
+
 void test_attach_probe(void)
 {
 	const char *kprobe_name = "kprobe/sys_nanosleep";
@@ -29,11 +49,15 @@ void test_attach_probe(void)
 	const char *uretprobe_name = "uretprobe/trigger_func";
 	const int kprobe_idx = 0, kretprobe_idx = 1;
 	const int uprobe_idx = 2, uretprobe_idx = 3;
-	const char *file = "./test_attach_probe.o";
+	const char *obj_name = "attach_probe";
+	LIBBPF_OPTS(bpf_object_open_opts, open_opts,
+		.object_name = obj_name,
+		.relaxed_maps = true,
+	);
 	struct bpf_program *kprobe_prog, *kretprobe_prog;
 	struct bpf_program *uprobe_prog, *uretprobe_prog;
 	struct bpf_object *obj;
-	int err, prog_fd, duration = 0, res;
+	int err, duration = 0, res;
 	struct bpf_link *kprobe_link = NULL;
 	struct bpf_link *kretprobe_link = NULL;
 	struct bpf_link *uprobe_link = NULL;
@@ -48,11 +72,16 @@ void test_attach_probe(void)
 		return;
 	uprobe_offset = (size_t)&get_base_addr - base_addr;
 
-	/* load programs */
-	err = bpf_prog_load(file, BPF_PROG_TYPE_KPROBE, &obj, &prog_fd);
-	if (CHECK(err, "obj_load", "err %d errno %d\n", err, errno))
+	/* open object */
+	obj = bpf_object__open_mem(probe_data, probe_size, &open_opts);
+	if (CHECK(IS_ERR(obj), "obj_open_mem", "err %ld\n", PTR_ERR(obj)))
 		return;
 
+	if (CHECK(strcmp(bpf_object__name(obj), obj_name), "obj_name",
+		  "wrong obj name '%s', expected '%s'\n",
+		  bpf_object__name(obj), obj_name))
+		goto cleanup;
+
 	kprobe_prog = bpf_object__find_program_by_title(obj, kprobe_name);
 	if (CHECK(!kprobe_prog, "find_probe",
 		  "prog '%s' not found\n", kprobe_name))
@@ -70,6 +99,16 @@ void test_attach_probe(void)
 		  "prog '%s' not found\n", uretprobe_name))
 		goto cleanup;
 
+	bpf_program__set_kprobe(kprobe_prog);
+	bpf_program__set_kprobe(kretprobe_prog);
+	bpf_program__set_kprobe(uprobe_prog);
+	bpf_program__set_kprobe(uretprobe_prog);
+
+	/* create maps && load programs */
+	err = bpf_object__load(obj);
+	if (CHECK(err, "obj_load", "err %d\n", err))
+		goto cleanup;
+
 	/* load maps */
 	results_map_fd = bpf_find_map(__func__, obj, "results_map");
 	if (CHECK(results_map_fd < 0, "find_results_map",
diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
index 5c78e2b5a917..86cee820d4d3 100644
--- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
+++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
@@ -3,16 +3,26 @@
 
 void test_reference_tracking(void)
 {
-	const char *file = "./test_sk_lookup_kern.o";
+	const char *file = "test_sk_lookup_kern.o";
+	const char *obj_name = "ref_track";
+	LIBBPF_OPTS(bpf_object_open_opts, open_opts,
+		.object_name = obj_name,
+		.relaxed_maps = true,
+	);
 	struct bpf_object *obj;
 	struct bpf_program *prog;
 	__u32 duration = 0;
 	int err = 0;
 
-	obj = bpf_object__open(file);
+	obj = bpf_object__open_file(file, &open_opts);
 	if (CHECK_FAIL(IS_ERR(obj)))
 		return;
 
+	if (CHECK(strcmp(bpf_object__name(obj), obj_name), "obj_name",
+		  "wrong obj name '%s', expected '%s'\n",
+		  bpf_object__name(obj), obj_name))
+		goto cleanup;
+
 	bpf_object__for_each_program(prog, obj) {
 		const char *title;
 
@@ -35,5 +45,7 @@ void test_reference_tracking(void)
 		}
 		CHECK(err, title, "\n");
 	}
+
+cleanup:
 	bpf_object__close(obj);
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 bpf-next 2/4] libbpf: add bpf_object__open_{file,mem} w/ extensible opts
  2019-10-04 22:40 ` [PATCH v3 bpf-next 2/4] libbpf: add bpf_object__open_{file,mem} w/ extensible opts Andrii Nakryiko
@ 2019-10-06  1:24   ` Alexei Starovoitov
  2019-10-06  2:35     ` Andrii Nakryiko
  0 siblings, 1 reply; 9+ messages in thread
From: Alexei Starovoitov @ 2019-10-06  1:24 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team

On Fri, Oct 04, 2019 at 03:40:35PM -0700, Andrii Nakryiko wrote:
> Add new set of bpf_object__open APIs using new approach to optional
> parameters extensibility allowing simpler ABI compatibility approach.
> 
> This patch demonstrates an approach to implementing libbpf APIs that
> makes it easy to extend existing APIs with extra optional parameters in
> such a way, that ABI compatibility is preserved without having to do
> symbol versioning and generating lots of boilerplate code to handle it.
> To facilitate succinct code for working with options, add OPTS_VALID,
> OPTS_HAS, and OPTS_GET macros that hide all the NULL, size, and zero
> checks.
> 
> Additionally, newly added libbpf APIs are encouraged to follow similar
> pattern of having all mandatory parameters as formal function parameters
> and always have optional (NULL-able) xxx_opts struct, which should
> always have real struct size as a first field and the rest would be
> optional parameters added over time, which tune the behavior of existing
> API, if specified by user.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
...
> +/* Helper macro to declare and initialize libbpf options struct
> + *
> + * This dance with uninitialized declaration, followed by memset to zero,
> + * followed by assignment using compound literal syntax is done to preserve
> + * ability to use a nice struct field initialization syntax and **hopefully**
> + * have all the padding bytes initialized to zero. It's not guaranteed though,
> + * when copying literal, that compiler won't copy garbage in literal's padding
> + * bytes, but that's the best way I've found and it seems to work in practice.
> + */
> +#define LIBBPF_OPTS(TYPE, NAME, ...)					    \
> +	struct TYPE NAME;						    \
> +	memset(&NAME, 0, sizeof(struct TYPE));				    \
> +	NAME = (struct TYPE) {						    \
> +		.sz = sizeof(struct TYPE),				    \
> +		__VA_ARGS__						    \
> +	}
> +
> +struct bpf_object_open_opts {
> +	/* size of this struct, for forward/backward compatiblity */
> +	size_t sz;
> +	/* object name override, if provided:
> +	 * - for object open from file, this will override setting object
> +	 *   name from file path's base name;
> +	 * - for object open from memory buffer, this will specify an object
> +	 *   name and will override default "<addr>-<buf-size>" name;
> +	 */
> +	const char *object_name;
> +	/* parse map definitions non-strictly, allowing extra attributes/data */
> +	bool relaxed_maps;
> +};
> +#define bpf_object_open_opts__last_field relaxed_maps

LIBBPF_OPTS macro doesn't inspire confidence, but despite the ugliness
it is strictly better than what libbpf is using internally to interface
into kernel via similar bpf_attr api.
So I think it's an improvement.
Should this macro be used inside libbpf as well?
May be rename it too to show that it's not libbpf specific?

Anyhow all that can be done in follow up.

Applied. Thanks


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 bpf-next 2/4] libbpf: add bpf_object__open_{file,mem} w/ extensible opts
  2019-10-06  1:24   ` Alexei Starovoitov
@ 2019-10-06  2:35     ` Andrii Nakryiko
  0 siblings, 0 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2019-10-06  2:35 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

On Sat, Oct 5, 2019 at 6:24 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Oct 04, 2019 at 03:40:35PM -0700, Andrii Nakryiko wrote:
> > Add new set of bpf_object__open APIs using new approach to optional
> > parameters extensibility allowing simpler ABI compatibility approach.
> >
> > This patch demonstrates an approach to implementing libbpf APIs that
> > makes it easy to extend existing APIs with extra optional parameters in
> > such a way, that ABI compatibility is preserved without having to do
> > symbol versioning and generating lots of boilerplate code to handle it.
> > To facilitate succinct code for working with options, add OPTS_VALID,
> > OPTS_HAS, and OPTS_GET macros that hide all the NULL, size, and zero
> > checks.
> >
> > Additionally, newly added libbpf APIs are encouraged to follow similar
> > pattern of having all mandatory parameters as formal function parameters
> > and always have optional (NULL-able) xxx_opts struct, which should
> > always have real struct size as a first field and the rest would be
> > optional parameters added over time, which tune the behavior of existing
> > API, if specified by user.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ...
> > +/* Helper macro to declare and initialize libbpf options struct
> > + *
> > + * This dance with uninitialized declaration, followed by memset to zero,
> > + * followed by assignment using compound literal syntax is done to preserve
> > + * ability to use a nice struct field initialization syntax and **hopefully**
> > + * have all the padding bytes initialized to zero. It's not guaranteed though,
> > + * when copying literal, that compiler won't copy garbage in literal's padding
> > + * bytes, but that's the best way I've found and it seems to work in practice.
> > + */
> > +#define LIBBPF_OPTS(TYPE, NAME, ...)                                     \
> > +     struct TYPE NAME;                                                   \
> > +     memset(&NAME, 0, sizeof(struct TYPE));                              \
> > +     NAME = (struct TYPE) {                                              \
> > +             .sz = sizeof(struct TYPE),                                  \
> > +             __VA_ARGS__                                                 \
> > +     }
> > +
> > +struct bpf_object_open_opts {
> > +     /* size of this struct, for forward/backward compatiblity */
> > +     size_t sz;
> > +     /* object name override, if provided:
> > +      * - for object open from file, this will override setting object
> > +      *   name from file path's base name;
> > +      * - for object open from memory buffer, this will specify an object
> > +      *   name and will override default "<addr>-<buf-size>" name;
> > +      */
> > +     const char *object_name;
> > +     /* parse map definitions non-strictly, allowing extra attributes/data */
> > +     bool relaxed_maps;
> > +};
> > +#define bpf_object_open_opts__last_field relaxed_maps
>
> LIBBPF_OPTS macro doesn't inspire confidence, but despite the ugliness
> it is strictly better than what libbpf is using internally to interface
> into kernel via similar bpf_attr api.
> So I think it's an improvement.
> Should this macro be used inside libbpf as well?
> May be rename it too to show that it's not libbpf specific?
>
> Anyhow all that can be done in follow up.
>
> Applied. Thanks
>

Thanks!

I think I'll keep LIBBPF_OPTS because it's specific to this xxx_opts
convention, which has .sz field. bpf_attr doesn't have that. But I'll
create a similar macro for internal libbpf usage and will put it into
bpf_internal.h.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 bpf-next 1/4] libbpf: stop enforcing kern_version, populate it for users
  2019-10-04 22:40 ` [PATCH v3 bpf-next 1/4] libbpf: stop enforcing kern_version, populate it for users Andrii Nakryiko
@ 2019-10-07 16:14   ` Stanislav Fomichev
  2019-10-07 16:42     ` Andrii Nakryiko
  0 siblings, 1 reply; 9+ messages in thread
From: Stanislav Fomichev @ 2019-10-07 16:14 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team

On 10/04, Andrii Nakryiko wrote:
> Kernel version enforcement for kprobes/kretprobes was removed from
> 5.0 kernel in 6c4fc209fcf9 ("bpf: remove useless version check for prog load").
> Since then, BPF programs were specifying SEC("version") just to please
> libbpf. We should stop enforcing this in libbpf, if even kernel doesn't
> care. Furthermore, libbpf now will pre-populate current kernel version
> of the host system, in case we are still running on old kernel.

[..]
> This patch also removes __bpf_object__open_xattr from libbpf.h, as
> nothing in libbpf is relying on having it in that header. That function
> was never exported as LIBBPF_API and even name suggests its internal
> version. So this should be safe to remove, as it doesn't break ABI.
This gives me the following (I don't know why bpftool was allowed to link
against non-LIBBPF_API exposed function):

+ make -s -j72 -C tools/bpf/bpftool

prog.c: In function ‘load_with_options’:
prog.c:1227:8: warning: implicit declaration of function ‘__bpf_object__open_xattr’; did you mean ‘bpf_object__open_xattr’? [-Wimplicit-function-declaration]
  obj = __bpf_object__open_xattr(&open_attr, bpf_flags);
        ^~~~~~~~~~~~~~~~~~~~~~~~
        bpf_object__open_xattr
prog.c:1227:8: warning: nested extern declaration of ‘__bpf_object__open_xattr’ [-Wnested-externs]   
prog.c:1227:6: warning: assignment to ‘struct bpf_object *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
  obj = __bpf_object__open_xattr(&open_attr, bpf_flags);
      ^

Warning: Kernel ABI header at 'tools/include/uapi/linux/if_link.h' differs from latest version at 'include/uapi/linux/if_link.h'
/usr/bin/ld: prog.o: in function `load_with_options':
prog.c:(.text+0x49b): undefined reference to `__bpf_object__open_xattr'
collect2: error: ld returned 1 exit statu

> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  tools/lib/bpf/libbpf.c                        | 100 ++++--------------
>  tools/lib/bpf/libbpf.h                        |   2 -
>  .../selftests/bpf/progs/test_attach_probe.c   |   1 -
>  .../bpf/progs/test_get_stack_rawtp.c          |   1 -
>  .../selftests/bpf/progs/test_perf_buffer.c    |   1 -
>  .../selftests/bpf/progs/test_stacktrace_map.c |   1 -
>  6 files changed, 23 insertions(+), 83 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index e0276520171b..024334b29b54 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -33,6 +33,7 @@
>  #include <linux/limits.h>
>  #include <linux/perf_event.h>
>  #include <linux/ring_buffer.h>
> +#include <linux/version.h>
>  #include <sys/epoll.h>
>  #include <sys/ioctl.h>
>  #include <sys/mman.h>
> @@ -255,7 +256,7 @@ struct bpf_object {
>  	 */
>  	struct {
>  		int fd;
> -		void *obj_buf;
> +		const void *obj_buf;
>  		size_t obj_buf_sz;
>  		Elf *elf;
>  		GElf_Ehdr ehdr;
> @@ -491,8 +492,19 @@ bpf_object__init_prog_names(struct bpf_object *obj)
>  	return 0;
>  }
>  
> +static __u32 get_kernel_version(void)
> +{
> +	__u32 major, minor, patch;
> +	struct utsname info;
> +
> +	uname(&info);
> +	if (sscanf(info.release, "%u.%u.%u", &major, &minor, &patch) != 3)
> +		return 0;
> +	return KERNEL_VERSION(major, minor, patch);
> +}
> +
>  static struct bpf_object *bpf_object__new(const char *path,
> -					  void *obj_buf,
> +					  const void *obj_buf,
>  					  size_t obj_buf_sz)
>  {
>  	struct bpf_object *obj;
> @@ -526,6 +538,7 @@ static struct bpf_object *bpf_object__new(const char *path,
>  	obj->efile.rodata_shndx = -1;
>  	obj->efile.bss_shndx = -1;
>  
> +	obj->kern_version = get_kernel_version();
>  	obj->loaded = false;
>  
>  	INIT_LIST_HEAD(&obj->list);
> @@ -569,7 +582,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
>  		 * obj_buf should have been validated by
>  		 * bpf_object__open_buffer().
>  		 */
> -		obj->efile.elf = elf_memory(obj->efile.obj_buf,
> +		obj->efile.elf = elf_memory((char *)obj->efile.obj_buf,
>  					    obj->efile.obj_buf_sz);
>  	} else {
>  		obj->efile.fd = open(obj->path, O_RDONLY);
> @@ -636,21 +649,6 @@ bpf_object__init_license(struct bpf_object *obj, void *data, size_t size)
>  	return 0;
>  }
>  
> -static int
> -bpf_object__init_kversion(struct bpf_object *obj, void *data, size_t size)
> -{
> -	__u32 kver;
> -
> -	if (size != sizeof(kver)) {
> -		pr_warning("invalid kver section in %s\n", obj->path);
> -		return -LIBBPF_ERRNO__FORMAT;
> -	}
> -	memcpy(&kver, data, sizeof(kver));
> -	obj->kern_version = kver;
> -	pr_debug("kernel version of %s is %x\n", obj->path, obj->kern_version);
> -	return 0;
> -}
> -
>  static int compare_bpf_map(const void *_a, const void *_b)
>  {
>  	const struct bpf_map *a = _a;
> @@ -1568,11 +1566,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
>  			if (err)
>  				return err;
>  		} else if (strcmp(name, "version") == 0) {
> -			err = bpf_object__init_kversion(obj,
> -							data->d_buf,
> -							data->d_size);
> -			if (err)
> -				return err;
> +			/* skip, we don't need it anymore */
>  		} else if (strcmp(name, "maps") == 0) {
>  			obj->efile.maps_shndx = idx;
>  		} else if (strcmp(name, MAPS_ELF_SEC) == 0) {
> @@ -3551,54 +3545,9 @@ bpf_object__load_progs(struct bpf_object *obj, int log_level)
>  	return 0;
>  }
>  
> -static bool bpf_prog_type__needs_kver(enum bpf_prog_type type)
> -{
> -	switch (type) {
> -	case BPF_PROG_TYPE_SOCKET_FILTER:
> -	case BPF_PROG_TYPE_SCHED_CLS:
> -	case BPF_PROG_TYPE_SCHED_ACT:
> -	case BPF_PROG_TYPE_XDP:
> -	case BPF_PROG_TYPE_CGROUP_SKB:
> -	case BPF_PROG_TYPE_CGROUP_SOCK:
> -	case BPF_PROG_TYPE_LWT_IN:
> -	case BPF_PROG_TYPE_LWT_OUT:
> -	case BPF_PROG_TYPE_LWT_XMIT:
> -	case BPF_PROG_TYPE_LWT_SEG6LOCAL:
> -	case BPF_PROG_TYPE_SOCK_OPS:
> -	case BPF_PROG_TYPE_SK_SKB:
> -	case BPF_PROG_TYPE_CGROUP_DEVICE:
> -	case BPF_PROG_TYPE_SK_MSG:
> -	case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
> -	case BPF_PROG_TYPE_LIRC_MODE2:
> -	case BPF_PROG_TYPE_SK_REUSEPORT:
> -	case BPF_PROG_TYPE_FLOW_DISSECTOR:
> -	case BPF_PROG_TYPE_UNSPEC:
> -	case BPF_PROG_TYPE_TRACEPOINT:
> -	case BPF_PROG_TYPE_RAW_TRACEPOINT:
> -	case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
> -	case BPF_PROG_TYPE_PERF_EVENT:
> -	case BPF_PROG_TYPE_CGROUP_SYSCTL:
> -	case BPF_PROG_TYPE_CGROUP_SOCKOPT:
> -		return false;
> -	case BPF_PROG_TYPE_KPROBE:
> -	default:
> -		return true;
> -	}
> -}
> -
> -static int bpf_object__validate(struct bpf_object *obj, bool needs_kver)
> -{
> -	if (needs_kver && obj->kern_version == 0) {
> -		pr_warning("%s doesn't provide kernel version\n",
> -			   obj->path);
> -		return -LIBBPF_ERRNO__KVERSION;
> -	}
> -	return 0;
> -}
> -
>  static struct bpf_object *
> -__bpf_object__open(const char *path, void *obj_buf, size_t obj_buf_sz,
> -		   bool needs_kver, int flags)
> +__bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
> +		   int flags)
>  {
>  	struct bpf_object *obj;
>  	int err;
> @@ -3617,7 +3566,6 @@ __bpf_object__open(const char *path, void *obj_buf, size_t obj_buf_sz,
>  	CHECK_ERR(bpf_object__probe_caps(obj), err, out);
>  	CHECK_ERR(bpf_object__elf_collect(obj, flags), err, out);
>  	CHECK_ERR(bpf_object__collect_reloc(obj), err, out);
> -	CHECK_ERR(bpf_object__validate(obj, needs_kver), err, out);
>  
>  	bpf_object__elf_finish(obj);
>  	return obj;
> @@ -3626,8 +3574,8 @@ __bpf_object__open(const char *path, void *obj_buf, size_t obj_buf_sz,
>  	return ERR_PTR(err);
>  }
>  
> -struct bpf_object *__bpf_object__open_xattr(struct bpf_object_open_attr *attr,
> -					    int flags)
> +static struct bpf_object *
> +__bpf_object__open_xattr(struct bpf_object_open_attr *attr, int flags)
>  {
>  	/* param validation */
>  	if (!attr->file)
> @@ -3635,9 +3583,7 @@ struct bpf_object *__bpf_object__open_xattr(struct bpf_object_open_attr *attr,
>  
>  	pr_debug("loading %s\n", attr->file);
>  
> -	return __bpf_object__open(attr->file, NULL, 0,
> -				  bpf_prog_type__needs_kver(attr->prog_type),
> -				  flags);
> +	return __bpf_object__open(attr->file, NULL, 0, flags);
>  }
>  
>  struct bpf_object *bpf_object__open_xattr(struct bpf_object_open_attr *attr)
> @@ -3673,7 +3619,7 @@ struct bpf_object *bpf_object__open_buffer(void *obj_buf,
>  	}
>  	pr_debug("loading object '%s' from buffer\n", name);
>  
> -	return __bpf_object__open(name, obj_buf, obj_buf_sz, true, true);
> +	return __bpf_object__open(name, obj_buf, obj_buf_sz, true);
>  }
>  
>  int bpf_object__unload(struct bpf_object *obj)
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index e8f70977d137..2905dffd70b2 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -70,8 +70,6 @@ struct bpf_object_open_attr {
>  LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
>  LIBBPF_API struct bpf_object *
>  bpf_object__open_xattr(struct bpf_object_open_attr *attr);
> -struct bpf_object *__bpf_object__open_xattr(struct bpf_object_open_attr *attr,
> -					    int flags);
>  LIBBPF_API struct bpf_object *bpf_object__open_buffer(void *obj_buf,
>  						      size_t obj_buf_sz,
>  						      const char *name);
> diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> index 63a8dfef893b..534621e38906 100644
> --- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
> +++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> @@ -49,4 +49,3 @@ int handle_uprobe_return(struct pt_regs *ctx)
>  }
>  
>  char _license[] SEC("license") = "GPL";
> -__u32 _version SEC("version") = 1;
> diff --git a/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c b/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c
> index f8ffa3f3d44b..736b6955bba7 100644
> --- a/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c
> +++ b/tools/testing/selftests/bpf/progs/test_get_stack_rawtp.c
> @@ -100,4 +100,3 @@ int bpf_prog1(void *ctx)
>  }
>  
>  char _license[] SEC("license") = "GPL";
> -__u32 _version SEC("version") = 1; /* ignored by tracepoints, required by libbpf.a */
> diff --git a/tools/testing/selftests/bpf/progs/test_perf_buffer.c b/tools/testing/selftests/bpf/progs/test_perf_buffer.c
> index 876c27deb65a..07c09ca5546a 100644
> --- a/tools/testing/selftests/bpf/progs/test_perf_buffer.c
> +++ b/tools/testing/selftests/bpf/progs/test_perf_buffer.c
> @@ -22,4 +22,3 @@ int handle_sys_nanosleep_entry(struct pt_regs *ctx)
>  }
>  
>  char _license[] SEC("license") = "GPL";
> -__u32 _version SEC("version") = 1;
> diff --git a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
> index fa0be3e10a10..3b7e1dca8829 100644
> --- a/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
> +++ b/tools/testing/selftests/bpf/progs/test_stacktrace_map.c
> @@ -74,4 +74,3 @@ int oncpu(struct sched_switch_args *ctx)
>  }
>  
>  char _license[] SEC("license") = "GPL";
> -__u32 _version SEC("version") = 1; /* ignored by tracepoints, required by libbpf.a */
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 bpf-next 1/4] libbpf: stop enforcing kern_version, populate it for users
  2019-10-07 16:14   ` Stanislav Fomichev
@ 2019-10-07 16:42     ` Andrii Nakryiko
  0 siblings, 0 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2019-10-07 16:42 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

On Mon, Oct 7, 2019 at 9:14 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 10/04, Andrii Nakryiko wrote:
> > Kernel version enforcement for kprobes/kretprobes was removed from
> > 5.0 kernel in 6c4fc209fcf9 ("bpf: remove useless version check for prog load").
> > Since then, BPF programs were specifying SEC("version") just to please
> > libbpf. We should stop enforcing this in libbpf, if even kernel doesn't
> > care. Furthermore, libbpf now will pre-populate current kernel version
> > of the host system, in case we are still running on old kernel.
>
> [..]
> > This patch also removes __bpf_object__open_xattr from libbpf.h, as
> > nothing in libbpf is relying on having it in that header. That function
> > was never exported as LIBBPF_API and even name suggests its internal
> > version. So this should be safe to remove, as it doesn't break ABI.
> This gives me the following (I don't know why bpftool was allowed to link
> against non-LIBBPF_API exposed function):
>
> + make -s -j72 -C tools/bpf/bpftool
>
> prog.c: In function ‘load_with_options’:
> prog.c:1227:8: warning: implicit declaration of function ‘__bpf_object__open_xattr’; did you mean ‘bpf_object__open_xattr’? [-Wimplicit-function-declaration]
>   obj = __bpf_object__open_xattr(&open_attr, bpf_flags);
>         ^~~~~~~~~~~~~~~~~~~~~~~~
>         bpf_object__open_xattr
> prog.c:1227:8: warning: nested extern declaration of ‘__bpf_object__open_xattr’ [-Wnested-externs]
> prog.c:1227:6: warning: assignment to ‘struct bpf_object *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
>   obj = __bpf_object__open_xattr(&open_attr, bpf_flags);
>       ^
>

Cool, I somehow didn't find any users of that API, but I looked only
in libbpf and selftests, forgot about bpftool. I'll fix it to use new
APIs.

Thanks for reporting!



> Warning: Kernel ABI header at 'tools/include/uapi/linux/if_link.h' differs from latest version at 'include/uapi/linux/if_link.h'
> /usr/bin/ld: prog.o: in function `load_with_options':
> prog.c:(.text+0x49b): undefined reference to `__bpf_object__open_xattr'
> collect2: error: ld returned 1 exit statu
>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---

[...]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-10-07 16:43 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-04 22:40 [PATCH v3 bpf-next 0/4] Add new-style bpf_object__open APIs Andrii Nakryiko
2019-10-04 22:40 ` [PATCH v3 bpf-next 1/4] libbpf: stop enforcing kern_version, populate it for users Andrii Nakryiko
2019-10-07 16:14   ` Stanislav Fomichev
2019-10-07 16:42     ` Andrii Nakryiko
2019-10-04 22:40 ` [PATCH v3 bpf-next 2/4] libbpf: add bpf_object__open_{file,mem} w/ extensible opts Andrii Nakryiko
2019-10-06  1:24   ` Alexei Starovoitov
2019-10-06  2:35     ` Andrii Nakryiko
2019-10-04 22:40 ` [PATCH v3 bpf-next 3/4] libbpf: fix bpf_object__name() to actually return object name Andrii Nakryiko
2019-10-04 22:40 ` [PATCH v3 bpf-next 4/4] selftests/bpf: switch tests to new bpf_object__open_{file,mem}() APIs Andrii Nakryiko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).