Re: [iovisor-dev] [PATCH RFC v2] bpf: allow map helpers access to map values directly

[Alexei Starovoitov via iovisor-dev]

On Mon, Sep 18, 2017 at 11:14 AM, Paul Chaignon
 wrote:
>> > +   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.

that is a bit of grey territory unfortunately which we need to
document and audit the verifier.
Most map helpers don't take advantage of key/value alignment
constraint, but pcpu maps, lpm maps already require
both key and value to be aligned.
Ex: bpf_long_memcpy() and trie_lookup_elem().
___
iovisor-dev mailing list
iovisor-dev@lists.iovisor.org
https://lists.iovisor.org/mailman/listinfo/iovisor-dev

Re: [iovisor-dev] [PATCH RFC v2] bpf: allow map helpers access to map values directly

[Alexei Starovoitov via iovisor-dev]

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(, >di);
> u64 *count = bpf_map_lookup_elem(, info);
> (*count)++;
> }
> 
> Signed-off-by: Paul Chaignon 
> ---
> 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),
> +