thanks for the patch, it works.
>> from 52 to 80: R0=imm1,min_value=1,max_value=1 R1=pkt(id=0,off=0,r=34)
>> R2=pkt_end R3=inv R4=imm272 R5=inv56,min_value=17,max_value=17
>> R6=pkt(id=0,off=26,r=34) R10=fp
>> 80: (07) r4 += 71
>> 81: (18) r5 = 0xfffffff8
>> 83: (5f) r4 &= r5
>> 84: (77) r4 >>= 3
>> 85: (0f) r1 += r4
>> cannot add integer value with 3 upper zero bits to ptr_to_packet
>>
>> I have no idea why this time the compiler wants to do "and" then
>> "right shift", instead of directly shit right 3 bit. R5's type is
>> UNKNOWN_VALUE, So in addition to your patch, I add
>
>
> But what the compiler is doing here is still correct eventually I assume?
Yes, it zero out the least significant 3 bits first, then shift right 3 bits.
> We could also add BPF_AND, but that r5's type is UNKNOWN_VALUE doesn't
> seem right if I'm not mistaken. That r5 = 0xfffffff8 should be a dw imm
> load, print_bpf_insn() seems only to print first part. So next r4 &= r5
> should be purely imm operation as well and thus trackable. Only that we
> need to teach verifier to keep the dw imm when not used with analyzer_ops
> (nfp related).
>
> Could you try the below (only compile tested here) to see whether the
> verifier can now analyze that part?
>
> Thanks,
> Daniel
>
> kernel/bpf/verifier.c | 35
> ++++++++++++++++++++++-------
> tools/testing/selftests/bpf/test_verifier.c | 24 ++++++++++++++++++++
> 2 files changed, 51 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d60e12c..777ef0a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1567,19 +1567,44 @@ static int evaluate_reg_imm_alu(struct
> bpf_verifier_env *env,
>
> struct bpf_reg_state *src_reg = ®s[insn->src_reg];
> u8 opcode = BPF_OP(insn->code);
>
> - /* dst_reg->type == CONST_IMM here, simulate execution of 'add'/'or'
> - * insn. Don't care about overflow or negative values, just add them
> + /* dst_reg->type == CONST_IMM here, simulate execution of
> 'add'/'or'/...
> + * insn. Don't care about overflow or negative values, just add
> them.
> */
> if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_K)
> dst_reg->imm += insn->imm;
> else if (opcode == BPF_ADD && BPF_SRC(insn->code) == BPF_X &&
> src_reg->type == CONST_IMM)
> dst_reg->imm += src_reg->imm;
> + else if (opcode == BPF_SUB && BPF_SRC(insn->code) == BPF_K)
> + dst_reg->imm -= insn->imm;
> + else if (opcode == BPF_SUB && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM)
> + dst_reg->imm -= src_reg->imm;
> + else if (opcode == BPF_MUL && BPF_SRC(insn->code) == BPF_K)
> + dst_reg->imm *= insn->imm;
> + else if (opcode == BPF_MUL && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM)
> + dst_reg->imm *= src_reg->imm;
> else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_K)
> dst_reg->imm |= insn->imm;
> else if (opcode == BPF_OR && BPF_SRC(insn->code) == BPF_X &&
> src_reg->type == CONST_IMM)
> dst_reg->imm |= src_reg->imm;
> + else if (opcode == BPF_AND && BPF_SRC(insn->code) == BPF_K)
> + dst_reg->imm &= insn->imm;
> + else if (opcode == BPF_AND && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM)
> + dst_reg->imm &= src_reg->imm;
Thanks, with this patch. now r5 is CONST_IMM, not UNKNOWN_VALUE.
Regards,
William
> + else if (opcode == BPF_RSH && BPF_SRC(insn->code) == BPF_K)
> + dst_reg->imm >>= insn->imm;
> + else if (opcode == BPF_RSH && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM)
> + dst_reg->imm >>= src_reg->imm;
> + else if (opcode == BPF_LSH && BPF_SRC(insn->code) == BPF_K)
> + dst_reg->imm <<= insn->imm;
> + else if (opcode == BPF_LSH && BPF_SRC(insn->code) == BPF_X &&
> + src_reg->type == CONST_IMM)
> + dst_reg->imm <<= src_reg->imm;
> else
> mark_reg_unknown_value(regs, insn->dst_reg);
> return 0;
> @@ -2225,14 +2250,8 @@ static int check_ld_imm(struct bpf_verifier_env *env,
> struct bpf_insn *insn)
> return err;
>
> if (insn->src_reg == 0) {
> - /* generic move 64-bit immediate into a register,
> - * only analyzer needs to collect the ld_imm value.
> - */
> u64 imm = ((u64)(insn + 1)->imm << 32) | (u32)insn->imm;
>
> - if (!env->analyzer_ops)
> - return 0;
> -
> regs[insn->dst_reg].type = CONST_IMM;
> regs[insn->dst_reg].imm = imm;
>
> return 0;
> diff --git a/tools/testing/selftests/bpf/test_verifier.c
> b/tools/testing/selftests/bpf/test_verifier.c
> index 1aa7324..d60bd07 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -2326,6 +2326,30 @@ static struct bpf_test tests[] = {
> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> },
> {
> + "direct packet access: test11 (shift, good access)",
> + .insns = {
> + BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
> + offsetof(struct __sk_buff, data)),
> + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_1,
> + offsetof(struct __sk_buff, data_end)),
> + BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 22),
> + BPF_JMP_REG(BPF_JGT, BPF_REG_0, BPF_REG_3, 8),
> + BPF_MOV64_IMM(BPF_REG_3, 144),
> + BPF_MOV64_REG(BPF_REG_5, BPF_REG_3),
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_5, 23),
> + BPF_ALU64_IMM(BPF_RSH, BPF_REG_5, 3),
> + BPF_MOV64_REG(BPF_REG_6, BPF_REG_2),
> + BPF_ALU64_REG(BPF_ADD, BPF_REG_6, BPF_REG_5),
> + BPF_MOV64_IMM(BPF_REG_0, 1),
> + BPF_EXIT_INSN(),
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .result = ACCEPT,
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + },
> + {
> "helper access to packet: test1, valid packet_ptr range",
> .insns = {
> BPF_LDX_MEM(BPF_W, BPF_REG_2, BPF_REG_1,
> --
> 2.5.5
>
>
_______________________________________________
iovisor-dev mailing list
[email protected]
https://lists.iovisor.org/mailman/listinfo/iovisor-dev
