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

[Song Liu <songliubraving@meta.com>]

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

Thanks,
Song