Messages in this thread | | | Subject | Re: [PATCH bpf-next v2 4/6] bpf: override the meaning of ARG_PTR_TO_MAP_VALUE for sockmap and sockhash | From | Yonghong Song <> | Date | Thu, 20 Aug 2020 09:10:39 -0700 |
| |
On 8/20/20 6:57 AM, Lorenz Bauer wrote: > The verifier assumes that map values are simple blobs of memory, and > therefore treats ARG_PTR_TO_MAP_VALUE, etc. as such. However, there are > map types where this isn't true. For example, sockmap and sockhash store > sockets. In general this isn't a big problem: we can just > write helpers that explicitly requests PTR_TO_SOCKET instead of > ARG_PTR_TO_MAP_VALUE. > > The one exception are the standard map helpers like map_update_elem, > map_lookup_elem, etc. Here it would be nice we could overload the > function prototype for different kinds of maps. Unfortunately, this > isn't entirely straight forward: > We only know the type of the map once we have resolved meta->map_ptr > in check_func_arg. This means we can't swap out the prototype > in check_helper_call until we're half way through the function. > > Instead, modify check_func_arg to treat ARG_PTR_TO_MAP_VALUE* to > mean "the native type for the map" instead of "pointer to memory" > for sockmap and sockhash. This means we don't have to modify the > function prototype at all > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> > --- > kernel/bpf/verifier.c | 37 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index b6ccfce3bf4c..24feec515d3e 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -3872,6 +3872,35 @@ static int int_ptr_type_to_size(enum bpf_arg_type type) > return -EINVAL; > } > > +static int resolve_map_arg_type(struct bpf_verifier_env *env, > + const struct bpf_call_arg_meta *meta, > + enum bpf_arg_type *arg_type) > +{ > + if (!meta->map_ptr) { > + /* kernel subsystem misconfigured verifier */ > + verbose(env, "invalid map_ptr to access map->type\n"); > + return -EACCES; > + } > + > + switch (meta->map_ptr->map_type) { > + case BPF_MAP_TYPE_SOCKMAP: > + case BPF_MAP_TYPE_SOCKHASH: > + if (*arg_type == ARG_PTR_TO_MAP_VALUE) { > + *arg_type = ARG_PTR_TO_SOCKET; > + } else if (*arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) { > + *arg_type = ARG_PTR_TO_SOCKET_OR_NULL;
Is this *arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL possible with current implementation?
If not, we can remove this "else if" and return -EINVAL, right?
> + } else { > + verbose(env, "invalid arg_type for sockmap/sockhash\n"); > + return -EINVAL; > + } > + break; > + > + default: > + break; > + } > + return 0; > +} > + > static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > struct bpf_call_arg_meta *meta, > const struct bpf_func_proto *fn) > @@ -3904,6 +3933,14 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > return -EACCES; > } > > + if (arg_type == ARG_PTR_TO_MAP_VALUE || > + arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE || > + arg_type == ARG_PTR_TO_MAP_VALUE_OR_NULL) { > + err = resolve_map_arg_type(env, meta, &arg_type);
I am okay with this to cover all MAP_VALUE types with func name resolve_map_arg_type as a generic helper.
> + if (err) > + return err; > + } > + > if (arg_type == ARG_PTR_TO_MAP_KEY || > arg_type == ARG_PTR_TO_MAP_VALUE || > arg_type == ARG_PTR_TO_UNINIT_MAP_VALUE || >
| |