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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On Nov 18, 2022, at 6:25 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> 
> 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.

I see. So PTR_TRUSTED task_struct is an refcounted task_struct; 
while PTR_TRUSTED vm_area_struct is a vma with its mm_struct locked. 
That makes perfect sense. 

Thanks for the explanation!

Song





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux