On Fri, Jan 17, 2020 at 4:07 PM Alexei Starovoitov <ast@xxxxxxxxxx> wrote:
>
> Introduce dynamic program extensions. The users can load additional BPF
> functions and replace global functions in previously loaded BPF programs while
> these programs are executing.
>
> Global functions are verified individually by the verifier based on their types only.
> Hence the global function in the new program which types match older function can
> safely replace that corresponding function.
>
> This new function/program is called 'an extension' of old program. At load time
> the verifier uses (attach_prog_fd, attach_btf_id) pair to identify the function
> to be replaced. The BPF program type is derived from the target program into
> extension program. Technically bpf_verifier_ops is copied from target program.
> The BPF_PROG_TYPE_EXT program type is a placeholder. It has empty verifier_ops.
> The extension program can call the same bpf helper functions as target program.
> Single BPF_PROG_TYPE_EXT type is used to extend XDP, SKB and all other program
> types. The verifier allows only one level of replacement. Meaning that the
> extension program cannot recursively extend an extension. That also means that
> the maximum stack size is increasing from 512 to 1024 bytes and maximum
> function nesting level from 8 to 16. The programs don't always consume that
> much. The stack usage is determined by the number of on-stack variables used by
> the program. The verifier could have enforced 512 limit for combined original
> plus extension program, but it makes for difficult user experience. The main
> use case for extensions is to provide generic mechanism to plug external
> programs into policy program or function call chaining.
>
> BPF trampoline is used to track both fentry/fexit and program extensions
> because both are using the same nop slot at the beginning of every BPF
> function. Attaching fentry/fexit to a function that was replaced is not
> allowed. The opposite is true as well. Replacing a function that currently
> being analyzed with fentry/fexit is not allowed. The executable page allocated
> by BPF trampoline is not used by program extensions. This inefficiency will be
> optimized in future patches.
>
> Function by function verification of global function supports scalars and
> pointer to context only. Hence program extensions are supported for such class
> of global functions only. In the future the verifier will be extended with
> support to pointers to structures, arrays with sizes, etc.
>
> Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> ---
> include/linux/bpf.h | 10 ++-
> include/linux/bpf_types.h | 2 +
> include/linux/btf.h | 5 ++
> include/uapi/linux/bpf.h | 1 +
> kernel/bpf/btf.c | 152 +++++++++++++++++++++++++++++++++++++-
> kernel/bpf/syscall.c | 15 +++-
> kernel/bpf/trampoline.c | 38 +++++++++-
> kernel/bpf/verifier.c | 84 ++++++++++++++++-----
> 8 files changed, 281 insertions(+), 26 deletions(-)
>
[...]
> @@ -200,6 +208,26 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog)
> tr = prog->aux->trampoline;
> kind = bpf_attach_type_to_tramp(prog->expected_attach_type);
> mutex_lock(&tr->mutex);
> + if (kind == BPF_TRAMP_REPLACE) {
> + /* If this program already has an extension program
> + * or it has fentry/fexit attached then return EBUSY.
> + */
> + if (tr->extension_prog ||
> + tr->progs_cnt[BPF_TRAMP_FENTRY] +
> + tr->progs_cnt[BPF_TRAMP_FEXIT]) {
> + err = -EBUSY;
> + goto out;
> + }
> + tr->extension_prog = prog;
> + err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP, NULL,
> + prog->bpf_func);
> + goto out;
> + }
> + if (tr->extension_prog) {
> + /* cannot attach fentry/fexit if extension prog is attached */
> + err = -EBUSY;
> + goto out;
> + }
move this check before BPF_TRAMP_REPLACE check and check additonally
for fentry+fexit for BPF_TRAMP_REPLACE? Nothing can replace
extension_prog, right?
> if (tr->progs_cnt[BPF_TRAMP_FENTRY] + tr->progs_cnt[BPF_TRAMP_FEXIT]
> >= BPF_MAX_TRAMP_PROGS) {
> err = -E2BIG;
[...]
> @@ -9788,8 +9789,58 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
> return -EINVAL;
> }
> conservative = aux->func_info_aux[subprog].unreliable;
> + if (prog_extension) {
> + if (conservative) {
> + verbose(env,
> + "Cannot replace static functions\n");
> + return -EINVAL;
> + }
> + if (!prog->jit_requested) {
> + verbose(env,
> + "Extension programs should be JITed\n");
> + return -EINVAL;
> + }
> + env->ops = bpf_verifier_ops[tgt_prog->type];
> + }
> + if (!tgt_prog->jited) {
> + verbose(env, "Can attach to only JITed progs\n");
> + return -EINVAL;
> + }
> + if (tgt_prog->type == prog->type) {
> + /* Cannot fentry/fexit another fentry/fexit program.
> + * Cannot attach program extension to another extension.
> + * It's ok to attach fentry/fexit to extension program.
> + */
> + verbose(env, "Cannot recursively attach\n");
> + return -EINVAL;
> + }
> + if (tgt_prog->type == BPF_PROG_TYPE_TRACING &&
> + tgt_prog->expected_attach_type != BPF_TRACE_RAW_TP &&
if the intent is to prevent extending FENTRY/FEXIT, why not checking
explicitly for those two instead of making assumption that
expected_attach_type can be only one of RAW_TP/FENTRY/FEXIT, this can
easily change in the future. Besides, direct FENTRY/FEXIT comparison
is more self-documenting as well.
> + prog_extension) {
> + /* Program extensions can extend all program types
> + * except fentry/fexit. The reason is the following.
> + * The fentry/fexit programs are used for performance
> + * analysis, stats and can be attached to any program
> + * type except themselves. When extension program is
> + * replacing XDP function it is necessary to allow
> + * performance analysis of all functions. Both original
> + * XDP program and its program extension. Hence
> + * attaching fentry/fexit to BPF_PROG_TYPE_EXT is
> + * allowed. If extending of fentry/fexit was allowed it
> + * would be possible to create long call chain
> + * fentry->extension->fentry->extension beyond
> + * reasonable stack size. Hence extending fentry is not
> + * allowed.
> + */
> + verbose(env, "Cannot extend fentry/fexit\n");
> + return -EINVAL;
> + }
> key = ((u64)aux->id) << 32 | btf_id;
[...]
> @@ -9834,6 +9889,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
> btf_id);
> return -EINVAL;
> }
> + if (prog_extension &&
> + btf_check_type_match(env, prog, btf, t))
this reads so weird... btf_check_type_match (and
btf_check_func_type_match as well) are boolean functions (i.e., either
matches or not, or some error), why not using a conventional
boolean+error return convention: 0 - false, 1 - true, <0 - error
(bug)?
> + return -EINVAL;
> t = btf_type_by_id(btf, t->type);
> if (!btf_type_is_func_proto(t))
> return -EINVAL;
[...]
