| Message ID | 20190815000330.12044-1-a.s.protopopov@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] tools: libbpf: update extended attributes version of bpf_object__open() | expand |
> On Aug 14, 2019, at 5:03 PM, Anton Protopopov <a.s.protopopov@gmail.com> wrote: > [...] > > > int bpf_object__unload(struct bpf_object *obj) > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index e8f70977d137..634f278578dd 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -63,8 +63,13 @@ LIBBPF_API libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn); > struct bpf_object; > > struct bpf_object_open_attr { > - const char *file; > + union { > + const char *file; > + const char *obj_name; > + }; > enum bpf_prog_type prog_type; > + void *obj_buf; > + size_t obj_buf_sz; > }; I think this would break dynamically linked libbpf. No? Thanks, Song
чт, 29 авг. 2019 г. в 16:02, Song Liu <songliubraving@fb.com>: > > > > > On Aug 14, 2019, at 5:03 PM, Anton Protopopov <a.s.protopopov@gmail.com> wrote: > > > > [...] > > > > > > > int bpf_object__unload(struct bpf_object *obj) > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > > index e8f70977d137..634f278578dd 100644 > > --- a/tools/lib/bpf/libbpf.h > > +++ b/tools/lib/bpf/libbpf.h > > @@ -63,8 +63,13 @@ LIBBPF_API libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn); > > struct bpf_object; > > > > struct bpf_object_open_attr { > > - const char *file; > > + union { > > + const char *file; > > + const char *obj_name; > > + }; > > enum bpf_prog_type prog_type; > > + void *obj_buf; > > + size_t obj_buf_sz; > > }; > > I think this would break dynamically linked libbpf. No? Ah, yes, sure. What is the right way to make changes which break ABI in libbpf? BTW, does the commit ddc7c3042614 ("libbpf: implement BPF CO-RE offset relocation algorithm") which adds a new field to the struct bpf_object_load_attr also break ABI? > > Thanks, > Song >
> On Aug 30, 2019, at 11:53 AM, Anton Protopopov <a.s.protopopov@gmail.com> wrote: > > чт, 29 авг. 2019 г. в 16:02, Song Liu <songliubraving@fb.com>: >> >> >> >>> On Aug 14, 2019, at 5:03 PM, Anton Protopopov <a.s.protopopov@gmail.com> wrote: >>> >> >> [...] >> >>> >>> >>> int bpf_object__unload(struct bpf_object *obj) >>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h >>> index e8f70977d137..634f278578dd 100644 >>> --- a/tools/lib/bpf/libbpf.h >>> +++ b/tools/lib/bpf/libbpf.h >>> @@ -63,8 +63,13 @@ LIBBPF_API libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn); >>> struct bpf_object; >>> >>> struct bpf_object_open_attr { >>> - const char *file; >>> + union { >>> + const char *file; >>> + const char *obj_name; >>> + }; >>> enum bpf_prog_type prog_type; >>> + void *obj_buf; >>> + size_t obj_buf_sz; >>> }; >> >> I think this would break dynamically linked libbpf. No? > > Ah, yes, sure. What is the right way to make changes which break ABI in libbpf? I don't have a good idea here on the top of my head. Maybe we need a new struct and/or function for this. > > BTW, does the commit ddc7c3042614 ("libbpf: implement BPF CO-RE offset > relocation algorithm") which adds a new field to the struct > bpf_object_load_attr also break ABI? I think this change was in the same release, so it is OK. Thanks, Song
On 30-Aug 19:24, Song Liu wrote: > > > > On Aug 30, 2019, at 11:53 AM, Anton Protopopov <a.s.protopopov@gmail.com> wrote: > > > > чт, 29 авг. 2019 г. в 16:02, Song Liu <songliubraving@fb.com>: > >> > >> > >> > >>> On Aug 14, 2019, at 5:03 PM, Anton Protopopov <a.s.protopopov@gmail.com> wrote: > >>> > >> > >> [...] > >> > >>> > >>> > >>> int bpf_object__unload(struct bpf_object *obj) > >>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > >>> index e8f70977d137..634f278578dd 100644 > >>> --- a/tools/lib/bpf/libbpf.h > >>> +++ b/tools/lib/bpf/libbpf.h > >>> @@ -63,8 +63,13 @@ LIBBPF_API libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn); > >>> struct bpf_object; > >>> > >>> struct bpf_object_open_attr { > >>> - const char *file; > >>> + union { > >>> + const char *file; > >>> + const char *obj_name; > >>> + }; > >>> enum bpf_prog_type prog_type; > >>> + void *obj_buf; > >>> + size_t obj_buf_sz; > >>> }; > >> > >> I think this would break dynamically linked libbpf. No? > > > > Ah, yes, sure. What is the right way to make changes which break ABI in libbpf? > > I don't have a good idea here on the top of my head. > > Maybe we need a new struct and/or function for this. I incorporated the suggested fixes and sent a new patch for this as we ran into pretty much the same issue. (i.e. not being able to set needs_kver / flags). https://lore.kernel.org/bpf/20190927130834.18829-1-kpsingh@chromium.org/T/#u - KP > > > > > BTW, does the commit ddc7c3042614 ("libbpf: implement BPF CO-RE offset > > relocation algorithm") which adds a new field to the struct > > bpf_object_load_attr also break ABI? > > I think this change was in the same release, so it is OK. > > Thanks, > Song
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 2233f919dd88..7c8054afd901 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -3630,13 +3630,31 @@ __bpf_object__open(const char *path, void *obj_buf, size_t obj_buf_sz, struct bpf_object *__bpf_object__open_xattr(struct bpf_object_open_attr *attr, int flags) { + char tmp_name[64]; + /* param validation */ - if (!attr->file) + if (!attr) return NULL; - pr_debug("loading %s\n", attr->file); + if (attr->obj_buf) { + if (attr->obj_buf_sz <= 0) + return NULL; + if (!attr->file) { + snprintf(tmp_name, sizeof(tmp_name), "%lx-%lx", + (unsigned long)attr->obj_buf, + (unsigned long)attr->obj_buf_sz); + attr->obj_name = tmp_name; + } + pr_debug("loading object '%s' from buffer\n", attr->obj_name); + } else if (!attr->file) { + return NULL; + } else { + attr->obj_buf_sz = 0; - return __bpf_object__open(attr->file, NULL, 0, + pr_debug("loading object file '%s'\n", attr->file); + } + + return __bpf_object__open(attr->file, attr->obj_buf, attr->obj_buf_sz, bpf_prog_type__needs_kver(attr->prog_type), flags); } @@ -3660,21 +3678,14 @@ struct bpf_object *bpf_object__open_buffer(void *obj_buf, size_t obj_buf_sz, const char *name) { - char tmp_name[64]; - - /* param validation */ - if (!obj_buf || obj_buf_sz <= 0) - return NULL; - - if (!name) { - snprintf(tmp_name, sizeof(tmp_name), "%lx-%lx", - (unsigned long)obj_buf, - (unsigned long)obj_buf_sz); - name = tmp_name; - } - pr_debug("loading object '%s' from buffer\n", name); + struct bpf_object_open_attr attr = { + .obj_name = name, + .obj_buf = obj_buf, + .obj_buf_sz = obj_buf_sz, + .prog_type = BPF_PROG_TYPE_UNSPEC, + }; - return __bpf_object__open(name, obj_buf, obj_buf_sz, true, true); + return bpf_object__open_xattr(&attr); } int bpf_object__unload(struct bpf_object *obj) diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index e8f70977d137..634f278578dd 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -63,8 +63,13 @@ LIBBPF_API libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn); struct bpf_object; struct bpf_object_open_attr { - const char *file; + union { + const char *file; + const char *obj_name; + }; enum bpf_prog_type prog_type; + void *obj_buf; + size_t obj_buf_sz; }; LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
Update the bpf_object_open_attr structure and corresponding code so that the bpf_object__open_xattr function could be used to open objects from buffers as well as from files. The reason for this change is that the existing bpf_object__open_buffer function doesn't provide a way to specify neither the needs_kver nor flags parameters to the internal call to __bpf_object__open which makes it inconvenient for loading BPF objects which doesn't require a kernel version. Two new fields, obj_buf and obj_buf_sz, were added to the structure, and the file field was union'ed with obj_name so that one can open an object like this: struct bpf_object_open_attr attr = { .obj_name = name, .obj_buf = obj_buf, .obj_buf_sz = obj_buf_sz, .prog_type = BPF_PROG_TYPE_UNSPEC, }; return bpf_object__open_xattr(&attr); while still being able to use the file semantics: struct bpf_object_open_attr attr = { .file = path, .prog_type = BPF_PROG_TYPE_UNSPEC, }; return bpf_object__open_xattr(&attr); Another thing to note is that since the commit c034a177d3c8 ("bpf: bpftool, add flag to allow non-compat map definitions") which introduced a MAPS_RELAX_COMPAT flag to load objects with non-compat map definitions, bpf_object__open_buffer was called with this flag enabled (it was passed as the boolean true value in flags argument to __bpf_object__open). The default behaviour for all open functions is to clear this flag and this patch changes bpf_object__open_buffer to clears this flag. It can be enabled, if needed, by opening an object from buffer using __bpf_object__open_xattr. Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> --- tools/lib/bpf/libbpf.c | 45 ++++++++++++++++++++++++++---------------- tools/lib/bpf/libbpf.h | 7 ++++++- 2 files changed, 34 insertions(+), 18 deletions(-)