On 5/13/20 11:28 AM, Andrii Nakryiko wrote:
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?
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
