On Fri, Sep 08, 2017 at 10:42:06PM +0200, Paul Chaignon wrote: > Helpers that expect ARG_PTR_TO_MAP_KEY and ARG_PTR_TO_MAP_VALUE can only > access stack and packet memory. Allow these helpers to directly access map > values by passing registers of type PTR_TO_MAP_VALUE.
thanks for the patch. Yes. It makes sense to teach verifier to recognize this. > This change removes the need for an extra copy to the stack when using a > map value to perform a second map lookup, as in the following: > > struct bpf_map_def SEC("maps") infobyreq = { > .type = BPF_MAP_TYPE_HASHMAP, > .key_size = sizeof(struct request *), > .value_size = sizeof(struct info_t), > .max_entries = 1024, > }; > struct bpf_map_def SEC("maps") counts = { > .type = BPF_MAP_TYPE_HASHMAP, > .key_size = sizeof(struct info_t), > .value_size = sizeof(u64), > .max_entries = 1024, > }; > SEC("kprobe/blk_account_io_start") > int bpf_blk_account_io_start(struct pt_regs *ctx) > { > struct info_t *info = bpf_map_lookup_elem(&infobyreq, &ctx->di); > u64 *count = bpf_map_lookup_elem(&counts, info); since we're at it I think we should allow and test the other combinations: u64 *count = bpf_map_lookup_elem(&counts, info + off); // info + var, etc since I don't think the patch does it. > (*count)++; > } > > Signed-off-by: Paul Chaignon <paul.chaig...@orange.com> > --- > kernel/bpf/verifier.c | 9 ++++++++- > tools/testing/selftests/bpf/test_verifier.c | 18 ++++++++++++++++++ > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index d690c7d..50e057d 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -1351,7 +1351,8 @@ static int check_func_arg(struct bpf_verifier_env *env, > u32 regno, > if (arg_type == ARG_PTR_TO_MAP_KEY || > arg_type == ARG_PTR_TO_MAP_VALUE) { > expected_type = PTR_TO_STACK; > - if (type != PTR_TO_PACKET && type != expected_type) > + if (type != PTR_TO_PACKET && type != PTR_TO_MAP_VALUE && > + type != expected_type) > goto err_type; > } else if (arg_type == ARG_CONST_SIZE || > arg_type == ARG_CONST_SIZE_OR_ZERO) { > @@ -1404,6 +1405,9 @@ static int check_func_arg(struct bpf_verifier_env *env, > u32 regno, > if (type == PTR_TO_PACKET) > err = check_packet_access(env, regno, reg->off, > meta->map_ptr->key_size); > + else if (type == PTR_TO_MAP_VALUE) > + err = check_map_access(env, regno, 0, > + meta->map_ptr->key_size); why 0 ? shouldn't it be reg->off ? > else > err = check_stack_boundary(env, regno, > meta->map_ptr->key_size, > @@ -1420,6 +1424,9 @@ static int check_func_arg(struct bpf_verifier_env *env, > u32 regno, > if (type == PTR_TO_PACKET) > err = check_packet_access(env, regno, reg->off, > meta->map_ptr->value_size); > + else if (type == PTR_TO_MAP_VALUE) > + err = check_map_access(env, regno, 0, > + meta->map_ptr->key_size); s/0/reg->off/ s/key_size/value_size/ ? > else > err = check_stack_boundary(env, regno, > meta->map_ptr->value_size, > diff --git a/tools/testing/selftests/bpf/test_verifier.c > b/tools/testing/selftests/bpf/test_verifier.c > index 8eb0995..917e346 100644 > --- a/tools/testing/selftests/bpf/test_verifier.c > +++ b/tools/testing/selftests/bpf/test_verifier.c > @@ -5052,6 +5052,24 @@ static struct bpf_test tests[] = { > .prog_type = BPF_PROG_TYPE_TRACEPOINT, > }, > { > + "map helper access to map", > + .insns = { > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), > + BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0), > + BPF_LD_MAP_FD(BPF_REG_1, 0), > + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem), > + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4), > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_0), > + BPF_LD_MAP_FD(BPF_REG_1, 0), > + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem), > + BPF_EXIT_INSN(), > + }, > + .fixup_map1 = { 3, 8 }, > + .result = ACCEPT, > + .prog_type = BPF_PROG_TYPE_TRACEPOINT, Thanks for the test, since this stuff is delicate, please add more test to cover negative cases and value_ptr+variable as well > + }, > + { > "map element value is preserved across register spilling", > .insns = { > BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), > -- > 2.7.4 > _______________________________________________ iovisor-dev mailing list iovisor-dev@lists.iovisor.org https://lists.iovisor.org/mailman/listinfo/iovisor-dev