diff mbox series

[bpf-next,v4,1/3] bpf: Relax tracing prog recursive attach rules

Message ID 20231129195240.19091-2-9erthalion6@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Relax tracing prog recursive attach rules | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Posting correctly formatted
netdev/codegen success Generated files up to date
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2910 this patch: 2910
netdev/cc_maintainers warning 6 maintainers not CCed: kpsingh@kernel.org haoluo@google.com jolsa@kernel.org quentin@isovalent.com john.fastabend@gmail.com sdf@google.com
netdev/build_clang success Errors and warnings before: 1278 this patch: 1278
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 2992 this patch: 2992
netdev/checkpatch warning CHECK: Alignment should match open parenthesis WARNING: line length of 83 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-3 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-15 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-16 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-llvm-16 / build / build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-llvm-16 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-llvm-16 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-16 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-16 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-16 / veristat
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc

Commit Message

Dmitry Dolgov Nov. 29, 2023, 7:52 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 number of
attachments to 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.

Note, that currently, due to various limitations, it's actually not
possible to form such an attachment cycle the original implementation
was prohibiting. It seems that the idea was to make this part robust
even in the view of potential future changes.

[1]: https://lore.kernel.org/bpf/20191108064039.2041889-16-ast@kernel.org/

Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>
---
 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       |  3 +++
 tools/include/uapi/linux/bpf.h |  1 +
 6 files changed, 34 insertions(+), 4 deletions(-)

Comments

Song Liu Nov. 29, 2023, 11:58 p.m. UTC | #1
On Wed, Nov 29, 2023 at 11:56 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
> implement, add a new field into bpf_prog_aux to track the number of
> attachments to 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.
>
> Note, that currently, due to various limitations, it's actually not
> possible to form such an attachment cycle the original implementation
> was prohibiting. It seems that the idea was to make this part robust
> even in the view of potential future changes.
>
> [1]: https://lore.kernel.org/bpf/20191108064039.2041889-16-ast@kernel.org/
>
> Signed-off-by: Dmitrii Dolgov <9erthalion6@gmail.com>

We discussed this in earlier version:

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

I think this can be problematic for some users. Basically, doing
profiling on prog B can cause it to not work (cannot re-attach).

Given it is not possible to create a call circle, shall we remove
this issue?

Thanks,
Song
Dmitry Dolgov Nov. 30, 2023, 10:08 a.m. UTC | #2
> On Wed, Nov 29, 2023 at 03:58:02PM -0800, Song Liu wrote:
> We discussed this in earlier version:
>
> "
> > 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.
> "
>
> I think this can be problematic for some users. Basically, doing
> profiling on prog B can cause it to not work (cannot re-attach).

Sorry, I was probably not clear enough about this first time. Let me
elaborate:

* The patch affects only tracing programs (only they can reach the
  corresponding verifier change), so I assume in your example at least B
  and A are fentry/fexit.

* The patch is less restrictive than the current kernel implementation.
  Currently, no attach of a tracing program to another tracing program is
  possible, thus IIUC the case you describe (A, B: tracing, C -> B -> A,
  then re-attach B -> A) is not possible without the patch (the first B
  -> A is going to return a verifier error).

* I've also tried to reproduce this use case with the patch, and noticed
  that link_detach is not supported for tracing progs. Which means the
  re-attach part in (C -> B -> A) has to be done via unloading of prog B
  and C, then reattaching them one-by-one back. This is another
  limitation why the case above doesn't seem to be possible (attaching
  one-by-one back would of course work without any issues even with the
  patch).

Does it all make sense to you, or am I missing something about the
problem you describe?

> Given it is not possible to create a call circle, shall we remove
> this issue?

I was originally thinking about this when preparing the patch, even
independently of the question above, simply remove verifier limitation
for an impossible situation sounds interesting. I see the following
pros/cons:

* Just remove the verifier limitation on recursive attachment is of
  course easier.

* At the same time it makes the implementation less "defensive" against
  future changes.

* Tracking attachment depth & followers might be useful in some other
  contexts.

All in all I've decided that more elaborated approach is slightly
better. But if everyone in the community agrees that less
"defensiveness" is not an issue and verifier could be simply made less
restrictive, I'm fine with that. What do you think?
Jiri Olsa Nov. 30, 2023, 2:30 p.m. UTC | #3
On Wed, Nov 29, 2023 at 08:52:36PM +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 number of
> attachments to 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.
> 
> Note, that currently, due to various limitations, it's actually not
> possible to form such an attachment cycle the original implementation
> was prohibiting. It seems that the idea was to make this part robust
> even in the view of potential future changes.

SNIP

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8e7b6072e3f4..31ffcffb7198 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20109,6 +20109,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;
> +		}

IIUC the use case you have is to be able to attach fentry program on
another fentry program.. do you have use case that needs more than
that?

could we allow just single nesting? that might perhaps endup in easier
code while still allowing your use case?


