On 10/9/19 11:01 AM, Andrii Nakryiko wrote:
> On Fri, Oct 4, 2019 at 10:04 PM Alexei Starovoitov <ast@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>
> 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
