On Wed, May 13, 2020 at 9:46 AM Yonghong Song <yhs@xxxxxx> 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@xxxxxx>
> ---
> 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
>
