On Wed, Nov 22, 2023 at 08:18:09PM +0100, Dmitrii Dolgov wrote: > Currently, it's not allowed to attach an fentry/fexit prog to another > one of the same type. At the same time it's not uncommon to see a > tracing program with lots of logic in use, and the attachment limitation > prevents usage of fentry/fexit for performance analysis (e.g. with > "bpftool prog profile" command) in this case. An example could be > falcosecurity libs project that uses tp_btf tracing programs. > > Following the corresponding discussion [1], the reason for that is to > avoid tracing progs call cycles without introducing more complex > solutions. Relax "no same type" requirement to "no progs that are > already an attach target themselves" for the tracing type. In this way > only a standalone tracing program (without any other progs attached to > it) could be attached to another one, and no cycle could be formed. To > implement, add a new field into bpf_prog_aux to track the fact of > attachment in the target prog. > > As a side effect of this change alone, one could create an unbounded > chain of tracing progs attached to each other. Similar issues between > fentry/fexit and extend progs are addressed via forbidding certain > combinations that could lead to similar chains. Introduce an > attach_depth field to limit the attachment chain, and display it in > bpftool. > > [1]: https://lore.kernel.org/bpf/20191108064039.2041889-16-ast@xxxxxxxxxx/ > > Signed-off-by: Dmitrii Dolgov <9erthalion6@xxxxxxxxx> > --- > Previous discussion: https://lore.kernel.org/bpf/20231114084118.11095-1-9erthalion6@xxxxxxxxx/ > > Changes in v2: > - Verify tgt_prog is not null > - Replace boolean followed with number of followers, to handle > multiple progs attaching/detaching > > include/linux/bpf.h | 2 ++ > include/uapi/linux/bpf.h | 1 + > kernel/bpf/syscall.c | 12 +++++++++++- > kernel/bpf/verifier.c | 19 ++++++++++++++++--- > tools/bpf/bpftool/prog.c | 2 ++ > tools/include/uapi/linux/bpf.h | 1 + > 6 files changed, 33 insertions(+), 4 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 4001d11be151..1b890f65ac39 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1394,6 +1394,8 @@ struct bpf_prog_aux { > u32 real_func_cnt; /* includes hidden progs, only used for JIT and freeing progs */ > u32 func_idx; /* 0 for non-func prog, the index in func array for func prog */ > u32 attach_btf_id; /* in-kernel BTF type id to attach to */ > + u32 attach_depth; /* position of the prog in the attachment chain */ > + u32 follower_cnt; /* number of programs attached to it */ > u32 ctx_arg_info_size; > u32 max_rdonly_access; > u32 max_rdwr_access; > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 7cf8bcf9f6a2..aa6614547ef6 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -6465,6 +6465,7 @@ struct bpf_prog_info { > __u32 verified_insns; > __u32 attach_btf_obj_id; > __u32 attach_btf_id; > + __u32 attach_depth; > } __attribute__((aligned(8))); > > struct bpf_map_info { > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 0ed286b8a0f0..1809595958ef 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -3038,9 +3038,12 @@ static void bpf_tracing_link_release(struct bpf_link *link) > > bpf_trampoline_put(tr_link->trampoline); > > + link->prog->aux->attach_depth--; should we just set it to 0 ? the number is assigned from tgt_prog, so I think we'll endup with wrong up number here after detach (for both tgt_prog or kernel function) > /* tgt_prog is NULL if target is a kernel function */ > - if (tr_link->tgt_prog) > + if (tr_link->tgt_prog) { > + tr_link->tgt_prog->aux->follower_cnt--; > bpf_prog_put(tr_link->tgt_prog); > + } > } > > static void bpf_tracing_link_dealloc(struct bpf_link *link) > @@ -3235,6 +3238,12 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog, > if (err) > goto out_unlock; > > + if (tgt_prog) { > + /* Bookkeeping for managing the prog attachment chain. */ > + tgt_prog->aux->follower_cnt++; > + prog->aux->attach_depth = tgt_prog->aux->attach_depth + 1; > + } missing cleanup/dec if the next bpf_trampoline_link_prog call fails? probably better move that accounting after that call > + > err = bpf_trampoline_link_prog(&link->link, tr); > if (err) { > bpf_link_cleanup(&link_primer); > @@ -4509,6 +4518,7 @@ static int bpf_prog_get_info_by_fd(struct file *file, > if (prog->aux->btf) > info.btf_id = btf_obj_id(prog->aux->btf); > info.attach_btf_id = prog->aux->attach_btf_id; > + info.attach_depth = prog->aux->attach_depth; > if (attach_btf) > info.attach_btf_obj_id = btf_obj_id(attach_btf); > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 9ae6eae13471..de058a83d769 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -20329,6 +20329,12 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > if (tgt_prog) { > struct bpf_prog_aux *aux = tgt_prog->aux; > > + if (aux->attach_depth >= 32) { > + bpf_log(log, "Target program attach depth is %d. Too large\n", > + aux->attach_depth); > + return -EINVAL; > + } > + > if (bpf_prog_is_dev_bound(prog->aux) && > !bpf_prog_dev_bound_match(prog, tgt_prog)) { > bpf_log(log, "Target program bound device mismatch"); > @@ -20367,9 +20373,16 @@ int bpf_check_attach_target(struct bpf_verifier_log *log, > bpf_log(log, "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. > + if (tgt_prog->type == prog->type && > + (prog_extension || prog->aux->follower_cnt > 0)) { > + /* > + * To avoid potential call chain cycles, prevent attaching programs > + * of the same type. The only exception is standalone fentry/fexit > + * programs that themselves are not attachment targets. > + * That means: > + * - Cannot attach followed fentry/fexit to another > + * fentry/fexit program. > + * - Cannot attach program extension to another extension. would be great to have tests for this > * It's ok to attach fentry/fexit to extension program. > */ > bpf_log(log, "Cannot recursively attach\n"); > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index 7ec4f5671e7a..24565e8bb825 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -554,6 +554,8 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd) > printf(" memlock %sB", memlock); > free(memlock); > > + printf(" attach depth %d", info->attach_depth); > + I think we should print only if the value != 0 like we do for other fields jirka > if (info->nr_map_ids) > show_prog_maps(fd, info->nr_map_ids); > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h > index 7cf8bcf9f6a2..aa6614547ef6 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -6465,6 +6465,7 @@ struct bpf_prog_info { > __u32 verified_insns; > __u32 attach_btf_obj_id; > __u32 attach_btf_id; > + __u32 attach_depth; > } __attribute__((aligned(8))); > > struct bpf_map_info { > > base-commit: 100888fb6d8a185866b1520031ee7e3182b173de > -- > 2.41.0 > >
