Received-SPF: pass (google.com: domain of linux-kernel+bounces-337-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20231214062434.3565630-1-menglong8.dong@gmail.com>
 <20231214062434.3565630-3-menglong8.dong@gmail.com> <CAEf4BzZdLvwbh_-GNoqD=ghgK+GxgXwUBKP6yQQH=vWMP4Csqw@mail.gmail.com>
In-Reply-To: <CAEf4BzZdLvwbh_-GNoqD=ghgK+GxgXwUBKP6yQQH=vWMP4Csqw@mail.gmail.com>
From: Menglong Dong <menglong8.dong@gmail.com>
Date: Fri, 15 Dec 2023 10:17:18 +0800
Message-ID: <CADxym3Zv+cbQApEFgRVYj1p9B9i8Hyj6V4Cm5etA8dgWP2Vwqw@mail.gmail.com>
Subject: Re: [PATCH bpf-next v3 2/2] selftests/bpf: activate the OP_NE login
 in range_cond()
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: andrii@kernel.org, eddyz87@gmail.com, yonghong.song@linux.dev, 
	ast@kernel.org, daniel@iogearbox.net, john.fastabend@gmail.com, 
	martin.lau@linux.dev, song@kernel.org, kpsingh@kernel.org, sdf@google.com, 
	haoluo@google.com, jolsa@kernel.org, bpf@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Dec 15, 2023 at 7:24=E2=80=AFAM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Dec 13, 2023 at 10:28=E2=80=AFPM Menglong Dong <menglong8.dong@gm=
ail.com> wrote:
> >
> > The edge range checking for the registers is supported by the verifier
> > now, so we can activate the extended login in
> > tools/testing/selftests/bpf/prog_tests/reg_bounds.c/range_cond() to tes=
t
> > such logic.
> >
> > Besides, I added some cases to the "crafted_cases" array for this logic=
.
> > These cases are mainly used to test the edge of the src reg and dst reg=
.
> >
> > Signed-off-by: Menglong Dong <menglong8.dong@gmail.com>
> > ---
> > v2:
> > - add some cases to the "crafted_cases"
> > ---
> >  .../selftests/bpf/prog_tests/reg_bounds.c     | 25 ++++++++++++++-----
> >  1 file changed, 19 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c b/tool=
s/testing/selftests/bpf/prog_tests/reg_bounds.c
> > index 0c9abd279e18..53b8711cfd2d 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/reg_bounds.c
> > @@ -590,12 +590,7 @@ static void range_cond(enum num_t t, struct range =
x, struct range y,
> >                 *newy =3D range(t, max_t(t, x.a, y.a), min_t(t, x.b, y.=
b));
> >                 break;
> >         case OP_NE:
> > -               /* generic case, can't derive more information */
> > -               *newx =3D range(t, x.a, x.b);
> > -               *newy =3D range(t, y.a, y.b);
> > -               break;
> > -
> > -               /* below extended logic is not supported by verifier ju=
st yet */
> > +               /* below logic is supported by the verifier now */
> >                 if (x.a =3D=3D x.b && x.a =3D=3D y.a) {
> >                         /* X is a constant matching left side of Y */
> >                         *newx =3D range(t, x.a, x.b);
> > @@ -2101,6 +2096,24 @@ static struct subtest_case crafted_cases[] =3D {
> >         {S32, S64, {(u32)(s32)S32_MIN, (u32)(s32)-255}, {(u32)(s32)-2, =
0}},
> >         {S32, S64, {0, 1}, {(u32)(s32)S32_MIN, (u32)(s32)S32_MIN}},
> >         {S32, U32, {(u32)(s32)S32_MIN, (u32)(s32)S32_MIN}, {(u32)(s32)S=
32_MIN, (u32)(s32)S32_MIN}},
> > +
> > +       /* edge overlap testings for BPF_NE */
> > +       {U64, U64, {1, 1}, {1, 0x80000000}},
> > +       {U64, S64, {1, 1}, {1, 0x80000000}},
> > +       {U64, U32, {1, 1}, {1, 0x80000000}},
> > +       {U64, S32, {1, 1}, {1, 0x80000000}},
> > +       {U64, U64, {0x80000000, 0x80000000}, {1, 0x80000000}},
> > +       {U64, S64, {0x80000000, 0x80000000}, {1, 0x80000000}},
> > +       {U64, U32, {0x80000000, 0x80000000}, {1, 0x80000000}},
> > +       {U64, S32, {0x80000000, 0x80000000}, {1, 0x80000000}},
> > +       {U64, U64, {1, 0x80000000}, {1, 1}},
> > +       {U64, S64, {1, 0x80000000}, {1, 1}},
> > +       {U64, U32, {1, 0x80000000}, {1, 1}},
> > +       {U64, S32, {1, 0x80000000}, {1, 1}},
> > +       {U64, U64, {1, 0x80000000}, {0x80000000, 0x80000000}},
> > +       {U64, S64, {1, 0x80000000}, {0x80000000, 0x80000000}},
> > +       {U64, U32, {1, 0x80000000}, {0x80000000, 0x80000000}},
> > +       {U64, S32, {1, 0x80000000}, {0x80000000, 0x80000000}},
>
> JNE and JEQ are sign-agnostic, so there is no need to use both U64 and
> S64 variants for comparison. As for the choice of values. Wouldn't it
> make sense to use really a boundary conditions:
>
> 0, 0xffffffffffffffff, and 0x80000000000000 for 64-bit and
> 0, 0xffffffff, and 0x80000000 for 32-bit? For this one use U32 as the ini=
t type?
>

Yeah, this makes sense.

> BTW, all these cases should be tested with auto-generated tests, so
> please make sure to run
>
> sudo SLOW_TESTS=3D1 ./test_progs -t reg_bounds_gen -j
>
> locally. It will take a bit of time, but should help to get confidence
> in that everything is working and nothing regressed.
>

I have already run the slow testing (it indeed takes some time)
and everything works well. I'll add the test results to the commit
log in the next version too.

Thanks!
Menglong Dong

> >  };
> >
> >  /* Go over crafted hard-coded cases. This is fast, so we do it as part=
 of
> > --
> > 2.39.2
> >