> +
>  		if (bpf_prog_is_dev_bound(prog->aux) &&
>  		    !bpf_prog_dev_bound_match(prog, tgt_prog)) {
>  			bpf_log(log, "Target program bound device mismatch");
> @@ -20147,9 +20153,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.

next condition below denies extension on fentry/fexit and the reason
is the possibility:
  "... to create long call chain * fentry->extension->fentry->extension
  beyond reasonable stack size ..."

that might be problem also here with 32 allowed nesting

also the the comment mentions that it's not possible to attach fentry/fexit
on themselfs, so it should be updated

jirka

>  			 */
>  			bpf_log(log, "Cannot recursively attach\n");
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index feb8e305804f..83f999f5505d 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -558,6 +558,9 @@ static void print_prog_plain(struct bpf_prog_info *info, int fd, bool orphaned)
>  	if (orphaned)
>  		printf("  orphaned");
>  
> +	if (info->attach_depth)
> +		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 e88746ba7d21..9cf45ad914f1 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/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 {
> -- 
> 2.41.0
>
Dmitry Dolgov Nov. 30, 2023, 6:57 p.m. UTC | #4
> On Thu, Nov 30, 2023 at 03:30:38PM +0100, Jiri Olsa wrote:
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 8e7b6072e3f4..31ffcffb7198 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -20109,6 +20109,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;
> > +		}
>
> IIUC the use case you have is to be able to attach fentry program on
> another fentry program.. do you have use case that needs more than
> that?
>
> could we allow just single nesting? that might perhaps endup in easier
> code while still allowing your use case?

Right, there is no use case beyond attaching an fentry to another one,
so having just a single nesting should be fine.

> > +			/*
> > +			 * 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.
>
> next condition below denies extension on fentry/fexit and the reason
> is the possibility:
>   "... to create long call chain * fentry->extension->fentry->extension
>   beyond reasonable stack size ..."
>
> that might be problem also here with 32 allowed nesting

Reducing nesting to only one level probably will lift this question, but
for posterity, what kind of problem similar to "fentry->extension->fentry->..."
do you have in mind?
Song Liu Nov. 30, 2023, 8:19 p.m. UTC | #5
On Thu, Nov 30, 2023 at 2:08 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> > On Wed, Nov 29, 2023 at 03:58:02PM -0800, Song Liu wrote:
> > We discussed this in earlier version:
> >
> > "
> > > 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.
> > "
> >
> > I think this can be problematic for some users. Basically, doing
> > profiling on prog B can cause it to not work (cannot re-attach).
>
> Sorry, I was probably not clear enough about this first time. Let me
> elaborate:
>
> * The patch affects only tracing programs (only they can reach the
>   corresponding verifier change), so I assume in your example at least B
>   and A are fentry/fexit.
>
> * The patch is less restrictive than the current kernel implementation.
>   Currently, no attach of a tracing program to another tracing program is
>   possible, thus IIUC the case you describe (A, B: tracing, C -> B -> A,
>   then re-attach B -> A) is not possible without the patch (the first B
>   -> A is going to return a verifier error).

Yes, I was aware this is less restrictive than current rules, and I think
this can be very useful.

> * I've also tried to reproduce this use case with the patch, and noticed
>   that link_detach is not supported for tracing progs. Which means the
>   re-attach part in (C -> B -> A) has to be done via unloading of prog B
>   and C, then reattaching them one-by-one back. This is another
>   limitation why the case above doesn't seem to be possible (attaching
>   one-by-one back would of course work without any issues even with the
>   patch).

I think there is an issue without re-attach:
1. Load program A, B, C;
2. Attach C to B;
3. Attach B to A will fail.

> Does it all make sense to you, or am I missing something about the
> problem you describe?
>
> > Given it is not possible to create a call circle, shall we remove
> > this issue?
>
> I was originally thinking about this when preparing the patch, even
> independently of the question above, simply remove verifier limitation
> for an impossible situation sounds interesting. I see the following
> pros/cons:
>
> * Just remove the verifier limitation on recursive attachment is of
>   course easier.
>
> * At the same time it makes the implementation less "defensive" against
>   future changes.
>
> * Tracking attachment depth & followers might be useful in some other
>   contexts.
>
> All in all I've decided that more elaborated approach is slightly
> better. But if everyone in the community agrees that less
> "defensiveness" is not an issue and verifier could be simply made less
> restrictive, I'm fine with that. What do you think?

I think the follower_cnt check is not necessary, and may cause confusions.
For tracing programs, we are very specific on "which function(s) are we
tracing". So I don't think circular attachment can be a real issue. Do we
have potential use cases that make the circular attach possible?

Thanks,
Song
Dmitry Dolgov Nov. 30, 2023, 8:41 p.m. UTC | #6
> On Thu, Nov 30, 2023 at 12:19:31PM -0800, Song Liu wrote:
> > All in all I've decided that more elaborated approach is slightly
> > better. But if everyone in the community agrees that less
> > "defensiveness" is not an issue and verifier could be simply made less
> > restrictive, I'm fine with that. What do you think?
>
> I think the follower_cnt check is not necessary, and may cause confusions.
> For tracing programs, we are very specific on "which function(s) are we
> tracing". So I don't think circular attachment can be a real issue. Do we
> have potential use cases that make the circular attach possible?

At the moment no, nothing like that in sight. Ok, you've convinced me --
plus since nobody has yet actively mentioned that potential cycle
prevention is nice to have, I can drop follower_cnt and the
corresponding check in the verifier.
Jiri Olsa Nov. 30, 2023, 10:34 p.m. UTC | #7
On Thu, Nov 30, 2023 at 07:57:52PM +0100, Dmitry Dolgov wrote:
> > On Thu, Nov 30, 2023 at 03:30:38PM +0100, Jiri Olsa wrote:
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 8e7b6072e3f4..31ffcffb7198 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -20109,6 +20109,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;
> > > +		}
> >
> > IIUC the use case you have is to be able to attach fentry program on
> > another fentry program.. do you have use case that needs more than
> > that?
> >
> > could we allow just single nesting? that might perhaps endup in easier
> > code while still allowing your use case?
> 
> Right, there is no use case beyond attaching an fentry to another one,
> so having just a single nesting should be fine.
> 
> > > +			/*
> > > +			 * 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.
> >
> > next condition below denies extension on fentry/fexit and the reason
> > is the possibility:
> >   "... to create long call chain * fentry->extension->fentry->extension
> >   beyond reasonable stack size ..."
> >
> > that might be problem also here with 32 allowed nesting
> 
> Reducing nesting to only one level probably will lift this question, but
> for posterity, what kind of problem similar to "fentry->extension->fentry->..."
> do you have in mind?

I meant that allowing that amount of nesting will make it easier to get
to a point where we use all the available stack size

jirka
Artem Savkov Dec. 1, 2023, 9:55 a.m. UTC | #8
On Thu, Nov 30, 2023 at 09:41:34PM +0100, Dmitry Dolgov wrote:
> > On Thu, Nov 30, 2023 at 12:19:31PM -0800, Song Liu wrote:
> > > All in all I've decided that more elaborated approach is slightly
> > > better. But if everyone in the community agrees that less
> > > "defensiveness" is not an issue and verifier could be simply made less
> > > restrictive, I'm fine with that. What do you think?
> >
> > I think the follower_cnt check is not necessary, and may cause confusions.
> > For tracing programs, we are very specific on "which function(s) are we
> > tracing". So I don't think circular attachment can be a real issue. Do we
> > have potential use cases that make the circular attach possible?
> 
> At the moment no, nothing like that in sight. Ok, you've convinced me --
> plus since nobody has yet actively mentioned that potential cycle
> prevention is nice to have, I can drop follower_cnt and the
> corresponding check in the verifier.

If you are worried about potential future situations where cyclic
attaches are possible would it make sense to add a test that checks if
this fails?
Dmitry Dolgov Dec. 1, 2023, 2:29 p.m. UTC | #9
> On Fri, Dec 01, 2023 at 10:55:09AM +0100, Artem Savkov wrote:
> On Thu, Nov 30, 2023 at 09:41:34PM +0100, Dmitry Dolgov wrote:
> > > On Thu, Nov 30, 2023 at 12:19:31PM -0800, Song Liu wrote:
> > > > All in all I've decided that more elaborated approach is slightly
> > > > better. But if everyone in the community agrees that less
> > > > "defensiveness" is not an issue and verifier could be simply made less
> > > > restrictive, I'm fine with that. What do you think?
> > >
> > > I think the follower_cnt check is not necessary, and may cause confusions.
> > > For tracing programs, we are very specific on "which function(s) are we
> > > tracing". So I don't think circular attachment can be a real issue. Do we
> > > have potential use cases that make the circular attach possible?
> >
> > At the moment no, nothing like that in sight. Ok, you've convinced me --
> > plus since nobody has yet actively mentioned that potential cycle
> > prevention is nice to have, I can drop follower_cnt and the
> > corresponding check in the verifier.
>
> If you are worried about potential future situations where cyclic
> attaches are possible would it make sense to add a test that checks if
> this fails?

Do you mean a test that cyclic attachment doesn't work due to the
current limitations (not the one in verifier)? Sounds interesting, but
I'm hesitant to add such a test -- it would verify a property that is
more like a side effect of e.g. having attach_prog_fd at prog load, and
most likely will be either incomplete or flaky.

At the moment I'm pretty convinced that even if the future changes will
make cycles possible, it's something that has to be discussed at the
point when such change will land, and it's fine for now to simplify
things.
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index eb447b0a9423..d7ace97d8e4b 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1400,6 +1400,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 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..a595d7a62dbc 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3039,9 +3039,12 @@  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)
+	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)
@@ -3243,6 +3246,12 @@  static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 		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;
+	}
+
 	link->tgt_prog = tgt_prog;
 	link->trampoline = tr;
 
@@ -4510,6 +4519,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 8e7b6072e3f4..31ffcffb7198 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20109,6 +20109,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");
@@ -20147,9 +20153,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 feb8e305804f..83f999f5505d 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -558,6 +558,9 @@  static void print_prog_plain(struct bpf_prog_info *info, int fd, bool orphaned)
 	if (orphaned)
 		printf("  orphaned");
 
+	if (info->attach_depth)
+		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 e88746ba7d21..9cf45ad914f1 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/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 {