On Thu, May 15, 2025 at 4:45 AM Paul Chaignon <paul.chaignon@xxxxxxxxx> wrote:
>
> Throughout the verifier's logic, there are multiple checks for
> inconsistent states that should never happen and would indicate a
> verifier bug. These bugs are typically logged in the verifier logs and
> sometimes preceded by a WARN_ONCE.
>
> This patch reworks these checks to consistently emit a verifier log AND
> a warning when CONFIG_DEBUG_KERNEL is enabled. The consistent use of
> WARN_ONCE should help fuzzers (ex. syzkaller) expose any situation
> where they are actually able to reach one of those buggy verifier
> states.
>
> Signed-off-by: Paul Chaignon <paul.chaignon@xxxxxxxxx>
> ---
> Changes in v3:
> - Introduce and use verifier_bug_if, as suggested by Andrii.
> Changes in v2:
> - Introduce a new BPF_WARN_ONCE macro, with WARN_ONCE conditioned on
> CONFIG_DEBUG_KERNEL, as per reviews.
> - Use the new helper function for verifier bugs missed in v1,
> particularly around backtracking.
>
> include/linux/bpf.h | 6 ++
> include/linux/bpf_verifier.h | 10 +++
> kernel/bpf/btf.c | 4 +-
> kernel/bpf/verifier.c | 121 ++++++++++++++++-------------------
> 4 files changed, 74 insertions(+), 67 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 83c56f40842b..5b25d278409b 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -346,6 +346,12 @@ static inline const char *btf_field_type_name(enum btf_field_type type)
> }
> }
>
> +#if IS_ENABLED(CONFIG_DEBUG_KERNEL)
> +#define BPF_WARN_ONCE(cond, format...) WARN_ONCE(cond, format)
> +#else
> +#define BPF_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
> +#endif
> +
> static inline u32 btf_field_type_size(enum btf_field_type type)
> {
> switch (type) {
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index cedd66867ecf..c3fd6905793a 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -839,6 +839,16 @@ __printf(3, 4) void verbose_linfo(struct bpf_verifier_env *env,
> u32 insn_off,
> const char *prefix_fmt, ...);
>
> +#define verifier_bug_if(cond, env, fmt, args...) \
> + ({ \
> + if (unlikely(cond)) { \
> + BPF_WARN_ONCE(1, "verifier bug: " fmt, ##args); \
> + bpf_log(&env->log, "verifier bug: " fmt, ##args); \
> + } \
> + (cond); \
> + })
> +#define verifier_bug(env, fmt, args...) verifier_bug_if(1, env, fmt, ##args)
>
(cond) shouldn't be double evaluated.
Also let's stringify condition in verifier_bug_if().
The messages will be easier for humans to parse.
Especially when code evolves and line numbers don't correspond
to new code.
Then some text can be reduced, removed or reworded.
Like:
- if
(WARN_ON_ONCE(env->cur_state->loop_entry)) {
- verbose(env, "verifier
bug: env->cur_state->loop_entry != NULL\n");
+ if
(verifier_bug_if(env->cur_state->loop_entry, env,
+
"cur_state->loop_entry not null\n"))
can be:
if (verifier_bug_if(env->cur_state->loop_entry, env, "broken loop detection"))
