| Message ID | 20200514053206.1298415-1-yhs@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: enforce returning 0 for fentry/fexit programs | expand |
On Wed, May 13, 2020 at 10:32 PM Yonghong Song <yhs@fb.com> wrote: > > Currently, tracing/fentry and tracing/fexit prog > return values are not enforced. In trampoline codes, > the fentry/fexit prog return values are ignored. > Let us enforce it to be 0 to avoid confusion and > allows potential future extension. > > This patch also explicitly added return value > checking for tracing/raw_tp, tracing/fmod_ret, > and freplace programs such that these program > return values can be anything. The purpose are > two folds: > 1. to make it explicit about return value expectations > for these programs in verifier. > 2. for tracing prog_type, if a future attach type > is added, the default is -ENOTSUPP which will > enforce to specify return value ranges explicitly. > > Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline") > Signed-off-by: Yonghong Song <yhs@fb.com> > --- Looks good, except a nit below. Acked-by: Andrii Nakryiko <andriin@fb.com> [...] > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index fa1d8245b925..2d80cce0a28a 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -7059,6 +7059,24 @@ static int check_return_code(struct bpf_verifier_env *env) > return 0; > range = tnum_const(0); > break; > + case BPF_PROG_TYPE_TRACING: > + switch ((env->prog->expected_attach_type)) { nit: extra pair of ()? > + case BPF_TRACE_FENTRY: > + case BPF_TRACE_FEXIT: > + range = tnum_const(0); > + break; > + case BPF_TRACE_RAW_TP: > + case BPF_MODIFY_RETURN: > + return 0; > + default: > + return -ENOTSUPP; > + } > + > + break; > + case BPF_PROG_TYPE_EXT: > + /* freplace program can return anything as its return value > + * depends on the to-be-replaced kernel func or bpf program. > + */ > default: > return 0; > } > -- > 2.24.1 >
On 5/13/20 11:14 PM, Andrii Nakryiko wrote: > On Wed, May 13, 2020 at 10:32 PM Yonghong Song <yhs@fb.com> wrote: >> >> Currently, tracing/fentry and tracing/fexit prog >> return values are not enforced. In trampoline codes, >> the fentry/fexit prog return values are ignored. >> Let us enforce it to be 0 to avoid confusion and >> allows potential future extension. >> >> This patch also explicitly added return value >> checking for tracing/raw_tp, tracing/fmod_ret, >> and freplace programs such that these program >> return values can be anything. The purpose are >> two folds: >> 1. to make it explicit about return value expectations >> for these programs in verifier. >> 2. for tracing prog_type, if a future attach type >> is added, the default is -ENOTSUPP which will >> enforce to specify return value ranges explicitly. >> >> Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline") >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- > > Looks good, except a nit below. > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > [...] > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c >> index fa1d8245b925..2d80cce0a28a 100644 >> --- a/kernel/bpf/verifier.c >> +++ b/kernel/bpf/verifier.c >> @@ -7059,6 +7059,24 @@ static int check_return_code(struct bpf_verifier_env *env) >> return 0; >> range = tnum_const(0); >> break; >> + case BPF_PROG_TYPE_TRACING: >> + switch ((env->prog->expected_attach_type)) { > > nit: extra pair of ()? Sorry about this. Not sure whether it is worthwhile to send another revision. Please let me know if another revision is needed. > > >> + case BPF_TRACE_FENTRY: >> + case BPF_TRACE_FEXIT: >> + range = tnum_const(0); >> + break; >> + case BPF_TRACE_RAW_TP: >> + case BPF_MODIFY_RETURN: >> + return 0; >> + default: >> + return -ENOTSUPP; >> + } >> + >> + break; >> + case BPF_PROG_TYPE_EXT: >> + /* freplace program can return anything as its return value >> + * depends on the to-be-replaced kernel func or bpf program. >> + */ >> default: >> return 0; >> } >> -- >> 2.24.1 >>
On Thu, May 14, 2020 at 8:01 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 5/13/20 11:14 PM, Andrii Nakryiko wrote: > > On Wed, May 13, 2020 at 10:32 PM Yonghong Song <yhs@fb.com> wrote: > >> > >> Currently, tracing/fentry and tracing/fexit prog > >> return values are not enforced. In trampoline codes, > >> the fentry/fexit prog return values are ignored. > >> Let us enforce it to be 0 to avoid confusion and > >> allows potential future extension. > >> > >> This patch also explicitly added return value > >> checking for tracing/raw_tp, tracing/fmod_ret, > >> and freplace programs such that these program > >> return values can be anything. The purpose are > >> two folds: > >> 1. to make it explicit about return value expectations > >> for these programs in verifier. > >> 2. for tracing prog_type, if a future attach type > >> is added, the default is -ENOTSUPP which will > >> enforce to specify return value ranges explicitly. > >> > >> Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline") > >> Signed-off-by: Yonghong Song <yhs@fb.com> > >> --- > > > > Looks good, except a nit below. > > > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > > > [...] > > > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > >> index fa1d8245b925..2d80cce0a28a 100644 > >> --- a/kernel/bpf/verifier.c > >> +++ b/kernel/bpf/verifier.c > >> @@ -7059,6 +7059,24 @@ static int check_return_code(struct bpf_verifier_env *env) > >> return 0; > >> range = tnum_const(0); > >> break; > >> + case BPF_PROG_TYPE_TRACING: > >> + switch ((env->prog->expected_attach_type)) { > > > > nit: extra pair of ()? > > Sorry about this. Not sure whether it is worthwhile to send another > revision. Please let me know if another revision is needed. Applied to bpf tree with that typo fixed. Thanks
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index fa1d8245b925..2d80cce0a28a 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -7059,6 +7059,24 @@ static int check_return_code(struct bpf_verifier_env *env) return 0; range = tnum_const(0); break; + case BPF_PROG_TYPE_TRACING: + switch ((env->prog->expected_attach_type)) { + case BPF_TRACE_FENTRY: + case BPF_TRACE_FEXIT: + range = tnum_const(0); + break; + case BPF_TRACE_RAW_TP: + case BPF_MODIFY_RETURN: + return 0; + default: + return -ENOTSUPP; + } + + break; + case BPF_PROG_TYPE_EXT: + /* freplace program can return anything as its return value + * depends on the to-be-replaced kernel func or bpf program. + */ default: return 0; }
Currently, tracing/fentry and tracing/fexit prog return values are not enforced. In trampoline codes, the fentry/fexit prog return values are ignored. Let us enforce it to be 0 to avoid confusion and allows potential future extension. This patch also explicitly added return value checking for tracing/raw_tp, tracing/fmod_ret, and freplace programs such that these program return values can be anything. The purpose are two folds: 1. to make it explicit about return value expectations for these programs in verifier. 2. for tracing prog_type, if a future attach type is added, the default is -ENOTSUPP which will enforce to specify return value ranges explicitly. Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline") Signed-off-by: Yonghong Song <yhs@fb.com> --- kernel/bpf/verifier.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) bpf-next Commit 15d83c4d7cef ("bpf: Allow loading of a bpf iter program") contains the following change: --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -7101,6 +7101,10 @@ static int check_return_code(struct bpf_verifier_env *env) return 0; range = tnum_const(0); break; + case BPF_PROG_TYPE_TRACING: + if (env->prog->expected_attach_type != BPF_TRACE_ITER) + return 0; + break; default: return 0; } If this patch is accepted, it will have a merge conflict when syncing the change back to net-next/bpf-next, To resolve it, we can change to something like below: case BPF_TRACE_RAW_TP: case BPF_MODIFY_RETURN: return 0; case BPF_TRACE_ITER: break; default: return -ENOTSUPP;