Re: [PATCH v2 bpf-next 10/16] bpf: Add bpf_btf_find_by_name_kind() helper.

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Mon, Apr 26, 2021 at 03:46:29PM -0700, Andrii Nakryiko wrote:
> On Thu, Apr 22, 2021 at 5:27 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > Add new helper:
> >
> > long bpf_btf_find_by_name_kind(u32 btf_fd, char *name, u32 kind, int flags)
> >         Description
> >                 Find given name with given type in BTF pointed to by btf_fd.
> >                 If btf_fd is zero look for the name in vmlinux BTF and in module's BTFs.
> >         Return
> >                 Returns btf_id and btf_obj_fd in lower and upper 32 bits.
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ---
> >  include/linux/bpf.h            |  1 +
> >  include/uapi/linux/bpf.h       |  8 ++++
> >  kernel/bpf/btf.c               | 68 ++++++++++++++++++++++++++++++++++
> >  kernel/bpf/syscall.c           |  2 +
> >  tools/include/uapi/linux/bpf.h |  8 ++++
> >  5 files changed, 87 insertions(+)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 0f841bd0cb85..4cf361eb6a80 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1972,6 +1972,7 @@ extern const struct bpf_func_proto bpf_get_socket_ptr_cookie_proto;
> >  extern const struct bpf_func_proto bpf_task_storage_get_proto;
> >  extern const struct bpf_func_proto bpf_task_storage_delete_proto;
> >  extern const struct bpf_func_proto bpf_for_each_map_elem_proto;
> > +extern const struct bpf_func_proto bpf_btf_find_by_name_kind_proto;
> >
> >  const struct bpf_func_proto *bpf_tracing_func_proto(
> >         enum bpf_func_id func_id, const struct bpf_prog *prog);
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index de58a714ed36..253f5f031f08 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -4748,6 +4748,13 @@ union bpf_attr {
> >   *             Execute bpf syscall with given arguments.
> >   *     Return
> >   *             A syscall result.
> > + *
> > + * long bpf_btf_find_by_name_kind(u32 btf_fd, char *name, u32 kind, int flags)
> > + *     Description
> > + *             Find given name with given type in BTF pointed to by btf_fd.
> 
> "Find BTF type with given name"? Should the limits on name length be

+1

> specified? KSYM_NAME_LEN is a pretty arbitrary restriction.

that's implementation detail that shouldn't leak into uapi.

> Also,
> would it still work fine if the caller provides a pointer to a much
> shorter piece of memory?
> 
> Why not add name_sz right after name, as we do with a lot of other
> arguments like this?

That's an option too, but then the helper will have 5 args and 'flags'
would be likely useless. I mean unlikely it will help extending it.
I was thinking about ARG_PTR_TO_CONST_STR, but it doesn't work,
since blob is writeable by the prog. It's read only from user space.
I'm fine with name, name_sz though.

> 
> > + *             If btf_fd is zero look for the name in vmlinux BTF and in module's BTFs.
> > + *     Return
> > + *             Returns btf_id and btf_obj_fd in lower and upper 32 bits.
> 
> Mention that for vmlinux BTF btf_obj_fd will be zero? Also who "owns"
> the FD? If the BPF program doesn't close it, when are they going to be
> cleaned up?

just like bpf_sys_bpf. Who owns returned FD? The program that called
the helper, of course.
In the current shape of loader prog these btf fds are cleaned up correctly
in success and in error case.
Not all FDs though. map fds will stay around if bpf_sys_bpf(prog_load) fails to load.
Tweaking loader prog to close all FDs in error case is on todo list.