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

[Yonghong Song <yhs@fb.com>]

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