Re: [PATCHv3 bpf-next 1/2] bpf: Add bpf_vma_build_id_parse function and kfunc

[Jiri Olsa <olsajiri@gmail.com>]

On Fri, Nov 18, 2022 at 06:25:15PM -0800, Alexei Starovoitov wrote:
> On Fri, Nov 18, 2022 at 5:06 PM Song Liu <songliubraving@meta.com> wrote:
> >
> >
> >
> > > On Nov 18, 2022, at 3:45 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Fri, Nov 18, 2022 at 7:40 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >>
> > >> Adding bpf_vma_build_id_parse function to retrieve build id from
> > >> passed vma object and making it available as bpf kfunc.
> > >>
> > >> We can't use build_id_parse directly as kfunc, because we would
> > >> not have control over the build id buffer size provided by user.
> > >>
> > >> Instead we are adding new bpf_vma_build_id_parse function with
> > >> 'build_id__sz' argument that instructs verifier to check for the
> > >> available space in build_id buffer.
> > >>
> > >> This way  we check that there's  always available memory space
> > >> behind build_id pointer. We also check that the build_id__sz is
> > >> at least BUILD_ID_SIZE_MAX so we can place any buildid in.
> > >>
> > >> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > >> ---
> > >> include/linux/bpf.h      |  4 ++++
> > >> kernel/bpf/verifier.c    | 26 ++++++++++++++++++++++++++
> > >> kernel/trace/bpf_trace.c | 31 +++++++++++++++++++++++++++++++
> > >> 3 files changed, 61 insertions(+)
> > >>
> > >> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > >> index 8b32376ce746..7648188faa2c 100644
> > >> --- a/include/linux/bpf.h
> > >> +++ b/include/linux/bpf.h
> > >> @@ -2805,4 +2805,8 @@ static inline bool type_is_alloc(u32 type)
> > >>        return type & MEM_ALLOC;
> > >> }
> > >>
> > >> +int bpf_vma_build_id_parse(struct vm_area_struct *vma,
> > >> +                          unsigned char *build_id,
> > >> +                          size_t build_id__sz);
> > >> +
> > >> #endif /* _LINUX_BPF_H */
> > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > >> index 195d24316750..e20bad754a3a 100644
> > >> --- a/kernel/bpf/verifier.c
> > >> +++ b/kernel/bpf/verifier.c
> > >> @@ -8746,6 +8746,29 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> > >>        return 0;
> > >> }
> > >>
> > >> +BTF_ID_LIST_SINGLE(bpf_vma_build_id_parse_id, func, bpf_vma_build_id_parse)
> > >> +
> > >> +static int check_kfunc_caller(struct bpf_verifier_env *env, u32 func_id)
> > >> +{
> > >> +       struct bpf_func_state *cur;
> > >> +       struct bpf_insn *insn;
> > >> +
> > >> +       /* Allow bpf_vma_build_id_parse only from bpf_find_vma callback */
> > >> +       if (func_id == bpf_vma_build_id_parse_id[0]) {
> > >> +               cur = env->cur_state->frame[env->cur_state->curframe];
> > >> +               if (cur->callsite != BPF_MAIN_FUNC) {
> > >> +                       insn = &env->prog->insnsi[cur->callsite];
> > >> +                       if (insn->imm == BPF_FUNC_find_vma)
> > >> +                               return 0;
> > >> +               }
> > >> +               verbose(env, "calling bpf_vma_build_id_parse outside bpf_find_vma "
> > >> +                       "callback is not allowed\n");
> > >> +               return -1;
> > >> +       }
> > >> +
> > >> +       return 0;
> > >> +}
> > >
> > > I understand that calling bpf_vma_build_id_parse from find_vma
> > > is your only use case, but put yourself in the maintainer's shoes.
> > > We just did an arbitrary restriction and helped a single user.
> > > How are we going to explain this to other users?
> > > Let's figure out a more generic way where this call is safe.
> > > Have you looked at PTR_TRUSTED approach that David is doing
> > > for task_struct ? Can something like this be used here?
> >
> > I guess that won't work, as the vma is not refcounted. :( This is
> > why we have to hold mmap_lock when calling task_vma programs.
> >
> > OTOH, I would image bpf_vma_build_id_parse being quite useful for
> > task_vma programs.
> 
> Of course we cannot increment non-existing refcnt in vma :)
> I meant that PTR_TRUSTED part of the concept. The kfunc
> bpf_vma_build_id_parse(struct vm_area_struct *vma, ...)
> should have KF_TRUSTED_ARGS flag
> and it will be the job of the verifier to pass a trusted vma pointer.
> Meaning that the verifier needs to guarantee that
> the pointer is safe to operate on.
> That's what I was explaining to Kumar and David earlier
> about KF_TRUSTED_ARGS semantics.
> 
> PTR_TRUSTED doesn't mean that the pointer is refcnted.
> It means that it won't disappear and we can safely pass it
> to kfunc or helpers.
> For bpf_find_vma we can mark vma pointer PTR_TRUSTED on entry
> into callback bpf prog and the prog will be able to pass it
> to bpf_vma_build_id_parse() kfunc as long as the prog doesn't
> add any offset to it.
> The implementation of bpf_find_vma() guarantees that vma ptr
> passed into callback_fn is valid.
> So it's exactly PTR_TRUSTED.
> 
> Similarly task_vma programs will be receiving PTR_TRUSTED pointers too
> and will be able to call bpf_vma_build_id_parse() kfunc as well.
> Any place where we can guarantee the safety of the pointer
> we should be marking it as PTR_TRUSTED.
> 
> David's series start with marking all tp_btf arguments as PTR_TRUSTED.
> Doing this for iterators, bpf_find_vma callback
> will be a continuation of PTR_TRUSTED logic.

ok, sounds much better.. generic solution for both bpf_find_vma
and task_vma iterator

thanks,
jirka