linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v3 0/2] bpf: support to track BPF_JNE
@ 2023-12-14  6:24 Menglong Dong
  2023-12-14  6:24 ` [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs Menglong Dong
  2023-12-14  6:24 ` [PATCH bpf-next v3 2/2] selftests/bpf: activate the OP_NE login in range_cond() Menglong Dong
  0 siblings, 2 replies; 10+ messages in thread
From: Menglong Dong @ 2023-12-14  6:24 UTC (permalink / raw)
  To: andrii, eddyz87, yonghong.song
  Cc: ast, daniel, john.fastabend, martin.lau, song, kpsingh, sdf,
	haoluo, jolsa, bpf, linux-kernel, Menglong Dong

For now, the reg bounds is not handled for BPF_JNE case, which can cause
the failure of following case:

  /* The type of "a" is u16 */
  if (a > 0 && a < 100) {
    /* the range of the register for a is [0, 99], not [1, 99],
     * and will cause the following error:
     *
     *   invalid zero-sized read
     *
     * as a can be 0.
     */
    bpf_skb_store_bytes(skb, xx, xx, a, 0);
  }

In the code above, "a > 0" will be compiled to "jmp xxx if a == 0". In the
TRUE branch, the dst_reg will be marked as known to 0. However, in the
fallthrough(FALSE) branch, the dst_reg will not be handled, which makes
the [min, max] for a is [0, 99], not [1, 99].

In the 1st patch, we reduce the range of the dst reg if the src reg is a
const and is exactly the edge of the dst reg For BPF_JNE.

In the 2nd patch, we just activate the test case for this logic in
range_cond(), which is committed by Andrii in the
commit 8863238993e2 ("selftests/bpf: BPF register range bounds tester").

Changes since v2:
- fix a typo in the subject of the 1st patch
- add some comments to the 1st patch, as Eduard advised
- add some cases to the "crafted_cases"

Changes since v1:
- simplify the code in the 1st patch
- introduce the 2nd patch for the testing

Menglong Dong (2):
  bpf: make the verifier tracks the "not equal" for regs
  selftests/bpf: activate the OP_NE login in range_cond()

 kernel/bpf/verifier.c                         | 38 ++++++++++++++++++-
 .../selftests/bpf/prog_tests/reg_bounds.c     | 25 +++++++++---
 2 files changed, 56 insertions(+), 7 deletions(-)

-- 
2.39.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs
  2023-12-14  6:24 [PATCH bpf-next v3 0/2] bpf: support to track BPF_JNE Menglong Dong
@ 2023-12-14  6:24 ` Menglong Dong
  2023-12-14 10:00   ` Shung-Hsi Yu
                     ` (2 more replies)
  2023-12-14  6:24 ` [PATCH bpf-next v3 2/2] selftests/bpf: activate the OP_NE login in range_cond() Menglong Dong
  1 sibling, 3 replies; 10+ messages in thread
From: Menglong Dong @ 2023-12-14  6:24 UTC (permalink / raw)
  To: andrii, eddyz87, yonghong.song
  Cc: ast, daniel, john.fastabend, martin.lau, song, kpsingh, sdf,
	haoluo, jolsa, bpf, linux-kernel, Menglong Dong

We can derive some new information for BPF_JNE in regs_refine_cond_op().
Take following code for example:

  /* The type of "a" is u16 */
  if (a > 0 && a < 100) {
    /* the range of the register for a is [0, 99], not [1, 99],
     * and will cause the following error:
     *
     *   invalid zero-sized read
     *
     * as a can be 0.
     */
    bpf_skb_store_bytes(skb, xx, xx, a, 0);
  }

In the code above, "a > 0" will be compiled to "jmp xxx if a == 0". In the
TRUE branch, the dst_reg will be marked as known to 0. However, in the
fallthrough(FALSE) branch, the dst_reg will not be handled, which makes
the [min, max] for a is [0, 99], not [1, 99].

For BPF_JNE, we can reduce the range of the dst reg if the src reg is a
const and is exactly the edge of the dst reg.

Signed-off-by: Menglong Dong <menglong8.dong@gmail.com>
---
v2:
- fix a typo in the subject
- add some comments, as Eduard advised
---
 kernel/bpf/verifier.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 727a59e4a647..9b1932e51823 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -14332,7 +14332,43 @@ static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state
 		}
 		break;
 	case BPF_JNE:
-		/* we don't derive any new information for inequality yet */
+		if (!is_reg_const(reg2, is_jmp32))
+			swap(reg1, reg2);
+		if (!is_reg_const(reg2, is_jmp32))
+			break;
+
+		/* try to recompute the bound of reg1 if reg2 is a const and
+		 * is exactly the edge of reg1.
+		 */
+		val = reg_const_value(reg2, is_jmp32);
+		if (is_jmp32) {
+			/* u32_min_value is not equal to 0xffffffff at this point,
+			 * because otherwise u32_max_value is 0xffffffff as well,
+			 * in such a case both reg1 and reg2 would be constants,
+			 * jump would be predicted and reg_set_min_max() won't
+			 * be called.
+			 *
+			 * Same reasoning works for all {u,s}{min,max}{32,64} cases
+			 * below.
+			 */
+			if (reg1->u32_min_value == (u32)val)
+				reg1->u32_min_value++;
+			if (reg1->u32_max_value == (u32)val)
+				reg1->u32_max_value--;
+			if (reg1->s32_min_value == (s32)val)
+				reg1->s32_min_value++;
+			if (reg1->s32_max_value == (s32)val)
+				reg1->s32_max_value--;
+		} else {
+			if (reg1->umin_value == (u64)val)
+				reg1->umin_value++;
+			if (reg1->umax_value == (u64)val)
+				reg1->umax_value--;
+			if (reg1->smin_value == (s64)val)
+				reg1->smin_value++;
+			if (reg1->smax_value == (s64)val)
+				reg1->smax_value--;
+		}
 		break;
 	case BPF_JSET:
 		if (!is_reg_const(reg2, is_jmp32))
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next v3 2/2] selftests/bpf: activate the OP_NE login in range_cond()
  2023-12-14  6:24 [PATCH bpf-next v3 0/2] bpf: support to track BPF_JNE Menglong Dong
  2023-12-14  6:24 ` [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs Menglong Dong
@ 2023-12-14  6:24 ` Menglong Dong
  2023-12-14 23:24   ` Andrii Nakryiko
  1 sibling, 1 reply; 10+ messages in thread
From: Menglong Dong @ 2023-12-14  6:24 UTC (permalink / raw)
  To: andrii, eddyz87, yonghong.song
  Cc: ast, daniel, john.fastabend, martin.lau, song, kpsingh, sdf,
	haoluo, jolsa, bpf, linux-kernel, Menglong Dong

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 test
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/tools/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 = 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 = range(t, x.a, x.b);
-		*newy = range(t, y.a, y.b);
-		break;
-
-		/* below extended logic is not supported by verifier just yet */
+		/* below logic is supported by the verifier now */
 		if (x.a == x.b && x.a == y.a) {
 			/* X is a constant matching left side of Y */
 			*newx = range(t, x.a, x.b);
@@ -2101,6 +2096,24 @@ static struct subtest_case crafted_cases[] = {
 	{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)S32_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}},
 };
 
 /* Go over crafted hard-coded cases. This is fast, so we do it as part of
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs
  2023-12-14  6:24 ` [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs Menglong Dong
@ 2023-12-14 10:00   ` Shung-Hsi Yu
  2023-12-14 13:49   ` Alexei Starovoitov
  2023-12-14 23:19   ` Andrii Nakryiko
  2 siblings, 0 replies; 10+ messages in thread
From: Shung-Hsi Yu @ 2023-12-14 10:00 UTC (permalink / raw)
  To: Menglong Dong
  Cc: andrii, eddyz87, yonghong.song, ast, daniel, john.fastabend,
	martin.lau, song, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel

On Thu, Dec 14, 2023 at 02:24:33PM +0800, Menglong Dong wrote:
> We can derive some new information for BPF_JNE in regs_refine_cond_op().
> Take following code for example:
> 
>   /* The type of "a" is u16 */
>   if (a > 0 && a < 100) {
>     /* the range of the register for a is [0, 99], not [1, 99],
>      * and will cause the following error:
>      *
>      *   invalid zero-sized read
>      *
>      * as a can be 0.
>      */
>     bpf_skb_store_bytes(skb, xx, xx, a, 0);
>   }
> 
> In the code above, "a > 0" will be compiled to "jmp xxx if a == 0". In the
> TRUE branch, the dst_reg will be marked as known to 0. However, in the
> fallthrough(FALSE) branch, the dst_reg will not be handled, which makes
> the [min, max] for a is [0, 99], not [1, 99].
> 
> For BPF_JNE, we can reduce the range of the dst reg if the src reg is a
> const and is exactly the edge of the dst reg.
> 
> Signed-off-by: Menglong Dong <menglong8.dong@gmail.com>

Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs
  2023-12-14  6:24 ` [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs Menglong Dong
  2023-12-14 10:00   ` Shung-Hsi Yu
@ 2023-12-14 13:49   ` Alexei Starovoitov
  2023-12-14 14:07     ` Menglong Dong
  2023-12-14 23:19   ` Andrii Nakryiko
  2 siblings, 1 reply; 10+ messages in thread
From: Alexei Starovoitov @ 2023-12-14 13:49 UTC (permalink / raw)
  To: Menglong Dong
  Cc: Andrii Nakryiko, Eddy Z, Yonghong Song, Alexei Starovoitov,
	Daniel Borkmann, John Fastabend, Martin KaFai Lau, Song Liu,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, LKML

On Wed, Dec 13, 2023 at 10:28 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
>
> We can derive some new information for BPF_JNE in regs_refine_cond_op().
> Take following code for example:
>
>   /* The type of "a" is u16 */
>   if (a > 0 && a < 100) {
>     /* the range of the register for a is [0, 99], not [1, 99],
>      * and will cause the following error:
>      *
>      *   invalid zero-sized read
>      *
>      * as a can be 0.
>      */
>     bpf_skb_store_bytes(skb, xx, xx, a, 0);
>   }

Please craft a selftest from above with inline asm
(C might not work as compiler might optimize it)

Also we call:
        /* fallthrough (FALSE) branch */
        regs_refine_cond_op(false_reg1, false_reg2,
rev_opcode(opcode), is_jmp32);
        /* jump (TRUE) branch */
        regs_refine_cond_op(true_reg1, true_reg2, opcode, is_jmp32);

so despite BPF_JNE is not handled explicitly it still should have
caught above due to rev_opcode() ?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs
  2023-12-14 13:49   ` Alexei Starovoitov
@ 2023-12-14 14:07     ` Menglong Dong
  2023-12-14 16:10       ` Alexei Starovoitov
  0 siblings, 1 reply; 10+ messages in thread
From: Menglong Dong @ 2023-12-14 14:07 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Andrii Nakryiko, Eddy Z, Yonghong Song, Alexei Starovoitov,
	Daniel Borkmann, John Fastabend, Martin KaFai Lau, Song Liu,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, LKML

Hello,

On Thu, Dec 14, 2023 at 9:49 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Dec 13, 2023 at 10:28 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
> >
> > We can derive some new information for BPF_JNE in regs_refine_cond_op().
> > Take following code for example:
> >
> >   /* The type of "a" is u16 */
> >   if (a > 0 && a < 100) {
> >     /* the range of the register for a is [0, 99], not [1, 99],
> >      * and will cause the following error:
> >      *
> >      *   invalid zero-sized read
> >      *
> >      * as a can be 0.
> >      */
> >     bpf_skb_store_bytes(skb, xx, xx, a, 0);
> >   }
>
> Please craft a selftest from above with inline asm
> (C might not work as compiler might optimize it)

Okay! Should I add this selftests to reg_bounds as a subtest,
or add a "verifier_reg_edge.c" for verifier testing?

> Also we call:
>         /* fallthrough (FALSE) branch */
>         regs_refine_cond_op(false_reg1, false_reg2,
> rev_opcode(opcode), is_jmp32);
>         /* jump (TRUE) branch */
>         regs_refine_cond_op(true_reg1, true_reg2, opcode, is_jmp32);
>
> so despite BPF_JNE is not handled explicitly it still should have
> caught above due to rev_opcode() ?

Ennn.....I'm a little confused. In this case, the TRUE path is
handled properly, as the opcode is BPF_JEQ; and the FALSE
is not handled properly, as the opcode is rev_opcode(BPF_JEQ),
which is BPF_JNE. And the bpf_skb_store_bytes() will be called
in the FALSE path. The origin state of false_reg* should be the same
as true_reg*.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs
  2023-12-14 14:07     ` Menglong Dong
@ 2023-12-14 16:10       ` Alexei Starovoitov
  0 siblings, 0 replies; 10+ messages in thread
From: Alexei Starovoitov @ 2023-12-14 16:10 UTC (permalink / raw)
  To: Menglong Dong
  Cc: Andrii Nakryiko, Eddy Z, Yonghong Song, Alexei Starovoitov,
	Daniel Borkmann, John Fastabend, Martin KaFai Lau, Song Liu,
	KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, bpf, LKML

On Thu, Dec 14, 2023 at 6:07 AM Menglong Dong <menglong8.dong@gmail.com> wrote:
>
> Hello,
>
> On Thu, Dec 14, 2023 at 9:49 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Wed, Dec 13, 2023 at 10:28 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
> > >
> > > We can derive some new information for BPF_JNE in regs_refine_cond_op().
> > > Take following code for example:
> > >
> > >   /* The type of "a" is u16 */
> > >   if (a > 0 && a < 100) {
> > >     /* the range of the register for a is [0, 99], not [1, 99],
> > >      * and will cause the following error:
> > >      *
> > >      *   invalid zero-sized read
> > >      *
> > >      * as a can be 0.
> > >      */
> > >     bpf_skb_store_bytes(skb, xx, xx, a, 0);
> > >   }
> >
> > Please craft a selftest from above with inline asm
> > (C might not work as compiler might optimize it)
>
> Okay! Should I add this selftests to reg_bounds as a subtest,
> or add a "verifier_reg_edge.c" for verifier testing?

reg_bounds is for automated.
I think it will fit fine in the existing progs/verifier_bounds.c

>
> > Also we call:
> >         /* fallthrough (FALSE) branch */
> >         regs_refine_cond_op(false_reg1, false_reg2,
> > rev_opcode(opcode), is_jmp32);
> >         /* jump (TRUE) branch */
> >         regs_refine_cond_op(true_reg1, true_reg2, opcode, is_jmp32);
> >
> > so despite BPF_JNE is not handled explicitly it still should have
> > caught above due to rev_opcode() ?
>
> Ennn.....I'm a little confused. In this case, the TRUE path is
> handled properly, as the opcode is BPF_JEQ; and the FALSE
> is not handled properly, as the opcode is rev_opcode(BPF_JEQ),
> which is BPF_JNE. And the bpf_skb_store_bytes() will be called
> in the FALSE path. The origin state of false_reg* should be the same
> as true_reg*.

Ahh. I see.
It wasn't clear how 'a > 0 && a < 100' got to be JNE after optimizations.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs
  2023-12-14  6:24 ` [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs Menglong Dong
  2023-12-14 10:00   ` Shung-Hsi Yu
  2023-12-14 13:49   ` Alexei Starovoitov
@ 2023-12-14 23:19   ` Andrii Nakryiko
  2 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2023-12-14 23:19 UTC (permalink / raw)
  To: Menglong Dong
  Cc: andrii, eddyz87, yonghong.song, ast, daniel, john.fastabend,
	martin.lau, song, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel

On Wed, Dec 13, 2023 at 10:28 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
>
> We can derive some new information for BPF_JNE in regs_refine_cond_op().
> Take following code for example:
>
>   /* The type of "a" is u16 */
>   if (a > 0 && a < 100) {
>     /* the range of the register for a is [0, 99], not [1, 99],
>      * and will cause the following error:
>      *
>      *   invalid zero-sized read
>      *
>      * as a can be 0.
>      */
>     bpf_skb_store_bytes(skb, xx, xx, a, 0);
>   }
>
> In the code above, "a > 0" will be compiled to "jmp xxx if a == 0". In the
> TRUE branch, the dst_reg will be marked as known to 0. However, in the
> fallthrough(FALSE) branch, the dst_reg will not be handled, which makes
> the [min, max] for a is [0, 99], not [1, 99].
>
> For BPF_JNE, we can reduce the range of the dst reg if the src reg is a
> const and is exactly the edge of the dst reg.
>
> Signed-off-by: Menglong Dong <menglong8.dong@gmail.com>
> ---
> v2:
> - fix a typo in the subject
> - add some comments, as Eduard advised
> ---
>  kernel/bpf/verifier.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>

The logic looks good

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 727a59e4a647..9b1932e51823 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -14332,7 +14332,43 @@ static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state
>                 }
>                 break;
>         case BPF_JNE:
> -               /* we don't derive any new information for inequality yet */
> +               if (!is_reg_const(reg2, is_jmp32))
> +                       swap(reg1, reg2);
> +               if (!is_reg_const(reg2, is_jmp32))
> +                       break;
> +
> +               /* try to recompute the bound of reg1 if reg2 is a const and
> +                * is exactly the edge of reg1.
> +                */
> +               val = reg_const_value(reg2, is_jmp32);
> +               if (is_jmp32) {
> +                       /* u32_min_value is not equal to 0xffffffff at this point,
> +                        * because otherwise u32_max_value is 0xffffffff as well,
> +                        * in such a case both reg1 and reg2 would be constants,
> +                        * jump would be predicted and reg_set_min_max() won't
> +                        * be called.
> +                        *
> +                        * Same reasoning works for all {u,s}{min,max}{32,64} cases
> +                        * below.
> +                        */
> +                       if (reg1->u32_min_value == (u32)val)
> +                               reg1->u32_min_value++;
> +                       if (reg1->u32_max_value == (u32)val)
> +                               reg1->u32_max_value--;
> +                       if (reg1->s32_min_value == (s32)val)
> +                               reg1->s32_min_value++;
> +                       if (reg1->s32_max_value == (s32)val)
> +                               reg1->s32_max_value--;
> +               } else {
> +                       if (reg1->umin_value == (u64)val)
> +                               reg1->umin_value++;
> +                       if (reg1->umax_value == (u64)val)
> +                               reg1->umax_value--;
> +                       if (reg1->smin_value == (s64)val)
> +                               reg1->smin_value++;
> +                       if (reg1->smax_value == (s64)val)
> +                               reg1->smax_value--;
> +               }
>                 break;
>         case BPF_JSET:
>                 if (!is_reg_const(reg2, is_jmp32))
> --
> 2.39.2
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 2/2] selftests/bpf: activate the OP_NE login in range_cond()
  2023-12-14  6:24 ` [PATCH bpf-next v3 2/2] selftests/bpf: activate the OP_NE login in range_cond() Menglong Dong
@ 2023-12-14 23:24   ` Andrii Nakryiko
  2023-12-15  2:17     ` Menglong Dong
  0 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2023-12-14 23:24 UTC (permalink / raw)
  To: Menglong Dong
  Cc: andrii, eddyz87, yonghong.song, ast, daniel, john.fastabend,
	martin.lau, song, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel

On Wed, Dec 13, 2023 at 10:28 PM Menglong Dong <menglong8.dong@gmail.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 test
> 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/tools/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 = 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 = range(t, x.a, x.b);
> -               *newy = range(t, y.a, y.b);
> -               break;
> -
> -               /* below extended logic is not supported by verifier just yet */
> +               /* below logic is supported by the verifier now */
>                 if (x.a == x.b && x.a == y.a) {
>                         /* X is a constant matching left side of Y */
>                         *newx = range(t, x.a, x.b);
> @@ -2101,6 +2096,24 @@ static struct subtest_case crafted_cases[] = {
>         {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)S32_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 init type?

BTW, all these cases should be tested with auto-generated tests, so
please make sure to run

sudo SLOW_TESTS=1 ./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.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next v3 2/2] selftests/bpf: activate the OP_NE login in range_cond()
  2023-12-14 23:24   ` Andrii Nakryiko
@ 2023-12-15  2:17     ` Menglong Dong
  0 siblings, 0 replies; 10+ messages in thread
From: Menglong Dong @ 2023-12-15  2:17 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: andrii, eddyz87, yonghong.song, ast, daniel, john.fastabend,
	martin.lau, song, kpsingh, sdf, haoluo, jolsa, bpf, linux-kernel

On Fri, Dec 15, 2023 at 7:24 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Dec 13, 2023 at 10:28 PM Menglong Dong <menglong8.dong@gmail.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 test
> > 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/tools/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 = 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 = range(t, x.a, x.b);
> > -               *newy = range(t, y.a, y.b);
> > -               break;
> > -
> > -               /* below extended logic is not supported by verifier just yet */
> > +               /* below logic is supported by the verifier now */
> >                 if (x.a == x.b && x.a == y.a) {
> >                         /* X is a constant matching left side of Y */
> >                         *newx = range(t, x.a, x.b);
> > @@ -2101,6 +2096,24 @@ static struct subtest_case crafted_cases[] = {
> >         {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)S32_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 init 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=1 ./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
> >

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-12-15  2:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-14  6:24 [PATCH bpf-next v3 0/2] bpf: support to track BPF_JNE Menglong Dong
2023-12-14  6:24 ` [PATCH bpf-next v3 1/2] bpf: make the verifier tracks the "not equal" for regs Menglong Dong
2023-12-14 10:00   ` Shung-Hsi Yu
2023-12-14 13:49   ` Alexei Starovoitov
2023-12-14 14:07     ` Menglong Dong
2023-12-14 16:10       ` Alexei Starovoitov
2023-12-14 23:19   ` Andrii Nakryiko
2023-12-14  6:24 ` [PATCH bpf-next v3 2/2] selftests/bpf: activate the OP_NE login in range_cond() Menglong Dong
2023-12-14 23:24   ` Andrii Nakryiko
2023-12-15  2:17     ` Menglong Dong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).