Re: [PATCH bpf-next v3 2/2] bpf: Warn on non-preallocated case for missed trace types

[Yafang Shao <laoar.shao@gmail.com>]

On Tue, Jul 12, 2022 at 3:04 AM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 7/10/22 11:48 PM, Yafang Shao wrote:
> > On Mon, Jul 11, 2022 at 1:51 AM Yonghong Song <yhs@fb.com> wrote:
> >>
> >>
> >>
> >> On 7/9/22 8:44 AM, Yafang Shao wrote:
> >>> The raw tracepoint may cause unexpected memory allocation if we set
> >>> BPF_F_NO_PREALLOC. So let's warn on it.
> >>
> >> Please extend raw_tracepoint to other attach types which
> >> may cause runtime map allocations.
> >>
> >
> > Per my understanding, it is safe to allocate memory in a non-process
> > context as long as we don't allow blocking it.
> > So this issue doesn't matter with whether it causes runtime map
> > allocations or not, while it really matters with the tracepoint or
> > kprobe.
> > Regarding the tracepoint or kprobe, if we don't use non-preallocated
> > maps, it may allocate other extra memory besides the map element
> > itself.
> > I have verified that it is safe to use non-preallocated maps in
> > BPF_TRACE_ITER or BPF_TRACE_FENTRY.
> > So filtering out BPF_TRACE_RAW_TP only is enough. >
> >>>
> >>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> >>> ---
> >>>    kernel/bpf/verifier.c | 18 +++++++++++++-----
> >>>    1 file changed, 13 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >>> index e3cf6194c24f..3cd8260827e0 100644
> >>> --- a/kernel/bpf/verifier.c
> >>> +++ b/kernel/bpf/verifier.c
> >>> @@ -12574,14 +12574,20 @@ static int check_map_prealloc(struct bpf_map *map)
> >>>                !(map->map_flags & BPF_F_NO_PREALLOC);
> >>>    }
> >>>
> >>> -static bool is_tracing_prog_type(enum bpf_prog_type type)
> >>> +static bool is_tracing_prog_type(enum bpf_prog_type prog_type,
> >>> +                              enum bpf_attach_type attach_type)
> >>>    {
> >>> -     switch (type) {
> >>> +     switch (prog_type) {
> >>>        case BPF_PROG_TYPE_KPROBE:
> >>>        case BPF_PROG_TYPE_TRACEPOINT:
> >>>        case BPF_PROG_TYPE_PERF_EVENT:
> >>>        case BPF_PROG_TYPE_RAW_TRACEPOINT:
> >>> +     case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
> >>>                return true;
> >>> +     case BPF_PROG_TYPE_TRACING:
> >>> +             if (attach_type == BPF_TRACE_RAW_TP)
> >>> +                     return true;
> >>
> >> As Alexei mentioned earlier, here we should have
> >>                  if (attach_type != BPF_TRACE_ITER)
> >>                          return true;
> >
> > That will break selftests/bpf/progs/timer.c, because it uses timer in fentry.
>
> Okay. Let us remove BPF_PROG_TYPE_TRACING from this patch for now.
> fentry/fexit/fmod may attach any kallsyms functions and many of them
> are called in process context and non-preallocated hashmap totally fine.
> It is not good to prohibit non-preallocated hashmap for any
> fentry/fexit/fmod bpf programs.
>

Got it. I will do it.

-- 
Regards
Yafang