| 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
| ||
|
Message-ID: <CAEf4BzYKpstQk8JO_iOws93VpHEEs+J+z+ZO7cKRiKRNvN1zMg@mail.gmail.com>
Date: Thu, 19 Dec 2019 15:50:57 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: Daniel Borkmann <daniel@...earbox.net>,
Alexei Starovoitov <ast@...nel.org>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
Jesper Dangaard Brouer <brouer@...hat.com>,
David Miller <davem@...emloft.net>,
Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH RFC bpf-next 1/3] libbpf: Add new bpf_object__load2()
using new-style opts
On Thu, Dec 19, 2019 at 6:29 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@...hat.com>
>
> Since we introduced DECLARE_LIBBPF_OPTS and related macros for declaring
> function options, that is now the preferred way to extend APIs. Introduce a
> variant of the bpf_object__load() function that uses this function, and
> deprecate the _xattr variant. Since all the good function names were taken,
> the new function is unimaginatively called bpf_object__load2().
>
> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
> ---
I've been thinking about options for load quite a bit lately, and I'm
leaning towards an opinion, that bpf_object__load() shouldn't take any
options, and all the various per-bpf_object options have to be
specified in bpf_object_open_opts and stored, if necessary for
load/attach phase. So I'd rather move target_btf_path and log_level to
open_opts instead.
For cases where we need to tune load behavior/options per individual
BPF program, bpf_object_load_opts don't work either way, and we'll
have to add some getters/setters for bpf_program objects, anyways. So
I think it's overall cleaner to specify everything per-bpf_object at
"creation time" (which is open), and keep load()/attach() option-less.
> tools/lib/bpf/libbpf.c | 31 ++++++++++++++++++-------------
> tools/lib/bpf/libbpf.h | 13 +++++++++++++
> tools/lib/bpf/libbpf.map | 1 +
> 3 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index febbaac3daf4..266b725e444b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4844,15 +4844,12 @@ static int bpf_object__resolve_externs(struct bpf_object *obj,
> return 0;
> }
>
> -int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> +int bpf_object__load2(struct bpf_object *obj,
> + const struct bpf_object_load_opts *opts)
> {
> - struct bpf_object *obj;
> int err, i;
>
> - if (!attr)
> - return -EINVAL;
> - obj = attr->obj;
> - if (!obj)
> + if (!obj || !OPTS_VALID(opts, bpf_object_load_opts))
> return -EINVAL;
>
> if (obj->loaded) {
> @@ -4867,8 +4864,10 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> err = err ? : bpf_object__sanitize_and_load_btf(obj);
> err = err ? : bpf_object__sanitize_maps(obj);
> err = err ? : bpf_object__create_maps(obj);
> - err = err ? : bpf_object__relocate(obj, attr->target_btf_path);
> - err = err ? : bpf_object__load_progs(obj, attr->log_level);
> + err = err ? : bpf_object__relocate(obj,
> + OPTS_GET(opts, target_btf_path, NULL));
> + err = err ? : bpf_object__load_progs(obj,
> + OPTS_GET(opts, log_level, 0));
> if (err)
> goto out;
>
> @@ -4884,13 +4883,19 @@ int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> return err;
> }
>
> -int bpf_object__load(struct bpf_object *obj)
> +int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
> {
> - struct bpf_object_load_attr attr = {
> - .obj = obj,
> - };
> + DECLARE_LIBBPF_OPTS(bpf_object_load_opts, opts,
> + .log_level = attr->log_level,
> + .target_btf_path = attr->target_btf_path,
> + );
> +
> + return bpf_object__load2(attr->obj, &opts);
> +}
>
> - return bpf_object__load_xattr(&attr);
> +int bpf_object__load(struct bpf_object *obj)
> +{
> + return bpf_object__load2(obj, NULL);
> }
>
> static int make_parent_dir(const char *path)
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index fe592ef48f1b..ce86277d7445 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -132,8 +132,21 @@ struct bpf_object_load_attr {
> const char *target_btf_path;
> };
>
> +struct bpf_object_load_opts {
> + /* size of this struct, for forward/backward compatiblity */
> + size_t sz;
> + /* log level on load */
> + int log_level;
> + /* BTF path (for CO-RE relocations) */
> + const char *target_btf_path;
> +};
> +#define bpf_object_load_opts__last_field target_btf_path
> +
> /* Load/unload object into/from kernel */
> LIBBPF_API int bpf_object__load(struct bpf_object *obj);
> +LIBBPF_API int bpf_object__load2(struct bpf_object *obj,
> + const struct bpf_object_load_opts *opts);
> +/* deprecated, use bpf_object__load2() instead */
> LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr);
> LIBBPF_API int bpf_object__unload(struct bpf_object *obj);
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index e3a471f38a71..d6cb860763d1 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -217,6 +217,7 @@ LIBBPF_0.0.7 {
> bpf_object__attach_skeleton;
> bpf_object__destroy_skeleton;
> bpf_object__detach_skeleton;
> + bpf_object__load2;
> bpf_object__load_skeleton;
> bpf_object__open_skeleton;
> bpf_program__attach;
>
Powered by blists - more mailing lists