On Thu, Nov 9, 2023 at 2:00 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Wed, 2023-11-08 at 15:11 -0800, Andrii Nakryiko wrote:
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> (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.
yep, I also mentioned conditional bits. Now it's 0x12 or 0x13 for
FALLTHROUGH case, and 0x14 and 0x15 for BRANCH.
>
> > 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.
Agreed. It's a real head-scratcher, how check_cfg() is implemented.
> 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?
I think the intent is to distinguish pure DISCOVERED vs (DISCOVERED |
e) (where e is either FALLTHROUGH or BRANCH). Pure DISCOVERED means
that node is enqueued (in a stack, enstacked ;), but hasn't been
processed yet, while FALLTHROUGH|BRANCH means we are processing it
(but it's not yet fully EXPLORED).
CONDITIONAL is oblivious to FALLTHROUGH|BRANCH, so I put it as 1 so
that this >= check ignores it.
>
> [...]
