On Sat, Dec 02, 2023 at 08:15:47PM +0100, Dmitrii Dolgov wrote: > Currently, it's not allowed to attach an fentry/fexit prog to another > one fentry/fexit. 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. But currently it seems impossible to load and attach tracing > programs in a way that will form such a cycle. The limitation is coming > from the fact that attach_prog_fd is specified at the prog load (thus > making it impossible to attach to a program loaded after it in this > way), as well as tracing progs not implementing link_detach. > > Replace "no same type" requirement with verification that no more than > one level of attachment nesting is allowed. In this way only one > fentry/fexit program could be attached to another fentry/fexit to cover > profiling use case, and still no cycle could be formed. To implement, > add a new field into bpf_prog_aux to track the depth of attachment. > > [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/20231201154734.8545-1-9erthalion6@xxxxxxxxx/ > > Changes in v6: > - Apply nesting level limitation only to tracing programs, otherwise > it's possible to apply it in "fentry->extension" case and break it > > Changes in v5: > - Remove follower_cnt and drop unreachable cycle prevention condition > - Allow only one level of attachment nesting > - Do not display attach_depth in bpftool, as it doesn't make sense > anymore > > Changes in v3: > - Fix incorrect decreasing of attach_depth, setting to 0 instead > - Place bookkeeping later, to not miss a cleanup if needed > - Display attach_depth in bpftool only if the value is not 0 > > 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 | 1 + > include/uapi/linux/bpf.h | 1 + > kernel/bpf/syscall.c | 12 ++++++++++++ > kernel/bpf/verifier.c | 33 +++++++++++++++++++-------------- > tools/include/uapi/linux/bpf.h | 1 + > 5 files changed, 34 insertions(+), 14 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index eb447b0a9423..1588e48fe31c 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1400,6 +1400,7 @@ 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; /* for tracing prog, level of attachment nesting */ > 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 e88746ba7d21..9cf45ad914f1 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -6468,6 +6468,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 5e43ddd1b83f..9c56b5970d7e 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -3039,6 +3039,7 @@ static void bpf_tracing_link_release(struct bpf_link *link) > > bpf_trampoline_put(tr_link->trampoline); > > + link->prog->aux->attach_depth = 0; > /* tgt_prog is NULL if target is a kernel function */ > if (tr_link->tgt_prog) > bpf_prog_put(tr_link->tgt_prog); > @@ -3243,6 +3244,16 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog, > goto out_unlock; > } > > + if (tgt_prog) { > + /* Bookkeeping for managing the prog attachment chain. If it's a > + * tracing program, bump the level, otherwise carry it on. > + */ > + if (prog->type == BPF_PROG_TYPE_TRACING) > + prog->aux->attach_depth = tgt_prog->aux->attach_depth + 1; > + else > + prog->aux->attach_depth = tgt_prog->aux->attach_depth; > + } I'm not sure why we need to keep the attach_depth for all program types, I might be missing something but could we perhaps just use flag related just to tracing programs like in patch below jirka --- diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 10e5e4d8a00f..2387a29db193 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1425,6 +1425,7 @@ struct bpf_prog_aux { bool dev_bound; /* Program is bound to the netdev. */ bool offload_requested; /* Program is bound and offloaded to the netdev. */ bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */ + bool attach_tracing_prog; /* true if attaching to tracing prog */ bool func_proto_unreliable; bool sleepable; bool tail_call_reachable; diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 991e7e73ff3c..a3d309197bc0 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -3277,6 +3277,9 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog, goto out_unlock; } + if (tgt_prog && prog->type == BPF_PROG_TYPE_TRACING) + prog->aux->attach_tracing_prog = true; + link->tgt_prog = tgt_prog; link->trampoline = tr; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index e5ce530641ba..899e3b34c058 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -20235,8 +20235,13 @@ 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. + if (prog->type == BPF_PROG_TYPE_TRACING) { + if (aux->attach_tracing_prog) { + bpf_log(log, "Cannot nest tracing program attach more than once\n"); + return -EINVAL; + } + } else if (tgt_prog->type == prog->type) { + /* * Cannot attach program extension to another extension. * It's ok to attach fentry/fexit to extension program. */
