netdev - Re: [PATCH v3 bpf-next 15/18] bpf: Support attaching tracing BPF program to other BPF programs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 9 Nov 2019 23:17:37 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <ast@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>, x86@...nel.org,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v3 bpf-next 15/18] bpf: Support attaching tracing BPF
 program to other BPF programs

On Thu, Nov 7, 2019 at 10:41 PM Alexei Starovoitov <ast@...nel.org> wrote:
>
> Allow FENTRY/FEXIT BPF programs to attach to other BPF programs of any type
> including their subprograms. This feature allows snooping on input and output
> packets in XDP, TC programs including their return values. In order to do that
> the verifier needs to track types not only of vmlinux, but types of other BPF
> programs as well. The verifier also needs to translate uapi/linux/bpf.h types
> used by networking programs into kernel internal BTF types used by FENTRY/FEXIT
> BPF programs. In some cases LLVM optimizations can remove arguments from BPF
> subprograms without adjusting BTF info that LLVM backend knows. When BTF info
> disagrees with actual types that the verifiers sees the BPF trampoline has to
> fallback to conservative and treat all arguments as u64. The FENTRY/FEXIT
> program can still attach to such subprograms, but won't be able to recognize
> pointer types like 'struct sk_buff *' into won't be able to pass them to
> bpf_skb_output() for dumping to user space.
>
> The BPF_PROG_LOAD command is extended with attach_prog_fd field. When it's set
> to zero the attach_btf_id is one vmlinux BTF type ids. When attach_prog_fd
> points to previously loaded BPF program the attach_btf_id is BTF type id of
> main function or one of its subprograms.
>
> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
> ---
>  arch/x86/net/bpf_jit_comp.c |  3 +-
>  include/linux/bpf.h         |  2 +
>  include/linux/btf.h         |  1 +
>  include/uapi/linux/bpf.h    |  1 +
>  kernel/bpf/btf.c            | 58 +++++++++++++++++++---
>  kernel/bpf/core.c           |  2 +
>  kernel/bpf/syscall.c        | 19 +++++--
>  kernel/bpf/verifier.c       | 98 +++++++++++++++++++++++++++++--------
>  kernel/trace/bpf_trace.c    |  2 -
>  9 files changed, 151 insertions(+), 35 deletions(-)
>

[...]

> +
> +static bool btf_translate_to_vmlinux(struct bpf_verifier_log *log,
> +                                    struct btf *btf,
> +                                    const struct btf_type *t,
> +                                    struct bpf_insn_access_aux *info)
> +{
> +       const char *tname = __btf_name_by_offset(btf, t->name_off);
> +       int btf_id;
> +
> +       if (!tname) {
> +               bpf_log(log, "Program's type doesn't have a name\n");
> +               return false;
> +       }
> +       if (strcmp(tname, "__sk_buff") == 0) {

might be a good idea to ensure that t's type is also a struct?

> +               btf_id = btf_resolve_helper_id(log, &bpf_skb_output_proto, 0);

This is kind of ugly and high-maintenance. Have you considered having
something like this, to do this mapping:

struct bpf_ctx_mapping {
    struct sk_buff *__sk_buff;
    struct xdp_buff *xdp_md;
};

So field name is a name you are trying to match, while field type is
actual type you are mapping to? You won't need to find special
function protos (like bpf_skb_output_proto), it will be easy to
extend, you'll have real vmlinux types automatically captured for you
(you'll just have to initially find bpf_ctx_mapping's btf_id).


> +               if (btf_id < 0)
> +                       return false;
> +               info->btf_id = btf_id;
> +               return true;
> +       }
> +       return false;
> +}
>

[...]

> +               if (tgt_prog && conservative) {
> +                       struct btf_func_model *m = &tr->func.model;
> +
> +                       /* BTF function prototype doesn't match the verifier types.
> +                        * Fall back to 5 u64 args.
> +                        */
> +                       for (i = 0; i < 5; i++)
> +                               m->arg_size[i] = 8;
> +                       m->ret_size = 8;
> +                       m->nr_args = 5;
> +                       prog->aux->attach_func_proto = NULL;
> +               } else {
> +                       ret = btf_distill_func_proto(&env->log, btf, t,
> +                                                    tname, &tr->func.model);

there is nothing preventing some parallel thread to modify
tr->func.model in parallel, right? Should these modifications be
either locked or at least WRITE_ONCE, similar to
btf_resolve_helper_id?


> +                       if (ret < 0)
> +                               goto out;
> +               }

[...]