[RFC,bpf-next,v2] bpf: Relax tracing prog recursive attach rules

Commit Message

Dmitry Dolgov Nov. 22, 2023, 7:18 p.m. UTC

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/[e-mail]/

Signed-off-by: Dmitrii Dolgov <[e-mail]>
---
Previous discussion: https://lore.kernel.org/bpf/[e-mail]/

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

Comments

Jiri Olsa Nov. 23, 2023, 2:25 p.m. UTC | #1

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/[e-mail]/
> 
> Signed-off-by: Dmitrii Dolgov <[e-mail]>
> ---
> Previous discussion: https://lore.kernel.org/bpf/[e-mail]/
> 
> 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
> 
>

Dmitry Dolgov Nov. 23, 2023, 7:49 p.m. UTC | #2

> 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.

Song Liu Nov. 24, 2023, 7:24 a.m. UTC | #3

On Wed, Nov 22, 2023 at 11:22 AM Dmitrii Dolgov <[e-mail]> 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;
>                 }
[...]

Dmitry Dolgov Nov. 24, 2023, 9:16 p.m. UTC | #4

> 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?

Yonghong Song Nov. 25, 2023, 7:55 p.m. UTC | #5

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?

Song Liu Nov. 25, 2023, 8:39 p.m. UTC | #6

On Fri, Nov 24, 2023 at 1:20 PM Dmitry Dolgov <[e-mail]> 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.

Song Liu Nov. 25, 2023, 8:40 p.m. UTC | #7

On Sat, Nov 25, 2023 at 11:55 AM Yonghong Song <[e-mail]> 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?

Song Liu Nov. 25, 2023, 8:46 p.m. UTC | #8

On Wed, Nov 22, 2023 at 11:22 AM Dmitrii Dolgov <[e-mail]> 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

Dmitry Dolgov Nov. 25, 2023, 9:01 p.m. UTC | #9

> On Sat, Nov 25, 2023 at 12:46:57PM -0800, Song Liu wrote:
> On Wed, Nov 22, 2023 at 11:22 AM Dmitrii Dolgov <[e-mail]> 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.

Yonghong Song Nov. 26, 2023, 1:05 a.m. UTC | #10

On 11/25/23 3:40 PM, Song Liu wrote:
> On Sat, Nov 25, 2023 at 11:55 AM Yonghong Song <[e-mail]> 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.

[...]

Patch

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 {