netdev - Re: [PATCH v2 bpf-next 10/16] bpf: Add bpf_btf_find_by_name

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4BzaPDF8h9t1xqMo-hKqp=J_bE1OtWXh+jugZxV597qjdaw@mail.gmail.com>
Date:   Tue, 27 Apr 2021 10:45:38 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 10/16] bpf: Add bpf_btf_find_by_name_kind() helper.

On Mon, Apr 26, 2021 at 8:37 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> 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@...il.com> wrote:
> > >
> > > From: Alexei Starovoitov <ast@...nel.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@...nel.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.

Yeah, I figured ARG_PTR_TO_CONST_STR isn't an option here. By "flags
would be useless" you mean that you'd use another parameter if some
flags were set? Did we ever do that to existing helpers? We can always
add a new helper, if necessary. name + name_sz seems less error-prone,
tbh.

>
> >
> > > + *             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.

"program" as in the user-space process that did bpf_prog_test_run(),
right? In the cover letter you mentioned that BPF_PROG_TYPE_SYSCALL
might be called on syscall entry in the future, for that case there is
no clear "owning" process, so would be curious to see how that problem
gets solved.

> 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.

Ok, good, that seems important.