From: Stanislav Fomichev <sdf@fomichev.me>
To: Andrii Nakryiko <andriin@fb.com>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org, ast@fb.com,
daniel@iogearbox.net, andrii.nakryiko@gmail.com,
kernel-team@fb.com
Subject: Re: [PATCH v3 bpf-next 1/4] libbpf: stop enforcing kern_version, populate it for users
Date: Mon, 7 Oct 2019 09:14:37 -0700 [thread overview]
Message-ID: <20191007161437.GB2096@mini-arch> (raw)
In-Reply-To: <20191004224037.1625049-2-andriin@fb.com>
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
>
next prev parent reply other threads:[~2019-10-07 16:14 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191007161437.GB2096@mini-arch \
--to=sdf@fomichev.me \
--cc=andrii.nakryiko@gmail.com \
--cc=andriin@fb.com \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).