On Thu, Jul 16, 2020 at 12:28:16AM +0200, Daniel Borkmann wrote: > On 7/15/20 9:00 AM, Hangbin Liu wrote: > > Add a new bpf argument type ARG_CONST_MAP_PTR_OR_NULL which could be > > used when we want to allow NULL pointer for map parameter. The bpf helper > > need to take care and check if the map is NULL when use this type. > > > > Signed-off-by: Hangbin Liu <liuhangbin@xxxxxxxxx> > > Is this patch to be merged into the set in [0] for passing NULL ex_map as discussed? > Seems you sent out two incomplete sets? > > [0] https://lore.kernel.org/bpf/20200709013008.3900892-1-liuhangbin@xxxxxxxxx/T/#m99a8fa8ffe79d5f00d305c0800ad3abe619294f2 Yes, I did it by intend. I thought these two should be consider as different feature. So I'd prefer post them separately. Once both patches are merged, I will post a followup patch to add the NULL pointer support to xdp multicast helper. > > > --- > > include/linux/bpf.h | 1 + > > kernel/bpf/verifier.c | 11 ++++++++--- > > 2 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index c67c88ad35f8..9d4dbef3c943 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -253,6 +253,7 @@ enum bpf_arg_type { > > ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ > > ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ > > ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */ > > + ARG_CONST_MAP_PTR_OR_NULL, /* const argument used as pointer to bpf_map or NULL */ > > }; > > /* type of values returned from helper functions */ > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 3c1efc9d08fd..d3551a19853a 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -3849,9 +3849,13 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > > expected_type = SCALAR_VALUE; > > if (type != expected_type) > > goto err_type; > > - } else if (arg_type == ARG_CONST_MAP_PTR) { > > + } else if (arg_type == ARG_CONST_MAP_PTR || > > + arg_type == ARG_CONST_MAP_PTR_OR_NULL) { > > expected_type = CONST_PTR_TO_MAP; > > - if (type != expected_type) > > + if (register_is_null(reg) && > > + arg_type == ARG_CONST_MAP_PTR_OR_NULL) > > + /* final test in check_stack_boundary() */; > > + else if (type != expected_type) > > goto err_type; > > } else if (arg_type == ARG_PTR_TO_CTX || > > arg_type == ARG_PTR_TO_CTX_OR_NULL) { > > @@ -3957,7 +3961,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > > return -EFAULT; > > } > > - if (arg_type == ARG_CONST_MAP_PTR) { > > + if (arg_type == ARG_CONST_MAP_PTR || > > + (arg_type == ARG_CONST_MAP_PTR_OR_NULL && !register_is_null(reg))) { > > /* bpf_map_xxx(map_ptr) call: remember that map_ptr */ > > meta->map_ptr = reg->map_ptr; > > I would probably have the semantics a bit different in the sense that I would > update meta->map_ptr to the last ARG_CONST_MAP_PTR, meaning: > > meta->map_ptr = register_is_null(reg) ? NULL : reg->map_ptr; Thanks for the suggestion. I will update it. > > > } else if (arg_type == ARG_PTR_TO_MAP_KEY) { > > > > In combination with the set, this also needs test_verifier selftests in order to > exercise BPF insn snippets for the good & [expected] bad case. OK, I will add a test for this. Thanks Hangbin
