Re: [PATCH bpf-next 08/10] bpf: check types of arguments passed into helpers

[Alexei Starovoitov <ast@fb.com>]

On 10/9/19 11:01 AM, Andrii Nakryiko wrote:
> On Fri, Oct 4, 2019 at 10:04 PM Alexei Starovoitov <ast@kernel.org> wrote:
>>
>> Introduce new helper that reuses existing skb perf_event output
>> implementation, but can be called from raw_tracepoint programs
>> that receive 'struct sk_buff *' as tracepoint argument or
>> can walk other kernel data structures to skb pointer.
>>
>> In order to do that teach verifier to resolve true C types
>> of bpf helpers into in-kernel BTF ids.
>> The type of kernel pointer passed by raw tracepoint into bpf
>> program will be tracked by the verifier all the way until
>> it's passed into helper function.
>> For example:
>> kfree_skb() kernel function calls trace_kfree_skb(skb, loc);
>> bpf programs receives that skb pointer and may eventually
>> pass it into bpf_skb_output() bpf helper which in-kernel is
>> implemented via bpf_skb_event_output() kernel function.
>> Its first argument in the kernel is 'struct sk_buff *'.
>> The verifier makes sure that types match all the way.
>>
>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>> ---
> 
> no real concerns, few questions and nits below. Looks great otherwise!
> 
>>   include/linux/bpf.h                       |  3 +
>>   include/uapi/linux/bpf.h                  |  3 +-
>>   kernel/bpf/btf.c                          | 73 +++++++++++++++++++++++
>>   kernel/bpf/verifier.c                     | 29 +++++++++
>>   kernel/trace/bpf_trace.c                  |  4 ++
>>   net/core/filter.c                         | 15 ++++-
>>   tools/include/uapi/linux/bpf.h            |  3 +-
>>   tools/testing/selftests/bpf/bpf_helpers.h |  4 ++
>>   8 files changed, 131 insertions(+), 3 deletions(-)
>>
> 
> [...]
> 
>> +       args = (const struct btf_param *)(t + 1);
>> +       if (arg >= btf_type_vlen(t)) {
>> +               bpf_verifier_log_write(env,
>> +                                      "bpf helper '%s' doesn't have %d-th argument\n",
>> +                                      fnname, arg);
>> +               return -EINVAL;
>> +       }
>> +
>> +       t = btf_type_by_id(btf_vmlinux, args[arg].type);
>> +       if (!btf_type_is_ptr(t) || !t->type) {
>> +               /* anything but the pointer to struct is a helper config bug */
>> +               bpf_verifier_log_write(env,
>> +                                      "ARG_PTR_TO_BTF is misconfigured\n");
>> +
>> +               return -EFAULT;
>> +       }
>> +       btf_id = t->type;
>> +
>> +       t = btf_type_by_id(btf_vmlinux, t->type);
>> +       if (!btf_type_is_struct(t)) {
> 
> resolve mods/typedefs?

fixed

>> +                       verbose(env, "Helper has type %s got %s in R%d\n",
>> +                               btf_name_by_offset(btf_vmlinux,
>> +                                                  btf_type_by_id(btf_vmlinux,
>> +                                                                 meta->btf_id)->name_off),
>> +                               btf_name_by_offset(btf_vmlinux,
>> +                                                  btf_type_by_id(btf_vmlinux,
>> +                                                                 reg->btf_id)->name_off),
> 
> This is rather verbose, but popular, construct, maybe extract into a
> helper func and cut on code boilerplate? I think you had similar usage
> in few places in previous patches.

makes sense.

>> +       if (fn->arg1_type == ARG_PTR_TO_BTF_ID) {
>> +               if (!fn->btf_id[0])
>> +                       fn->btf_id[0] = btf_resolve_helper_id(env, fn->func, 0);
>> +               meta.btf_id = fn->btf_id[0];
>> +       }
> 
> Is this this baby-stepping thing that we do it only for arg1? Any
> complications from doing a loop over all 5 params?

fixed