[PATCH bpf 1/2] bpf: enforce returning 0 for fentry/fexit progs

[PATCH bpf 1/2] bpf: enforce returning 0 for fentry/fexit progs

[Yonghong Song]

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.

Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline")
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 kernel/bpf/verifier.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index fa1d8245b925..17b8448babfe 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7059,6 +7059,13 @@ 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_FENTRY ||
+		    env->prog->expected_attach_type == BPF_TRACE_FEXIT) {
+			range = tnum_const(0);
+			break;
+		}
+		return 0;
 	default:
 		return 0;
 	}
-- 
2.24.1

[PATCH bpf 0/2] bpf: enforce returning 0 for fentry/fexit programs

[Yonghong Song]

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.

Yonghong Song (2):
  bpf: enforce returning 0 for fentry/fexit progs
  selftests/bpf: enforce returning 0 for fentry/fexit programs

 kernel/bpf/verifier.c                             | 7 +++++++
 tools/testing/selftests/bpf/progs/test_overhead.c | 4 ++--
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.24.1

[PATCH bpf 2/2] selftests/bpf: enforce returning 0 for fentry/fexit programs

[Yonghong Song]

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

Re: [PATCH bpf 1/2] bpf: enforce returning 0 for fentry/fexit progs

[Andrii Nakryiko]

On Wed, May 13, 2020 at 9:46 AM 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.
>
> Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline")
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  kernel/bpf/verifier.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fa1d8245b925..17b8448babfe 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7059,6 +7059,13 @@ 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_FENTRY ||
> +                   env->prog->expected_attach_type == BPF_TRACE_FEXIT) {
> +                       range = tnum_const(0);
> +                       break;
> +               }
> +               return 0;


I find such if conditions without explicitly handling "else" case very
error-prone and easy to miss when adding new functionality. Having an
explicit switch with all known cases handled and default failing seems
best. WDYT?

E.g., in this case

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_MODIFY_RETURN:
            break;
        default:
            return -ENOTSUPP;
    }

This way if someone adds new tracing sub-type, they will need to
explicitly decide what to do with exit codes.

>         default:
>                 return 0;
>         }
> --
> 2.24.1
>

Re: [PATCH bpf 1/2] bpf: enforce returning 0 for fentry/fexit progs

[Yonghong Song]

On 5/13/20 11:28 AM, Andrii Nakryiko wrote:
> On Wed, May 13, 2020 at 9:46 AM 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.
>>
>> Fixes: fec56f5890d9 ("bpf: Introduce BPF trampoline")
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   kernel/bpf/verifier.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index fa1d8245b925..17b8448babfe 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -7059,6 +7059,13 @@ 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_FENTRY ||
>> +                   env->prog->expected_attach_type == BPF_TRACE_FEXIT) {
>> +                       range = tnum_const(0);
>> +                       break;
>> +               }
>> +               return 0;
> 
> 
> I find such if conditions without explicitly handling "else" case very
> error-prone and easy to miss when adding new functionality. Having an
> explicit switch with all known cases handled and default failing seems
> best. WDYT?

Make sense. Will send v2.

> 
> E.g., in this case
> 
> 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_MODIFY_RETURN:
>              break;
>          default:
>              return -ENOTSUPP;
>      }
> 
> This way if someone adds new tracing sub-type, they will need to
> explicitly decide what to do with exit codes.
> 
>>          default:
>>                  return 0;
>>          }
>> --
>> 2.24.1
>>