On 04/02/2019 01:58 PM, Paul Chaignon wrote:
> Three checks for verifier bugs were introduced in commit f4d7e40 ("bpf:
> introduce function calls (verification)"). The bugs were reported as
> incorrect programs instead of kernel warnings as the present patch
> implements.
>
> Signed-off-by: Paul Chaignon <paul.chaig...@orange.com>
Thanks for the patch, I think these WARN_ONCE() are a bit out of context though,
meaning it lacks additional information about the program in kernel log once we
actually manage to trigger it which we'd otherwise would potentially have had
with
the verbose() log. And from a program debugging pov, it makes it harder after
this
patch when verification log would suggest that all is fine. Looks like we
already
have a few WARN_ONCE() in verifier, they should probably be converted to
verbose()
as well to be consistent. If we really want to have a kernel warn, then lets
add a
helper macro verbose_and_warn(...) which will trigger a one-time warning, but
keeps
the verbose log intact as well.
> ---
> kernel/bpf/verifier.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 87221fda1321..12499e72b0d5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1145,9 +1145,9 @@ static int mark_reg_read(struct bpf_verifier_env *env,
> if (writes && state->live & REG_LIVE_WRITTEN)
> break;
> if (parent->live & REG_LIVE_DONE) {
> - verbose(env, "verifier BUG type %s var_off %lld off
> %d\n",
> - reg_type_str[parent->type],
> - parent->var_off.value, parent->off);
> + WARN_ONCE(1, "verifier bug type %s var_off %lld off
> %d\n",
> + reg_type_str[parent->type],
> + parent->var_off.value, parent->off);
> return -EFAULT;
> }
> /* ... then we depend on parent's value */
> @@ -2888,15 +2888,15 @@ static int check_func_call(struct bpf_verifier_env
> *env, struct bpf_insn *insn,
> target_insn = *insn_idx + insn->imm;
> subprog = find_subprog(env, target_insn + 1);
> if (subprog < 0) {
> - verbose(env, "verifier bug. No program starts at insn %d\n",
> - target_insn + 1);
> + WARN_ONCE(1, "verifier bug. No program starts at insn %d\n",
> + target_insn + 1);
> return -EFAULT;
> }
>
> caller = state->frame[state->curframe];
> if (state->frame[state->curframe + 1]) {
> - verbose(env, "verifier bug. Frame %d already allocated\n",
> - state->curframe + 1);
> + WARN_ONCE(1, "verifier bug. Frame %d already allocated\n",
> + state->curframe + 1);
> return -EFAULT;
> }
>
>
