On Fri, Nov 18, 2022 at 5:06 PM Song Liu <songliubraving@xxxxxxxx> wrote:
>
>
>
> > On Nov 18, 2022, at 3:45 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Fri, Nov 18, 2022 at 7:40 AM Jiri Olsa <jolsa@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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.
