Re: [PATCH bpf-next 3/4] bpf: fix control-flow graph checking in privileged mode

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2023-11-08 at 15:11 -0800, Andrii Nakryiko wrote:

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
(given that I understood check in push_insn correctly).

[...]

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index edca7f1ad335..35065cae98b7 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -15433,8 +15433,9 @@ static int check_return_code(struct bpf_verifier_env *env, int regno)

Nitpick: there is a comment right above this enum which has to be
         updated after changes to the enum.

>  enum {
>  	DISCOVERED = 0x10,
>  	EXPLORED = 0x20,
> -	FALLTHROUGH = 1,
> -	BRANCH = 2,
> +	CONDITIONAL = 0x01,
> +	FALLTHROUGH = 0x02,
> +	BRANCH = 0x04,
>  };
>  
>  static void mark_prune_point(struct bpf_verifier_env *env, int idx)
> @@ -15468,16 +15469,15 @@ enum {
>   * w - next instruction
>   * e - edge
>   */
> -static int push_insn(int t, int w, int e, struct bpf_verifier_env *env,
> -		     bool loop_ok)
> +static int push_insn(int t, int w, int e, struct bpf_verifier_env *env)
>  {
>  	int *insn_stack = env->cfg.insn_stack;
>  	int *insn_state = env->cfg.insn_state;
>  
> -	if (e == FALLTHROUGH && insn_state[t] >= (DISCOVERED | FALLTHROUGH))
> +	if ((e & FALLTHROUGH) && insn_state[t] >= (DISCOVERED | FALLTHROUGH))
>  		return DONE_EXPLORING;

This not related to your changes, but '>=' here is so confusing.
The intent is to check:
  ((insn_state[t] & (DISCOVERED | FALLTHROUGH)) == (DISCOVERED | FALLTHROUGH))
i.e. DONE_EXPLORING if fall-through branch of 't' had been explored already,
right?

[...]