Message ID | 20231122191816.5572-1-9erthalion6@gmail.com (mailing list archive) |
---|---|
State | RFC |
Delegated to: | BPF |
Headers | show |
Series | [RFC,bpf-next,v2] bpf: Relax tracing prog recursive attach rules | expand |
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@kernel.org/ > > Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com> > --- > Previous discussion: https://lore.kernel.org/bpf/20231114084118.11095-1-9erthalion6@gmail.com/ > > 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 > >
> On Thu, Nov 23, 2023 at 03:25:33PM +0100, Jiri Olsa wrote: > > + 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) > > + 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 > > + printf(" attach depth %d", info->attach_depth); > > + > > I think we should print only if the value != 0 like we do for other fields > > + 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 Agree on all points, thanks. Will post the updated version soon.
On Wed, Nov 22, 2023 at 11:22 AM Dmitrii Dolgov <9erthalion6@gmail.com> 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 If prog B attached to prog A, and prog C attached to prog B, then we detach B. At this point, can we re-attach B to A? > implement, add a new field into bpf_prog_aux to track the fact of > attachment in the target 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; > + } > + attach_depth is calculated at attach time, so... > 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; > + } > + (continue from above) attach_depth is always 0 at program load time, no? Thanks, Song > 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; > } [...]
> On Thu, Nov 23, 2023 at 11:24:34PM -0800, Song Liu wrote: > > 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 > > If prog B attached to prog A, and prog C attached to prog B, then we > detach B. At this point, can we re-attach B to A? Nope, with the proposed changes it still wouldn't be possible to reattach B to A (if we're talking about tracing progs of course), because this time B is an attachment target on its own. > > + 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; > > + } > > + > > attach_depth is calculated at attach time, so... > > > 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; > > + } > > + > > (continue from above) attach_depth is always 0 at program load time, no? Right, it's going to be always 0 for the just loaded program -- but here in verifier we check attach_depth of the target program, which is calculated at some point before. Or were you asking about something else?
On 11/24/23 4:16 PM, Dmitry Dolgov wrote: >> On Thu, Nov 23, 2023 at 11:24:34PM -0800, Song Liu wrote: >>> 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 >> If prog B attached to prog A, and prog C attached to prog B, then we >> detach B. At this point, can we re-attach B to A? > Nope, with the proposed changes it still wouldn't be possible to > reattach B to A (if we're talking about tracing progs of course), > because this time B is an attachment target on its own. IIUC, the 'prog B attached to prog A, and prog C attached to prog B' not really possible. After prog B attached to prog A, we have prog B follower_cnt = 1 prog A attach_depth = 1 Then prog C wants to attach to prog B, since we have prog B follower_cnt = 1, then attaching will fail. If we do have A <- B <- C chain by first prog C attached to prog B, and then prog B attached to A now we have prog B/C follower_cnt = 1 prog A/B attach_depth = 1 after detaching B from A, we have prog B follower_cnt = 0 prog A attach_depth = 0 In this particular case, prog B attaching to prog A should succeed since prog B follower_cnt = 0. Did I miss anything? In the commit message, 'falcosecurity libs project' is mentioned as a use case for chained fentry/fexit bpf programs. I think you should expand the use case in more details. It is possible with use case description, people might find better/alternative solutions for your use case. Also, if you can have a test case to exercise your commit logic, it will be even better. > >>> + 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; >>> + } >>> + >> attach_depth is calculated at attach time, so... >> >>> 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; >>> + } >>> + >> (continue from above) attach_depth is always 0 at program load time, no? > Right, it's going to be always 0 for the just loaded program -- but here > in verifier we check attach_depth of the target program, which is > calculated at some point before. Or were you asking about something else?
On Fri, Nov 24, 2023 at 1:20 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote: > > > On Thu, Nov 23, 2023 at 11:24:34PM -0800, Song Liu wrote: > > > 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 > > > > If prog B attached to prog A, and prog C attached to prog B, then we > > detach B. At this point, can we re-attach B to A? > > Nope, with the proposed changes it still wouldn't be possible to > reattach B to A (if we're talking about tracing progs of course), > because this time B is an attachment target on its own. > > > > + 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; > > > + } > > > + > > > > attach_depth is calculated at attach time, so... > > > > > 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; > > > + } > > > + > > > > (continue from above) attach_depth is always 0 at program load time, no? > > Right, it's going to be always 0 for the just loaded program -- but here > in verifier we check attach_depth of the target program, which is > calculated at some point before. Or were you asking about something else? Actually, I was wrong. attach_depth is checked at BPF_LINK_CREATE. So never mind about this one.
On Sat, Nov 25, 2023 at 11:55 AM Yonghong Song <yonghong.song@linux.dev> wrote: > > > On 11/24/23 4:16 PM, Dmitry Dolgov wrote: > >> On Thu, Nov 23, 2023 at 11:24:34PM -0800, Song Liu wrote: > >>> 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 > >> If prog B attached to prog A, and prog C attached to prog B, then we > >> detach B. At this point, can we re-attach B to A? > > Nope, with the proposed changes it still wouldn't be possible to > > reattach B to A (if we're talking about tracing progs of course), > > because this time B is an attachment target on its own. > > IIUC, the 'prog B attached to prog A, and prog C attached to prog B' > not really possible. > After prog B attached to prog A, we have > prog B follower_cnt = 1 > prog A attach_depth = 1 I think prog A has follower_cnt=1, while prog B has follow_cnt=0, no? Thanks, Song > Then prog C wants to attach to prog B, > since we have prog B follower_cnt = 1, then attaching will fail. > > If we do have A <- B <- C chain by > first prog C attached to prog B, and then prog B attached to A > now we have > prog B/C follower_cnt = 1 > prog A/B attach_depth = 1 > after detaching B from A, we have > prog B follower_cnt = 0 > prog A attach_depth = 0 > > In this particular case, prog B attaching to prog A should succeed > since prog B follower_cnt = 0. > > Did I miss anything? > > In the commit message, 'falcosecurity libs project' is mentioned as a use > case for chained fentry/fexit bpf programs. I think you should expand the > use case in more details. It is possible with use case description, people > might find better/alternative solutions for your use case. > > Also, if you can have a test case to exercise your commit logic, > it will be even better. > > > > >>> + 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; > >>> + } > >>> + > >> attach_depth is calculated at attach time, so... > >> > >>> 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; > >>> + } > >>> + > >> (continue from above) attach_depth is always 0 at program load time, no? > > Right, it's going to be always 0 for the just loaded program -- but here > > in verifier we check attach_depth of the target program, which is > > calculated at some point before. Or were you asking about something else?
On Wed, Nov 22, 2023 at 11:22 AM Dmitrii Dolgov <9erthalion6@gmail.com> 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. Actually, is it really possible to have tracing programs form a cycle? AFAICT, attach_prog_fd is specified at BPF_PROG_LOAD. So a program can never attach to another program loaded after it. Did I miss something? Only BPF_PROG_TYPE_EXT program can change target_fd at BPF_LINK_CREATE time. Thanks, Song
> On Sat, Nov 25, 2023 at 12:46:57PM -0800, Song Liu wrote: > On Wed, Nov 22, 2023 at 11:22 AM Dmitrii Dolgov <9erthalion6@gmail.com> 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. > > Actually, is it really possible to have tracing programs form a cycle? > AFAICT, attach_prog_fd is specified at BPF_PROG_LOAD. So a > program can never attach to another program loaded after it. Did I > miss something? Only BPF_PROG_TYPE_EXT program can change > target_fd at BPF_LINK_CREATE time. Yes, I've mentioned that in the v1 thread, it doesn't look like the actual tracing program cycle is possible currently (to test that a cycle would be prevented I had to minimally modify few other bits of prog lifecycle; although those changes were mostly in the verifier, I can check about attach_prog_fd). My understanding is that the original intent was to make everything robust against the future changes, even in case if the code around would make cycles possible at some point.
On 11/25/23 3:40 PM, Song Liu wrote: > On Sat, Nov 25, 2023 at 11:55 AM Yonghong Song <yonghong.song@linux.dev> wrote: >> >> On 11/24/23 4:16 PM, Dmitry Dolgov wrote: >>>> On Thu, Nov 23, 2023 at 11:24:34PM -0800, Song Liu wrote: >>>>> 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 >>>> If prog B attached to prog A, and prog C attached to prog B, then we >>>> detach B. At this point, can we re-attach B to A? >>> Nope, with the proposed changes it still wouldn't be possible to >>> reattach B to A (if we're talking about tracing progs of course), >>> because this time B is an attachment target on its own. >> IIUC, the 'prog B attached to prog A, and prog C attached to prog B' >> not really possible. >> After prog B attached to prog A, we have >> prog B follower_cnt = 1 >> prog A attach_depth = 1 > I think prog A has follower_cnt=1, while prog B has follow_cnt=0, no? I checked bpf_tracing_prog_attach() again. Yes, I made a dumb mistake and indeed After prog B attached to prog A, we should have prog A follower_cnt = 1 and prog B attach_depth = 1 > > Thanks, > Song > >> Then prog C wants to attach to prog B, >> since we have prog B follower_cnt = 1, then attaching will fail. >> >> If we do have A <- B <- C chain by >> first prog C attached to prog B, and then prog B attached to A >> now we have >> prog B/C follower_cnt = 1 >> prog A/B attach_depth = 1 >> after detaching B from A, we have >> prog B follower_cnt = 0 >> prog A attach_depth = 0 >> >> In this particular case, prog B attaching to prog A should succeed >> since prog B follower_cnt = 0. >> >> Did I miss anything? >> >> In the commit message, 'falcosecurity libs project' is mentioned as a use >> case for chained fentry/fexit bpf programs. I think you should expand the >> use case in more details. It is possible with use case description, people >> might find better/alternative solutions for your use case. >> >> Also, if you can have a test case to exercise your commit logic, >> it will be even better. [...]
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--; /* 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; + } + 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. * 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); + 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 {
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@kernel.org/ Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com> --- Previous discussion: https://lore.kernel.org/bpf/20231114084118.11095-1-9erthalion6@gmail.com/ 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(-) base-commit: 100888fb6d8a185866b1520031ee7e3182b173de