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

[Yonghong Song <yhs@fb.com>]

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.

> 
>> For attach types with BPF_PROG_TYPE_TRACING programs,
>> BPF_TRACE_ITER attach type can only appear in process context.
>> All other attach types may appear in non-process context.
>>
> 
> Thanks for the explanation.
> 
>>> +             return false;
>>>        default:
>>>                return false;
>>>        }
>>> @@ -12601,7 +12607,9 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
>>>                                        struct bpf_prog *prog)
>>>
>>>    {
>>> +     enum bpf_attach_type attach_type = prog->expected_attach_type;
>>>        enum bpf_prog_type prog_type = resolve_prog_type(prog);
>>> +
>> [...]
> 
> 
>