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

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(+)

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
>

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