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

[PATCH bpf v2 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.

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

[PATCH bpf v2 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.

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

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

[Andrii Nakryiko]

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
>

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

[Yonghong Song]

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

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

[Alexei Starovoitov]

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

[PATCH bpf v2 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 v2 2/2] selftests/bpf: enforce returning 0 for fentry/fexit programs

[Andrii Nakryiko]

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>

[...]