On Mon, Aug 12, 2024 at 10:57 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2024-08-12 at 10:50 -0700, Alexei Starovoitov wrote:
> > On Mon, Aug 12, 2024 at 10:47 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> > >
> > > On Mon, 2024-08-12 at 10:44 -0700, Alexei Starovoitov wrote:
> > >
> > > [...]
> > >
> > > > Should we move the check up instead?
> > > >
> > > > if (i >= cur->allocated_stack)
> > > > return false;
> > > >
> > > > Checking it twice looks odd.
> > >
> > > A few checks before that, namely:
> > >
> > > if (!(old->stack[spi].spilled_ptr.live & REG_LIVE_READ)
> > > && exact == NOT_EXACT) {
> > > i += BPF_REG_SIZE - 1;
> > > /* explored state didn't use this */
> > > continue;
> > > }
> > >
> > > if (old->stack[spi].slot_type[i % BPF_REG_SIZE] == STACK_INVALID)
> > > continue;
> > >
> > > if (env->allow_uninit_stack &&
> > > old->stack[spi].slot_type[i % BPF_REG_SIZE] == STACK_MISC)
> > > continue;
> > >
> > > Should be done regardless cur->allocated_stack.
> >
> > Right, but then let's sink old->slot_type != cur->slot_type down?
>
> It does not seem correct to swap the order for these two checks:
>
> if (exact != NOT_EXACT && i < cur->allocated_stack &&
> old->stack[spi].slot_type[i % BPF_REG_SIZE] !=
> cur->stack[spi].slot_type[i % BPF_REG_SIZE])
> return false;
>
> if (!(old->stack[spi].spilled_ptr.live & REG_LIVE_READ)
> && exact == NOT_EXACT) {
> i += BPF_REG_SIZE - 1;
> /* explored state didn't use this */
> continue;
> }
>
> if we do, 'slot_type' won't be checked for 'cur' when 'old' register is not marked live.
I see. This is to compare states in open coded iter loops when liveness
is not propagated yet, right?
Then when comparing for exact states we should probably do:
if (exact != NOT_EXACT &&
(i >= cur->allocated_stack ||
old->stack[spi].slot_type[i % BPF_REG_SIZE] !=
cur->stack[spi].slot_type[i % BPF_REG_SIZE]))
return false;
?
