All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next] bpf: relax verifier restriction on BPF_MOV | BPF_ALU
@ 2018-12-05  9:46 Jiong Wang
  2018-12-05 14:52 ` Edward Cree
  0 siblings, 1 reply; 7+ messages in thread
From: Jiong Wang @ 2018-12-05  9:46 UTC (permalink / raw
  To: ast, daniel; +Cc: netdev, oss-drivers, Jiong Wang

Currently, the destination register is marked as unknown for 32-bit
sub-register move (BPF_MOV | BPF_ALU) whenever the source register type is
SCALAR_VALUE.

This is too conservative that some valid cases will be rejected.
Especially, this may turn a constant scalar value into unknown value that
some runtime optimization could fail. For example, there is the following
insn pattern for test_l4lb_noinline.c when it is compiled by -mattr=+alu32:

  return from callee:
  411: (b4) (u32) r7 = (u32) 0
  412: (54) (u32) r7 &= (u32) 1
  413: (bc) (u32) r0 = (u32) r7
  414: (95) exit

  to caller:
  202: (bc) (u32) r1 = (u32) r0
  203: (b4) (u32) r0 = (u32) 2
  204: (bf) r7 = r9
  205: (15) if r1 == 0x0 goto pc+69
  206: (79) r9 = *(u64 *)(r10 -80)
  207: (71) r1 = *(u8 *)(r9 +16)

The ending sequence in callee is a sub-register move insn which should make
r0 be SCALAR_VALUE 0, however verifier is conservatively marking r0 as
unknown, so r0 is not a constant anymore, instead it is:

   R0=inv(id=0,umax_value=4294967295,var_off=(0x0; 0xffffffff))

Then, after returned to caller, r0 is copied back to r1 inside caller where
r1 is used to compare against constant 0. In the C code logic, the stack
slot (r10 - 80) will only be stored a valid pointer when r1 if not zero for
which case insn 207 will be a valid load. Otherwise no initialize of the
stack slot and insn 207 is invalid.

Now the code pattern above belongs to the path that is initializing r1 to
zero, therefore this path won't initialize the stack slot, but verifier
will smartly skip analyzing the fall through path starting at insn 206
because r1 is expected to be zero that the comparison at insn 205 will be
true. However due to r0 is conservatively marked as unknown in first place
in callee's ending sequence, we have lost the information of r0/r1. The
consequence is verifier will fail to skip the fall through path, and will
de-reference stack slot at (r10 - 80) which is not with a valid spilled
pointer for this path.

This patch relaxed the code marking sub-register move destination. For a
SCALAR_VALUE or pointer value which is allowed to be leaked as scalar
value, it is safe to just copy the value from source, force the value type
into SCALAR_VALUE and then truncate it into 32-bit.

A unit test also included to demonstrate this issue. This test will fail
before this patch.

As this relaxation is potentially giving verifier more accurate
information, bpf c program with reasonable size have been benchmarked,
including those inside kernel bpf selftests and those inside cilium repo.
There is NO processed instruction number regression, either with or without
-mattr=+alu32. And there are some improvements:

  - test_xdp_noinline now passed verification under -mattr=+alu32.
  - a few cililum bpf programs got processed insn number reduced.

Insn processed before/after this patch:

                        default     -mattr=+alu32

Kernel selftest
===
test_xdp.o              371/371      369/369
test_l4lb.o             6345/6345    5623/5623
test_xdp_noinline.o     2971/2971    rejected/2727
test_tcp_estates.o      429/429      430/430

Cilium bpf
===
bpf_lb-DLB_L3.o         2110/2110    1730/1733
bpf_lb-DLB_L4.o         2251/2251    2037/2029
bpf_lb-DUNKNOWN.o       701/701      729/622
bpf_lxc.o               96023/95033  N/A
bpf_netdev.o            7456/7245    N/A
bpf_overlay.o           3096/2898    3085/2947

bpf_lxc.o and bpf_netdev.o compiled by -mattr=+alu32 are rejected by
verifier because of another issue inside verifier on supporting alu32
binary.

One Cilium bpf program could generate several processed insn info, above
number is sum of them.

Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
---
 kernel/bpf/verifier.c                       | 32 ++++++++++++++---------------
 tools/testing/selftests/bpf/test_verifier.c | 13 ++++++++++++
 2 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9584438..ce8a1c3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3583,23 +3583,23 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
 			return err;
 
 		if (BPF_SRC(insn->code) == BPF_X) {
-			if (BPF_CLASS(insn->code) == BPF_ALU64) {
-				/* case: R1 = R2
-				 * copy register state to dest reg
-				 */
-				regs[insn->dst_reg] = regs[insn->src_reg];
-				regs[insn->dst_reg].live |= REG_LIVE_WRITTEN;
-			} else {
-				/* R1 = (u32) R2 */
-				if (is_pointer_value(env, insn->src_reg)) {
-					verbose(env,
-						"R%d partial copy of pointer\n",
-						insn->src_reg);
-					return -EACCES;
-				}
-				mark_reg_unknown(env, regs, insn->dst_reg);
-				coerce_reg_to_size(&regs[insn->dst_reg], 4);
+			u8 dst_reg, src_reg = insn->src_reg;
+
+			/* Reject partial pointer copy on R1 = (u32) R2. */
+			if (BPF_CLASS(insn->code) == BPF_ALU &&
+			    is_pointer_value(env, src_reg)) {
+				verbose(env, "R%d partial copy of pointer\n",
+					src_reg);
+				return -EACCES;
+			}
+			dst_reg = insn->dst_reg;
+			regs[dst_reg] = regs[src_reg];
+			if (BPF_CLASS(insn->code) == BPF_ALU) {
+				/* Update type and range info. */
+				regs[dst_reg].type = SCALAR_VALUE;
+				coerce_reg_to_size(&regs[dst_reg], 4);
 			}
+			regs[dst_reg].live |= REG_LIVE_WRITTEN;
 		} else {
 			/* case: R = imm
 			 * remember the value we stored into this reg
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 17021d2..18d0b7f 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -2936,6 +2936,19 @@ static struct bpf_test tests[] = {
 		.result = ACCEPT,
 	},
 	{
+		"alu32: mov u32 const",
+		.insns = {
+			BPF_MOV32_IMM(BPF_REG_7, 0),
+			BPF_ALU32_IMM(BPF_AND, BPF_REG_7, 1),
+			BPF_MOV32_REG(BPF_REG_0, BPF_REG_7),
+			BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 1),
+			BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_7, 0),
+			BPF_EXIT_INSN(),
+		},
+		.result = ACCEPT,
+		.retval = 0,
+	},
+	{
 		"unpriv: partial copy of pointer",
 		.insns = {
 			BPF_MOV32_REG(BPF_REG_1, BPF_REG_10),
-- 
2.7.4

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

* Re: [PATCH bpf-next] bpf: relax verifier restriction on BPF_MOV | BPF_ALU
  2018-12-05  9:46 [PATCH bpf-next] bpf: relax verifier restriction on BPF_MOV | BPF_ALU Jiong Wang
@ 2018-12-05 14:52 ` Edward Cree
  2018-12-05 15:32   ` Jiong Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Edward Cree @ 2018-12-05 14:52 UTC (permalink / raw
  To: Jiong Wang, ast, daniel; +Cc: netdev, oss-drivers

On 05/12/18 09:46, Jiong Wang wrote:
> There is NO processed instruction number regression, either with or without
> -mattr=+alu32.
<snip>
> Cilium bpf
> ===
> bpf_lb-DLB_L3.o         2110/2110    1730/1733
That looks like a regression of 3 insns in the 32-bit case.
May be worth investigating why.

> +			dst_reg = insn->dst_reg;
> +			regs[dst_reg] = regs[src_reg];
> +			if (BPF_CLASS(insn->code) == BPF_ALU) {
> +				/* Update type and range info. */
> +				regs[dst_reg].type = SCALAR_VALUE;
> +				coerce_reg_to_size(&regs[dst_reg], 4);
Won't this break when handed a pointer (as root, so allowed to leak
 it)?  E.g. (pointer + x) gets turned into scalar x, rather than
 unknown scalar in range [0, 0xffffffff].
The existing behaviour is correct for pointers: 32 unknown bits,
 because the value of the pointer base is unknown.
It's only for scalar_values that you want to copy and truncate the
 var_off and min/max from the src_reg.

-Ed

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

* Re: [PATCH bpf-next] bpf: relax verifier restriction on BPF_MOV | BPF_ALU
  2018-12-05 14:52 ` Edward Cree
