Re: [PATCH bpf-next] bpf: check bpf_map/bpf_program fd validity

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, Mar 18, 2024 at 6:18 AM Mykyta Yatsenko
<mykyta.yatsenko5@gmail.com> 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@meta.com>
>
> 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@meta.com>
> ---
>  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
>