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

Paul Chaignon paul.chaignon at orange.com
Tue Sep 12 08:30:38 UTC 2017


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.chaignon at 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...

Sorry for the mess. I'm not sure how I missed your previous comments.
I'll come back with a proper, thorougly tested, patch.

> 
> >  		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.
> 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 at lists.iovisor.org
> https://lists.iovisor.org/mailman/listinfo/iovisor-dev


More information about the iovisor-dev mailing list