* [PATCH bpf v2 0/2] bpf: enforce returning 0 for fentry/fexit programs
@ 2020-05-14 5:32 Yonghong Song
2020-05-14 5:32 ` [PATCH bpf v2 1/2] bpf: enforce returning 0 for fentry/fexit progs Yonghong Song
2020-05-14 5:32 ` [PATCH bpf v2 2/2] selftests/bpf: enforce returning 0 for fentry/fexit programs Yonghong Song
0 siblings, 2 replies; 7+ messages in thread
From: Yonghong Song @ 2020-05-14 5:32 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
Currently, tracing fentry/fexit program return value can be anything,
and these return values are actually ignored by trampoline codes.
Let us force return value to be 0 to avoid confusion and allow
possible future extension. Patch #1 is the kernel change and
Patch #2 fixed the selftest.
Changelog:
v1 -> v2:
- explicitly specify expected return value ranges for all
attach types of the tracing programs. Any unspecified attach
type will return an error. This will force any future
tracing attach_type to be explicit about its return value
range (Andrii).
Yonghong Song (2):
bpf: enforce returning 0 for fentry/fexit progs
selftests/bpf: enforce returning 0 for fentry/fexit programs
kernel/bpf/verifier.c | 18 ++++++++++++++++++
.../selftests/bpf/progs/test_overhead.c | 4 ++--
2 files changed, 20 insertions(+), 2 deletions(-)
--
2.24.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf v2 1/2] bpf: enforce returning 0 for fentry/fexit progs
2020-05-14 5:32 [PATCH bpf v2 0/2] bpf: enforce returning 0 for fentry/fexit programs Yonghong Song
@ 2020-05-14 5:32 ` Yonghong Song
2020-05-14 6:14 ` Andrii Nakryiko
2020-05-14 5:32 ` [PATCH bpf v2 2/2] selftests/bpf: enforce returning 0 for fentry/fexit programs Yonghong Song
1 sibling, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2020-05-14 5:32 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
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;
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;
}
--
2.24.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf v2 2/2] selftests/bpf: enforce returning 0 for fentry/fexit programs
2020-05-14 5:32 [PATCH bpf v2 0/2] bpf: enforce returning 0 for fentry/fexit programs Yonghong Song
2020-05-14 5:32 ` [PATCH bpf v2 1/2] bpf: enforce returning 0 for fentry/fexit progs Yonghong Song
@ 2020-05-14 5:32 ` Yonghong Song
2020-05-14 6:14 ` Andrii Nakryiko
1 sibling, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2020-05-14 5:32 UTC (permalink / raw)
To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
There are a few fentry/fexit programs returning non-0.
The tests with these programs will break with the previous
patch which enfoced return-0 rules. Fix them properly.
Fixes: ac065870d928 ("selftests/bpf: Add BPF_PROG, BPF_KPROBE, and BPF_KRETPROBE macros")
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/testing/selftests/bpf/progs/test_overhead.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/test_overhead.c b/tools/testing/selftests/bpf/progs/test_overhead.c
index 56a50b25cd33..abb7344b531f 100644
--- a/tools/testing/selftests/bpf/progs/test_overhead.c
+++ b/tools/testing/selftests/bpf/progs/test_overhead.c
@@ -30,13 +30,13 @@ int prog3(struct bpf_raw_tracepoint_args *ctx)
SEC("fentry/__set_task_comm")
int BPF_PROG(prog4, struct task_struct *tsk, const char *buf, bool exec)
{
- return !tsk;
+ return 0;
}
SEC("fexit/__set_task_comm")
int BPF_PROG(prog5, struct task_struct *tsk, const char *buf, bool exec)
{
- return !tsk;
+ return 0;
}
char _license[] SEC("license") = "GPL";
--
2.24.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf v2 1/2] bpf: enforce returning 0 for fentry/fexit progs
2020-05-14 5:32 ` [PATCH bpf v2 1/2] bpf: enforce returning 0 for fentry/fexit progs Yonghong Song
@ 2020-05-14 6:14 ` Andrii Nakryiko
2020-05-14 14:58 ` Yonghong Song
0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2020-05-14 6:14 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf v2 2/2] selftests/bpf: enforce returning 0 for fentry/fexit programs
2020-05-14 5:32 ` [PATCH bpf v2 2/2] selftests/bpf: enforce returning 0 for fentry/fexit programs Yonghong Song
@ 2020-05-14 6:14 ` Andrii Nakryiko
0 siblings, 0 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2020-05-14 6:14 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team
On Wed, May 13, 2020 at 10:33 PM Yonghong Song <yhs@fb.com> wrote:
>
> There are a few fentry/fexit programs returning non-0.
> The tests with these programs will break with the previous
> patch which enfoced return-0 rules. Fix them properly.
>
> Fixes: ac065870d928 ("selftests/bpf: Add BPF_PROG, BPF_KPROBE, and BPF_KRETPROBE macros")
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
Acked-by: Andrii Nakryiko <andriin@fb.com>
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf v2 1/2] bpf: enforce returning 0 for fentry/fexit progs
2020-05-14 6:14 ` Andrii Nakryiko
@ 2020-05-14 14:58 ` Yonghong Song
2020-05-14 19:57 ` Alexei Starovoitov
0 siblings, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2020-05-14 14:58 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team
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
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf v2 1/2] bpf: enforce returning 0 for fentry/fexit progs
2020-05-14 14:58 ` Yonghong Song
@ 2020-05-14 19:57 ` Alexei Starovoitov
0 siblings, 0 replies; 7+ messages in thread
From: Alexei Starovoitov @ 2020-05-14 19:57 UTC (permalink / raw)
To: Yonghong Song
Cc: Andrii Nakryiko, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-14 19:57 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-14 5:32 [PATCH bpf v2 0/2] bpf: enforce returning 0 for fentry/fexit programs Yonghong Song
2020-05-14 5:32 ` [PATCH bpf v2 1/2] bpf: enforce returning 0 for fentry/fexit progs Yonghong Song
2020-05-14 6:14 ` Andrii Nakryiko
2020-05-14 14:58 ` Yonghong Song
2020-05-14 19:57 ` Alexei Starovoitov
2020-05-14 5:32 ` [PATCH bpf v2 2/2] selftests/bpf: enforce returning 0 for fentry/fexit programs Yonghong Song
2020-05-14 6:14 ` Andrii Nakryiko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.