@ 2018-12-05 15:32   ` Jiong Wang
  2018-12-05 17:38     ` Jiong Wang
  2018-12-06  3:13     ` Alexei Starovoitov
  0 siblings, 2 replies; 7+ messages in thread
From: Jiong Wang @ 2018-12-05 15:32 UTC (permalink / raw
  To: Edward Cree, ast, daniel; +Cc: netdev, oss-drivers

On 05/12/2018 14:52, Edward Cree wrote:
> On 05/12/18 09:46, Jiong Wang wrote:
>> There is NO processed instruction number regression, either with or without
>> -mattr=+alu32.
> <snip>
>> Cilium bpf
>> ===
>> bpf_lb-DLB_L3.o         2110/2110    1730/1733
> That looks like a regression of 3 insns in the 32-bit case.
> May be worth investigating why.

Will look into this.

>
>> +			dst_reg = insn->dst_reg;
>> +			regs[dst_reg] = regs[src_reg];
>> +			if (BPF_CLASS(insn->code) == BPF_ALU) {
>> +				/* Update type and range info. */
>> +				regs[dst_reg].type = SCALAR_VALUE;
>> +				coerce_reg_to_size(&regs[dst_reg], 4);
> Won't this break when handed a pointer (as root, so allowed to leak
>   it)?  E.g. (pointer + x) gets turned into scalar x, rather than
>   unknown scalar in range [0, 0xffffffff].

Initially I was gating this to scalar_value only, later was thinking it
could be extended to ptr case if ptr leak is allowed.

But, your comment remind me min/max value doesn't mean real min/max value
for ptr types value, it means the offset only if I am understanding the
issue correctly. So, it will break pointer case.

Regards,
Jiong

> The existing behaviour is correct for pointers: 32 unknown bits,
>   because the value of the pointer base is unknown.
> It's only for scalar_values that you want to copy and truncate the
>   var_off and min/max from the src_reg.
>
> -Ed

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

* Re: [PATCH bpf-next] bpf: relax verifier restriction on BPF_MOV | BPF_ALU
  2018-12-05 15:32   ` Jiong Wang
