On Mon, Sep 11, 2017 at 06:14:59PM -0700, Alexei Starovoitov via iovisor-dev
wrote:
> On Sat, Sep 09, 2017 at 04:31:16PM +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.
> >
> > 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);
> > (*count)++;
> > }
> >
> > Signed-off-by: Paul Chaignon <paul.chaig...@orange.com>
> > ---
> > Changes in v2:
> > - Additional test cases for adjusted maps.
> >
> > kernel/bpf/verifier.c | 9 +++-
> > tools/testing/selftests/bpf/test_verifier.c | 84
> > +++++++++++++++++++++++++++++
> > 2 files changed, 92 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);
> > 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);
>
> as pointed out earlier. above is a bug.
> please add negative test cases to see it.
> See below...
>
> > 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..4532066 100644
> > --- a/tools/testing/selftests/bpf/test_verifier.c
> > +++ b/tools/testing/selftests/bpf/test_verifier.c
> > @@ -5052,6 +5052,90 @@ 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,
> > + },
> > + {
> > + "map helper access to adjusted map (via const imm)",
> > + .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, 5),
> > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_0),
> > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2,
> > + offsetof(struct test_val, foo)),
>
> there are few bugs here.
> 1. it adds 4 byte, so it should have been rejected as misaligned.
I've fixed the other bugs, but I don't understand that one. Don't all
map helpers assume byte aligned pointers? Even without this patch, map
helper accesses to misaligned pointers are accepted if we first copy
the value on the stack.
> 2. if you add 100 here, it will still pass.
>
> > + BPF_LD_MAP_FD(BPF_REG_1, 0),
> > + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
> > + BPF_EXIT_INSN(),
> > + },
> > + .fixup_map1 = { 9 },
> > + .fixup_map2 = { 3 },
> > + .result = ACCEPT,
> > + .prog_type = BPF_PROG_TYPE_TRACEPOINT,
> > + },
> > + {
> > + "map helper access to adjusted map (via const reg)",
> > + .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, 6),
> > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_0),
> > + BPF_MOV64_IMM(BPF_REG_3,
> > + offsetof(struct test_val, foo)),
> > + BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_3),
> > + BPF_LD_MAP_FD(BPF_REG_1, 0),
> > + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
> > + BPF_EXIT_INSN(),
> > + },
> > + .fixup_map1 = { 10 },
> > + .fixup_map2 = { 3 },
> > + .result = ACCEPT,
> > + .prog_type = BPF_PROG_TYPE_TRACEPOINT,
> > + },
> > + {
> > + "map helper access to adjusted map (via variable)",
> > + .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, 7),
> > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_0),
> > + BPF_LDX_MEM(BPF_W, BPF_REG_3, BPF_REG_0, 0),
> > + BPF_JMP_IMM(BPF_JGT, BPF_REG_3,
> > + offsetof(struct test_val, foo), 4),
> > + BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_3),
> > + BPF_LD_MAP_FD(BPF_REG_1, 0),
> > + BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
> > + BPF_EXIT_INSN(),
> > + },
> > + .fixup_map1 = { 11 },
> > + .fixup_map2 = { 3 },
> > + .result = ACCEPT,
> > + .prog_type = BPF_PROG_TYPE_TRACEPOINT,
> > + },
> > + {
> > "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
_______________________________________________
iovisor-dev mailing list
iovisor-dev@lists.iovisor.org
https://lists.iovisor.org/mailman/listinfo/iovisor-dev
