2019-08-15 00:06:30

by Anton Protopopov

[permalink] [raw]
Subject: [PATCH bpf-next] tools: libbpf: update extended attributes version of bpf_object__open()

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 <[email protected]>
---
tools/lib/bpf/libbpf.c | 45 ++++++++++++++++++++++++++----------------
tools/lib/bpf/libbpf.h | 7 ++++++-
2 files changed, 34 insertions(+), 18 deletions(-)

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);
--
2.19.1


2019-08-29 20:04:03

by Song Liu

[permalink] [raw]
Subject: Re: [PATCH bpf-next] tools: libbpf: update extended attributes version of bpf_object__open()



> On Aug 14, 2019, at 5:03 PM, Anton Protopopov <[email protected]> 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

2019-08-30 18:54:29

by Anton Protopopov

[permalink] [raw]
Subject: Re: [PATCH bpf-next] tools: libbpf: update extended attributes version of bpf_object__open()

чт, 29 авг. 2019 г. в 16:02, Song Liu <[email protected]>:
>
>
>
> > On Aug 14, 2019, at 5:03 PM, Anton Protopopov <[email protected]> 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
>

2019-08-30 19:26:21

by Song Liu

[permalink] [raw]
Subject: Re: [PATCH bpf-next] tools: libbpf: update extended attributes version of bpf_object__open()



> On Aug 30, 2019, at 11:53 AM, Anton Protopopov <[email protected]> wrote:
>
> чт, 29 авг. 2019 г. в 16:02, Song Liu <[email protected]>:
>>
>>
>>
>>> On Aug 14, 2019, at 5:03 PM, Anton Protopopov <[email protected]> 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

2019-09-27 13:13:08

by KP Singh

[permalink] [raw]
Subject: Re: [PATCH bpf-next] tools: libbpf: update extended attributes version of bpf_object__open()

On 30-Aug 19:24, Song Liu wrote:
>
>
> > On Aug 30, 2019, at 11:53 AM, Anton Protopopov <[email protected]> wrote:
> >
> > чт, 29 авг. 2019 г. в 16:02, Song Liu <[email protected]>:
> >>
> >>
> >>
> >>> On Aug 14, 2019, at 5:03 PM, Anton Protopopov <[email protected]> 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/[email protected]/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