On Mon, Mar 18, 2024 at 6:18 AM Mykyta Yatsenko
<mykyta.yatsenko5@xxxxxxxxx> wrote:
>
We generally use "libbpf: " prefix for patch subject if changes are
related to libbpf code base. I fixed it up while applying.
> From: Mykyta Yatsenko <yatsenko@xxxxxxxx>
>
> libbpf creates bpf_program/bpf_map structs for each program/map that
> user defines, but it allows to disable creating/loading those objects in
> kernel, in that case they won't have associated file descriptor
> (fd < 0). Such functionality is used for backward compatibility
> with some older kernels.
>
> Nothing prevents users from passing these maps or programs with no
> kernel counterpart to libbpf APIs. This change introduces explicit
> checks for kernel objects existence, aiming to improve visibility of
> those edge cases and provide meaningful warnings to users.
>
> Signed-off-by: Mykyta Yatsenko <yatsenko@xxxxxxxx>
> ---
> tools/lib/bpf/libbpf.c | 56 ++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 51 insertions(+), 5 deletions(-)
>
See nits below. I fixed all that up while applying, as it was just
some text adjustment, so it didn't seem worth another version. Thanks
for the patch, it will help users!
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 604368cfbf02..d1febdb036de 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -8572,6 +8572,12 @@ int bpf_map__pin(struct bpf_map *map, const char *path)
> return libbpf_err(-EINVAL);
> }
>
> + if (map->fd < 0) {
> + pr_warn("map '%s': can't pin BPF map without FD (was it created?)\n",
> + bpf_map__name(map));
> + return libbpf_err(-EINVAL);
> + }
> +
> if (map->pin_path) {
> if (path && strcmp(path, map->pin_path)) {
> pr_warn("map '%s' already has pin path '%s' different from '%s'\n",
> @@ -10316,6 +10322,11 @@ static int validate_map_op(const struct bpf_map *map, size_t key_sz,
> return -EINVAL;
> }
>
> + if (map->fd < 0) {
> + pr_warn("map '%s': can't use BPF map without FD (was it created?)\n", map->name);
> + return -EINVAL;
> + }
> +
> if (!check_value_sz)
> return 0;
>
> @@ -10428,8 +10439,15 @@ long libbpf_get_error(const void *ptr)
> int bpf_link__update_program(struct bpf_link *link, struct bpf_program *prog)
> {
> int ret;
> + int prog_fd = bpf_program__fd(prog);
>
> - ret = bpf_link_update(bpf_link__fd(link), bpf_program__fd(prog), NULL);
> + if (prog_fd < 0) {
> + pr_warn("prog '%s': can't use BPF program without FD (was it created?)\n",
maps are created, programs are loaded, so "was it loaded?"?
> + prog->name);
> + return libbpf_err(-EINVAL);
> + }
> +
> + ret = bpf_link_update(bpf_link__fd(link), prog_fd, NULL);
> return libbpf_err_errno(ret);
> }
>
> @@ -11347,6 +11365,13 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> if (!OPTS_VALID(opts, bpf_kprobe_multi_opts))
> return libbpf_err_ptr(-EINVAL);
>
> + prog_fd = bpf_program__fd(prog);
> + if (prog_fd < 0) {
> + pr_warn("prog '%s': can't attach BPF program without FD (was it created?)\n",
ditto
> + prog->name);
> + return libbpf_err_ptr(-EINVAL);
> + }
> +
> syms = OPTS_GET(opts, syms, false);
> addrs = OPTS_GET(opts, addrs, false);
> cnt = OPTS_GET(opts, cnt, false);
> @@ -11387,7 +11412,6 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> }
> link->detach = &bpf_link__detach_fd;
>
> - prog_fd = bpf_program__fd(prog);
> link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_KPROBE_MULTI, &lopts);
> if (link_fd < 0) {
> err = -errno;
> @@ -11770,6 +11794,13 @@ bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
> if (!OPTS_VALID(opts, bpf_uprobe_multi_opts))
> return libbpf_err_ptr(-EINVAL);
>
> + prog_fd = bpf_program__fd(prog);
> + if (prog_fd < 0) {
> + pr_warn("prog '%s': can't attach BPF program without FD (was it created?)\n",
loaded
> + prog->name);
> + return libbpf_err_ptr(-EINVAL);
> + }
> +
> syms = OPTS_GET(opts, syms, NULL);
> offsets = OPTS_GET(opts, offsets, NULL);
> ref_ctr_offsets = OPTS_GET(opts, ref_ctr_offsets, NULL);
> @@ -11845,7 +11876,6 @@ bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
> }
> link->detach = &bpf_link__detach_fd;
>
> - prog_fd = bpf_program__fd(prog);
> link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_UPROBE_MULTI, &lopts);
> if (link_fd < 0) {
> err = -errno;
> @@ -12671,6 +12701,12 @@ struct bpf_link *bpf_program__attach(const struct bpf_program *prog)
> if (!prog->sec_def || !prog->sec_def->prog_attach_fn)
> return libbpf_err_ptr(-EOPNOTSUPP);
>
> + if (bpf_program__fd(prog) < 0) {
> + pr_warn("prog '%s': can't attach BPF program w/o FD (did you load it?)\n",
I normalized w/o to without
> + prog->name);
> + return libbpf_err_ptr(-EINVAL);
> + }
> +
> err = prog->sec_def->prog_attach_fn(prog, prog->sec_def->cookie, &link);
> if (err)
> return libbpf_err_ptr(err);
> @@ -12711,9 +12747,14 @@ struct bpf_link *bpf_map__attach_struct_ops(const struct bpf_map *map)
> __u32 zero = 0;
> int err, fd;
>
> - if (!bpf_map__is_struct_ops(map) || map->fd == -1)
> + if (!bpf_map__is_struct_ops(map))
> return libbpf_err_ptr(-EINVAL);
>
> + if (map->fd < 0) {
> + pr_warn("map '%s': can't attach BPF map w/o FD (did you load it?)\n", map->name);
without, "was it created?"
> + return libbpf_err_ptr(-EINVAL);
> + }
> +
> link = calloc(1, sizeof(*link));
> if (!link)
> return libbpf_err_ptr(-EINVAL);
> @@ -12760,8 +12801,13 @@ int bpf_link__update_map(struct bpf_link *link, const struct bpf_map *map)
> __u32 zero = 0;
> int err;
>
> - if (!bpf_map__is_struct_ops(map) || !map_is_created(map))
> + if (!bpf_map__is_struct_ops(map))
> + return -EINVAL;
> +
> + if (map->fd < 0) {
> + pr_warn("map '%s': can't use BPF map w/o FD (did you load it?)\n", map->name);
same about create vs load
> return -EINVAL;
> + }
>
> st_ops_link = container_of(link, struct bpf_link_struct_ops, link);
> /* Ensure the type of a link is correct */
> --
> 2.42.0
>
