On Wed, Nov 8, 2023 at 10:22 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Fri, 2023-10-27 at 11:13 -0700, Andrii Nakryiko wrote:
> > Similar to kernel-side BPF verifier logic enhancements, use 32-bit
> > subrange knowledge for is_branch_taken() logic in reg_bounds selftests.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > .../selftests/bpf/prog_tests/reg_bounds.c | 19 +++++++++++++++----
> > 1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
> > index ac7354cfe139..330618cc12e7 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
> > @@ -750,16 +750,27 @@ static int reg_state_branch_taken_op(enum num_t t, struct reg_state *x, struct r
> > /* OP_EQ and OP_NE are sign-agnostic */
> > enum num_t tu = t_unsigned(t);
> > enum num_t ts = t_signed(t);
> > - int br_u, br_s;
> > + int br_u, br_s, br;
> >
> > br_u = range_branch_taken_op(tu, x->r[tu], y->r[tu], op);
> > br_s = range_branch_taken_op(ts, x->r[ts], y->r[ts], op);
> >
> > if (br_u >= 0 && br_s >= 0 && br_u != br_s)
> > ASSERT_FALSE(true, "branch taken inconsistency!\n");
> > - if (br_u >= 0)
> > - return br_u;
> > - return br_s;
> > +
> > + /* if 64-bit ranges are indecisive, use 32-bit subranges to
> > + * eliminate always/never taken branches, if possible
> > + */
> > + if (br_u == -1 && (t == U64 || t == S64)) {
> > + br = range_branch_taken_op(U32, x->r[U32], y->r[U32], op);
> > + if (br != -1)
> > + return br;
> > + br = range_branch_taken_op(S32, x->r[S32], y->r[S32], op);
> > + if (br != -1)
> > + return br;
>
> I'm not sure that these two checks are consistent with kernel side.
> In kernel:
> - for BPF_JEQ we can derive "won't happen" from u32/s32 ranges;
> - for BPF_JNE we can derive "will happen" from u32/s32 ranges.
>
> But here we seem to accept "will happen" for OP_EQ, which does not
> seem right. E.g. it is possible to have inconclusive upper 32 bits and
> equal lower 32 bits. What am I missing?
I think you are right, I should take into account OP_EQ vs OP_NE here
and the specific value of br. Will update. Nice catch!