@ 2018-12-05 17:38     ` Jiong Wang
  2018-12-06  3:13     ` Alexei Starovoitov
  1 sibling, 0 replies; 7+ messages in thread
From: Jiong Wang @ 2018-12-05 17:38 UTC (permalink / raw
  To: Edward Cree, ast, daniel; +Cc: netdev, oss-drivers

On 05/12/2018 15:32, Jiong Wang wrote:
> On 05/12/2018 14:52, Edward Cree wrote:
>> On 05/12/18 09:46, Jiong Wang wrote:
>>> There is NO processed instruction number regression, either with or 
>>> without
>>> -mattr=+alu32.
>> <snip>
>>> Cilium bpf
>>> ===
>>> bpf_lb-DLB_L3.o         2110/2110    1730/1733
>> That looks like a regression of 3 insns in the 32-bit case.
>> May be worth investigating why.
>
> Will look into this.
>
Got some analysis from trace log, making a register contains accurate
integer could affect both path skip (conditional jump with immediate)
and path prune, details described below.

But I want to emphasis before this patch, turning a constant into unknown
could potentially causing verifier rejecting valid programs.

For example, for test_l4lb_noinline.c, there is the following code:

     struct real_definition *dst

1:  if (!get_packet_dst(&dst, &pckt, vip_info, is_ipv6))
2:    return TC_ACT_SHOT;
3:
4:  if (dst->flags & F_IPV6) {

get_packet_dst is responsible for initialize dst into valid pointer and
return true (1), otherwise return false (0).

As described in the patch description, the return sequence using alu32
will be:

   412: (54) (u32) r7 &= (u32) 1
   413: (bc) (u32) r0 = (u32) r7
   414: (95) exit

which would turn r0 into unknown value for all cases.

This is causing trouble when the code path hasn't initialized dst inside
get_packet_dst, for which case 0 is returned, and we would expect verifer
to skip the fall through path at C code line 4, otherwise it will complain
dst is not a pointer and will reject the program.

For why there are 3 more processed insn under -mattr=+alu32 for bpf_lb.o,
there are two places where verification logic diverges:

One is the following pattern:

  475: (bc) (u32) r1 = (u32) r7
  ...
  478: (55) if r1 != 0x7 goto pc+23
  ...

r1 gets its value from r7 which could be one of three different integer
0, 7, -157 when reached here.

Before this patch, insn 475 will make r1 into unknown while the integer
value is preserved after this patch.

So, if r1 is with value other than 0x7, the fall through path is skipped
after this patch.

But before the patch, after insn 478, verifier could also fix the value
of r1 into constant 0x7, and could do path prune for another path reaching
insn 478 if state comparison is safe.

 From the trace log, preserve r1's integer value wins here, path skipped
are more than path pruned, so after this patch, get fewer processed insn
for this part of sequence.

However there are another pattern:

  360: (bc) (u32) r7 = (u32) r1
  361: (05) goto pc+74
  436: (bc) (u32) r1 = (u32) r7
  437: (67) r1 <<= 32
  438: (c7) r1 s>>= 32

For this case, one more path prune happened when r7 is unknown value and
the pruned path is a long one which caused the total processed insn number
become 3 more.

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

* Re: [PATCH bpf-next] bpf: relax verifier restriction on BPF_MOV | BPF_ALU
  2018-12-05 15:32   ` Jiong Wang
  2018-12-05 17:38     ` Jiong Wang
@ 2018-12-06  3:13     ` Alexei Starovoitov
  2018-12-07 17:19       ` Jiong Wang
  1 sibling, 1 reply; 7+ messages in thread
From: Alexei Starovoitov @ 2018-12-06  3:13 UTC (permalink / raw
  To: Jiong Wang; +Cc: Edward Cree, ast, daniel, netdev, oss-drivers

On Wed, Dec 05, 2018 at 03:32:50PM +0000, Jiong Wang wrote:
> On 05/12/2018 14:52, Edward Cree wrote:
> > On 05/12/18 09:46, Jiong Wang wrote:
> > > There is NO processed instruction number regression, either with or without
> > > -mattr=+alu32.
> > <snip>
> > > Cilium bpf
> > > ===
> > > bpf_lb-DLB_L3.o         2110/2110    1730/1733
> > That looks like a regression of 3 insns in the 32-bit case.
> > May be worth investigating why.
> 
> Will look into this.
> 
> > 
> > > +			dst_reg = insn->dst_reg;
> > > +			regs[dst_reg] = regs[src_reg];
> > > +			if (BPF_CLASS(insn->code) == BPF_ALU) {
> > > +				/* Update type and range info. */
> > > +				regs[dst_reg].type = SCALAR_VALUE;
> > > +				coerce_reg_to_size(&regs[dst_reg], 4);
> > Won't this break when handed a pointer (as root, so allowed to leak
> >   it)?  E.g. (pointer + x) gets turned into scalar x, rather than
> >   unknown scalar in range [0, 0xffffffff].
> 
> Initially I was gating this to scalar_value only, later was thinking it
> could be extended to ptr case if ptr leak is allowed.
> 
> But, your comment remind me min/max value doesn't mean real min/max value
> for ptr types value, it means the offset only if I am understanding the
> issue correctly. So, it will break pointer case.

correct. In case of is_pointer_value() && root -> mark_reg_unknown() has to be called

The explanation of additional 3 steps from another email makes sense to me.

Can you take a look why it helps default (llvm alu64) case too ?
bpf_overlay.o           3096/2898

Thanks

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

* Re: [PATCH bpf-next] bpf: relax verifier restriction on BPF_MOV | BPF_ALU
  2018-12-06  3:13     ` Alexei Starovoitov
@ 2018-12-07 17:19       ` Jiong Wang
  2018-12-07 18:56         ` Alexei Starovoitov
  0 siblings, 1 reply; 7+ messages in thread
From: Jiong Wang @ 2018-12-07 17:19 UTC (permalink / raw
  To: Alexei Starovoitov; +Cc: Edward Cree, ast, daniel, netdev, oss-drivers

On 06/12/2018 03:13, Alexei Starovoitov wrote:
> On Wed, Dec 05, 2018 at 03:32:50PM +0000, Jiong Wang wrote:
>> On 05/12/2018 14:52, Edward Cree wrote:
>>> On 05/12/18 09:46, Jiong Wang wrote:
>>>> There is NO processed instruction number regression, either with or without
>>>> -mattr=+alu32.
>>> <snip>
>>>> Cilium bpf
>>>> ===
>>>> bpf_lb-DLB_L3.o         2110/2110    1730/1733
>>> That looks like a regression of 3 insns in the 32-bit case.
>>> May be worth investigating why.
>>
>> Will look into this.
>>
>>>
>>>> +                  dst_reg = insn->dst_reg;
>>>> +                  regs[dst_reg] = regs[src_reg];
>>>> +                  if (BPF_CLASS(insn->code) == BPF_ALU) {
>>>> +                          /* Update type and range info. */
>>>> +                          regs[dst_reg].type = SCALAR_VALUE;
>>>> +                          coerce_reg_to_size(&regs[dst_reg], 4);
>>> Won't this break when handed a pointer (as root, so allowed to leak
>>>   it)?  E.g. (pointer + x) gets turned into scalar x, rather than
>>>   unknown scalar in range [0, 0xffffffff].
>>
>> Initially I was gating this to scalar_value only, later was thinking it
>> could be extended to ptr case if ptr leak is allowed.
>>
>> But, your comment remind me min/max value doesn't mean real min/max value
>> for ptr types value, it means the offset only if I am understanding the
>> issue correctly. So, it will break pointer case.
>
> correct. In case of is_pointer_value() && root -> mark_reg_unknown() has to be called
>
> The explanation of additional 3 steps from another email makes sense to me.
>
> Can you take a look why it helps default (llvm alu64) case too ?
> bpf_overlay.o           3096/2898

It is embarrassing that I am not able to reproduce this number after tried
quite a few env configurations. I think the number must be wrong because
llvm alu64 binary doesn't contain alu32 move so shouldn't be impacted by
this patch even though I double checked the raw data I collected on llvm
alu64, re-calculated the number before this patch, it is still 3096. I
guess there must be something wrong with the binary I was loading.

I improved my benchmarking methodology to build all alu64 and alu32
binaries first, and never change them later. Then used a script to load and
collect the processed number. (borrowed the script from
https://github.com/4ast/bpf_cilium_test/, only my binaries are built from
latest Cilium repo and contains alu32 version as well)

I ran this new benchmarking env for several times, and could get the
following new results consistently:

     bpf_lb-DLB_L3.o:        2085/2085     1685/1687
     bpf_lb-DLB_L4.o:        2287/2287     1986/1982
     bpf_lb-DUNKNOWN.o:      690/690       622/622
     bpf_lxc.o:              95033/95033   N/A
     bpf_netdev.o:           7245/7245     N/A
     bpf_overlay.o:          2898/2898     3085/2947

No change on alu64 binary.

For alu32, bpf_overlay.o still get fewer processed instruction number, this
is because there is the following sequence (and another similar one).
Before this patch, r2 at insn 139 is unknown, so verifier always explore
both path-taken and path-fall_through. After this patch, it explores
path-fall_through only, so saved some insns.

   129: (b4) (u32) r7 = (u32) -140
   ...
   136: (bc) (u32) r2 = (u32) r7
   137: (74) (u32) r2 >>= (u32) 31
   138: (4c) (u32) r2 |= (u32) r1
   139: (15) if r2 == 0x0 goto pc+342
   140: (b4) (u32) r1 = (u32) 2

And a permissive register value for r2 hasn't released more path prune for
this test, so in all, after this patch, there is fewer processed insn.

I have sent out a v2, gated this change under SCALAR_VALUE, and also
updated the patch description.

Thanks.

Regards,
Jiong

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

* Re: [PATCH bpf-next] bpf: relax verifier restriction on BPF_MOV | BPF_ALU
  2018-12-07 17:19       ` Jiong Wang
@ 2018-12-07 18:56         ` Alexei Starovoitov
  0 siblings, 0 replies; 7+ messages in thread
From: Alexei Starovoitov @ 2018-12-07 18:56 UTC (permalink / raw
  To: Jiong Wang; +Cc: Edward Cree, ast, daniel, netdev, oss-drivers

On Fri, Dec 07, 2018 at 05:19:21PM +0000, Jiong Wang wrote:
> On 06/12/2018 03:13, Alexei Starovoitov wrote:
> > On Wed, Dec 05, 2018 at 03:32:50PM +0000, Jiong Wang wrote:
> > > On 05/12/2018 14:52, Edward Cree wrote:
> > > > On 05/12/18 09:46, Jiong Wang wrote:
> > > > > There is NO processed instruction number regression, either with or without
> > > > > -mattr=+alu32.
> > > > <snip>
> > > > > Cilium bpf
> > > > > ===
> > > > > bpf_lb-DLB_L3.o         2110/2110    1730/1733
> > > > That looks like a regression of 3 insns in the 32-bit case.
> > > > May be worth investigating why.
> > > 
> > > Will look into this.
> > > 
> > > > 
> > > > > +                  dst_reg = insn->dst_reg;
> > > > > +                  regs[dst_reg] = regs[src_reg];
> > > > > +                  if (BPF_CLASS(insn->code) == BPF_ALU) {
> > > > > +                          /* Update type and range info. */
> > > > > +                          regs[dst_reg].type = SCALAR_VALUE;
> > > > > +                          coerce_reg_to_size(&regs[dst_reg], 4);
> > > > Won't this break when handed a pointer (as root, so allowed to leak
> > > >   it)?  E.g. (pointer + x) gets turned into scalar x, rather than
> > > >   unknown scalar in range [0, 0xffffffff].
> > > 
> > > Initially I was gating this to scalar_value only, later was thinking it
> > > could be extended to ptr case if ptr leak is allowed.
> > > 
> > > But, your comment remind me min/max value doesn't mean real min/max value
> > > for ptr types value, it means the offset only if I am understanding the
> > > issue correctly. So, it will break pointer case.
> > 
> > correct. In case of is_pointer_value() && root -> mark_reg_unknown() has to be called
> > 
> > The explanation of additional 3 steps from another email makes sense to me.
> > 
> > Can you take a look why it helps default (llvm alu64) case too ?
> > bpf_overlay.o           3096/2898
> 
> It is embarrassing that I am not able to reproduce this number after tried
> quite a few env configurations. I think the number must be wrong because
> llvm alu64 binary doesn't contain alu32 move so shouldn't be impacted by
> this patch even though I double checked the raw data I collected on llvm
> alu64, re-calculated the number before this patch, it is still 3096. I
> guess there must be something wrong with the binary I was loading.
> 
> I improved my benchmarking methodology to build all alu64 and alu32
> binaries first, and never change them later. Then used a script to load and
> collect the processed number. (borrowed the script from
> https://github.com/4ast/bpf_cilium_test/, only my binaries are built from
> latest Cilium repo and contains alu32 version as well)
> 
> I ran this new benchmarking env for several times, and could get the
> following new results consistently:
> 
>     bpf_lb-DLB_L3.o:        2085/2085     1685/1687
>     bpf_lb-DLB_L4.o:        2287/2287     1986/1982
>     bpf_lb-DUNKNOWN.o:      690/690       622/622
>     bpf_lxc.o:              95033/95033   N/A
>     bpf_netdev.o:           7245/7245     N/A
>     bpf_overlay.o:          2898/2898     3085/2947
> 
> No change on alu64 binary.
> 
> For alu32, bpf_overlay.o still get fewer processed instruction number, this
> is because there is the following sequence (and another similar one).
> Before this patch, r2 at insn 139 is unknown, so verifier always explore
> both path-taken and path-fall_through. After this patch, it explores
> path-fall_through only, so saved some insns.
> 
>   129: (b4) (u32) r7 = (u32) -140
>   ...
>   136: (bc) (u32) r2 = (u32) r7
>   137: (74) (u32) r2 >>= (u32) 31
>   138: (4c) (u32) r2 |= (u32) r1
>   139: (15) if r2 == 0x0 goto pc+342
>   140: (b4) (u32) r1 = (u32) 2
> 
> And a permissive register value for r2 hasn't released more path prune for
> this test, so in all, after this patch, there is fewer processed insn.
> 
> I have sent out a v2, gated this change under SCALAR_VALUE, and also
> updated the patch description.

Thanks for the update. Makes sense.

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

end of thread, other threads:[~2018-12-07 18:56 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-05  9:46 [PATCH bpf-next] bpf: relax verifier restriction on BPF_MOV | BPF_ALU Jiong Wang
2018-12-05 14:52 ` Edward Cree
2018-12-05 15:32   ` Jiong Wang
2018-12-05 17:38     ` Jiong Wang
2018-12-06  3:13     ` Alexei Starovoitov
2018-12-07 17:19       ` Jiong Wang
2018-12-07 18:56         ` Alexei Starovoitov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.