netdev - Re: [PATCH v3 bpf-next 2/4] libbpf: add bpf_object__open

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4Bza_BzDYjzsxYKrz8mJ2CkfseFFoDG9j2XR9b80S4QYp7A@mail.gmail.com>
Date:   Sat, 5 Oct 2019 19:35:32 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v3 bpf-next 2/4] libbpf: add bpf_object__open_{file,mem}
 w/ extensible opts

On Sat, Oct 5, 2019 at 6:24 PM Alexei Starovoitov
<alexei.starovoitov@...il.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@...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.