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?
> + bpf_verifier_log_write(env,
> + "ARG_PTR_TO_BTF is not a struct\n");
> +
> + return -EFAULT;
> + }
> + bpf_verifier_log_write(env,
> + "helper '%s' arg%d has btf_id %d struct '%s'\n",
> + fnname + 4, arg, btf_id,
> + __btf_name_by_offset(btf_vmlinux, t->name_off));
> + return btf_id;
> +}
> +
> void btf_type_seq_show(const struct btf *btf, u32 type_id, void *obj,
> struct seq_file *m)
> {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 957ee442f2b4..0717aacb7801 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -205,6 +205,7 @@ struct bpf_call_arg_meta {
> u64 msize_umax_value;
> int ref_obj_id;
> int func_id;
> + u32 btf_id;
> };
>
> struct btf *btf_vmlinux;
> @@ -3367,6 +3368,27 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
> expected_type = PTR_TO_SOCKET;
> if (type != expected_type)
> goto err_type;
> + } else if (arg_type == ARG_PTR_TO_BTF_ID) {
> + expected_type = PTR_TO_BTF_ID;
> + if (type != expected_type)
> + goto err_type;
> + if (reg->btf_id != meta->btf_id) {
just double-checking, both reg->btf_id and meta->btf_id will be
resolved through modifiers/typedefs all the way to the struct, right?
> + 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.
> + regno);
> +
> + return -EACCES;
> + }
[...]
> @@ -4053,6 +4077,11 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
> return err;
> }
>
> + 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?
> meta.func_id = func_id;
> /* check args */
> err = check_func_arg(env, BPF_REG_1, fn->arg1_type, &meta);
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 6221e8c6ecc3..52f7e9d8c29b 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -995,6 +995,8 @@ static const struct bpf_func_proto bpf_perf_event_output_proto_raw_tp = {
> .arg5_type = ARG_CONST_SIZE_OR_ZERO,
> };
>
[...]
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index 54a50699bbfd..c5e05d1a806f 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -65,6 +65,10 @@ static int (*bpf_perf_event_output)(void *ctx, void *map,
> unsigned long long flags, void *data,
> int size) =
> (void *) BPF_FUNC_perf_event_output;
> +static int (*bpf_skb_output)(void *ctx, void *map,
> + unsigned long long flags, void *data,
> + int size) =
> + (void *) BPF_FUNC_skb_output;
Obsolete now, no more manual list of helpers.
> static int (*bpf_get_stackid)(void *ctx, void *map, int flags) =
> (void *) BPF_FUNC_get_stackid;
> static int (*bpf_probe_write_user)(void *dst, const void *src, int size) =
> --
> 2.20.0
>
