| Message ID | 20200922070415.1916194-1-kafai@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Enable bpf_skc_to_* sock casting helper to networking prog type | expand |
On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote: > > check_reg_type() checks whether a reg can be used as an arg of a > func_proto. For PTR_TO_BTF_ID, the check is actually not > completely done until the reg->btf_id is pointing to a > kernel struct that is acceptable by the func_proto. > > Thus, this patch moves the btf_id check into check_reg_type(). > The compatible_reg_types[] usage is localized in check_reg_type() now. > > The "if (!btf_id) verbose(...); " is also removed since it won't happen. > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- > kernel/bpf/verifier.c | 65 +++++++++++++++++++++++-------------------- > 1 file changed, 35 insertions(+), 30 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 15ab889b0a3f..3ce61c412ea0 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -4028,20 +4028,29 @@ static const struct bpf_reg_types *compatible_reg_types[] = { > [__BPF_ARG_TYPE_MAX] = NULL, > }; > > -static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > - const struct bpf_reg_types *compatible) > +static int check_reg_type(struct bpf_verifier_env *env, u32 arg, > + enum bpf_arg_type arg_type, > + const struct bpf_func_proto *fn) How about (env, regno, arg_type, expected_btf_id) instead? Otherwise implementing sockmap update from iter with your current approach is difficult for me. See resolve_map_arg_type, which now needs to resolve expected_bpf_id. See below for what I mean: diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 2468533bc4a1..3a238a295c37 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3931,7 +3931,8 @@ static int int_ptr_type_to_size(enum bpf_arg_type type) static int resolve_map_arg_type(struct bpf_verifier_env *env, const struct bpf_call_arg_meta *meta, - enum bpf_arg_type *arg_type) + enum bpf_arg_type *arg_type, + u32 **expected_btf_id) { if (!meta->map_ptr) { /* kernel subsystem misconfigured verifier */ @@ -3943,7 +3944,8 @@ static int resolve_map_arg_type(struct bpf_verifier_env *env, case BPF_MAP_TYPE_SOCKMAP: case BPF_MAP_TYPE_SOCKHASH: if (*arg_type == ARG_PTR_TO_MAP_VALUE) { - *arg_type = ARG_PTR_TO_SOCKET; + *arg_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON; + *expected_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]; } else { verbose(env, "invalid arg_type for sockmap/sockhash\n"); return -EINVAL; @@ -4044,11 +4046,9 @@ static const struct bpf_reg_types *compatible_reg_types[] = { [__BPF_ARG_TYPE_MAX] = NULL, }; -static int check_reg_type(struct bpf_verifier_env *env, u32 arg, - enum bpf_arg_type arg_type, - const struct bpf_func_proto *fn) +static int check_reg_type(struct bpf_verifier_env *env, u32 regno, + enum bpf_arg_type arg_type, u32 *expected_btf_id) { - u32 regno = BPF_REG_1 + arg; struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; enum bpf_reg_type expected, type = reg->type; const struct bpf_reg_types *compatible; @@ -4077,8 +4077,6 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 arg, found: if (type == PTR_TO_BTF_ID) { - u32 *expected_btf_id = fn->arg_btf_id[arg]; - if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id, *expected_btf_id)) { verbose(env, "R%d is of type %s but %s is expected\n", @@ -4105,6 +4103,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; enum bpf_arg_type arg_type = fn->arg_type[arg]; enum bpf_reg_type type = reg->type; + u32 *expected_btf_id = fn->arg_btf_id[arg]; int err = 0; if (arg_type == ARG_DONTCARE) @@ -4132,7 +4131,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, 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); + err = resolve_map_arg_type(env, meta, &arg_type, &expected_btf_id); if (err) return err; } @@ -4143,7 +4142,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, */ goto skip_type_check; - err = check_reg_type(env, arg, arg_type, fn); + err = check_reg_type(env, regno, arg_type, expected_btf_id); if (err) return err;
On Tue, Sep 22, 2020 at 10:56:55AM +0100, Lorenz Bauer wrote: > On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote: > > > > check_reg_type() checks whether a reg can be used as an arg of a > > func_proto. For PTR_TO_BTF_ID, the check is actually not > > completely done until the reg->btf_id is pointing to a > > kernel struct that is acceptable by the func_proto. > > > > Thus, this patch moves the btf_id check into check_reg_type(). > > The compatible_reg_types[] usage is localized in check_reg_type() now. > > > > The "if (!btf_id) verbose(...); " is also removed since it won't happen. > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > --- > > kernel/bpf/verifier.c | 65 +++++++++++++++++++++++-------------------- > > 1 file changed, 35 insertions(+), 30 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 15ab889b0a3f..3ce61c412ea0 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -4028,20 +4028,29 @@ static const struct bpf_reg_types *compatible_reg_types[] = { > > [__BPF_ARG_TYPE_MAX] = NULL, > > }; > > > > -static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > > - const struct bpf_reg_types *compatible) > > +static int check_reg_type(struct bpf_verifier_env *env, u32 arg, > > + enum bpf_arg_type arg_type, > > + const struct bpf_func_proto *fn) Yes. I think that works as good. An idea for the mid term, I think this map helper's arg override logic should belong to a new map_ops and this new map_ops can return the whole "fn" instead of overriding on an arg-by-arg base.
On Tue, Sep 22, 2020 at 10:56:55AM +0100, Lorenz Bauer wrote: > On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote: > > > > check_reg_type() checks whether a reg can be used as an arg of a > > func_proto. For PTR_TO_BTF_ID, the check is actually not > > completely done until the reg->btf_id is pointing to a > > kernel struct that is acceptable by the func_proto. > > > > Thus, this patch moves the btf_id check into check_reg_type(). > > The compatible_reg_types[] usage is localized in check_reg_type() now. > > > > The "if (!btf_id) verbose(...); " is also removed since it won't happen. > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > --- > > kernel/bpf/verifier.c | 65 +++++++++++++++++++++++-------------------- > > 1 file changed, 35 insertions(+), 30 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 15ab889b0a3f..3ce61c412ea0 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -4028,20 +4028,29 @@ static const struct bpf_reg_types *compatible_reg_types[] = { > > [__BPF_ARG_TYPE_MAX] = NULL, > > }; > > > > -static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > > - const struct bpf_reg_types *compatible) > > +static int check_reg_type(struct bpf_verifier_env *env, u32 arg, > > + enum bpf_arg_type arg_type, > > + const struct bpf_func_proto *fn) > > How about (env, regno, arg_type, expected_btf_id) instead? Otherwise Last reply cut off in the wrong line. I will make this change in v4.
On Tue, 22 Sep 2020 at 19:38, Martin KaFai Lau <kafai@fb.com> wrote: > > On Tue, Sep 22, 2020 at 10:56:55AM +0100, Lorenz Bauer wrote: > > On Tue, 22 Sep 2020 at 08:04, Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > check_reg_type() checks whether a reg can be used as an arg of a > > > func_proto. For PTR_TO_BTF_ID, the check is actually not > > > completely done until the reg->btf_id is pointing to a > > > kernel struct that is acceptable by the func_proto. > > > > > > Thus, this patch moves the btf_id check into check_reg_type(). > > > The compatible_reg_types[] usage is localized in check_reg_type() now. > > > > > > The "if (!btf_id) verbose(...); " is also removed since it won't happen. > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > --- > > > kernel/bpf/verifier.c | 65 +++++++++++++++++++++++-------------------- > > > 1 file changed, 35 insertions(+), 30 deletions(-) > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index 15ab889b0a3f..3ce61c412ea0 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -4028,20 +4028,29 @@ static const struct bpf_reg_types *compatible_reg_types[] = { > > > [__BPF_ARG_TYPE_MAX] = NULL, > > > }; > > > > > > -static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > > > - const struct bpf_reg_types *compatible) > > > +static int check_reg_type(struct bpf_verifier_env *env, u32 arg, > > > + enum bpf_arg_type arg_type, > > > + const struct bpf_func_proto *fn) > Yes. I think that works as good. > > An idea for the mid term, I think this map helper's arg override logic > should belong to a new map_ops and this new map_ops can return the whole > "fn" instead of overriding on an arg-by-arg base. Yeah, agreed. I've had a similar idea, but no time to implement it yet.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 15ab889b0a3f..3ce61c412ea0 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4028,20 +4028,29 @@ static const struct bpf_reg_types *compatible_reg_types[] = { [__BPF_ARG_TYPE_MAX] = NULL, }; -static int check_reg_type(struct bpf_verifier_env *env, u32 regno, - const struct bpf_reg_types *compatible) +static int check_reg_type(struct bpf_verifier_env *env, u32 arg, + enum bpf_arg_type arg_type, + const struct bpf_func_proto *fn) { + u32 regno = BPF_REG_1 + arg; struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; enum bpf_reg_type expected, type = reg->type; + const struct bpf_reg_types *compatible; int i, j; + compatible = compatible_reg_types[arg_type]; + if (!compatible) { + verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type); + return -EFAULT; + } + for (i = 0; i < ARRAY_SIZE(compatible->types); i++) { expected = compatible->types[i]; if (expected == NOT_INIT) break; if (type == expected) - return 0; + goto found; } verbose(env, "R%d type=%s expected=", regno, reg_type_str[type]); @@ -4049,6 +4058,27 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, verbose(env, "%s, ", reg_type_str[compatible->types[j]]); verbose(env, "%s\n", reg_type_str[compatible->types[j]]); return -EACCES; + +found: + if (type == PTR_TO_BTF_ID) { + u32 *expected_btf_id = fn->arg_btf_id[arg]; + + if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id, + *expected_btf_id)) { + verbose(env, "R%d is of type %s but %s is expected\n", + regno, kernel_type_name(reg->btf_id), + kernel_type_name(*expected_btf_id)); + return -EACCES; + } + + if (!tnum_is_const(reg->var_off) || reg->var_off.value) { + verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n", + regno); + return -EACCES; + } + } + + return 0; } static int check_func_arg(struct bpf_verifier_env *env, u32 arg, @@ -4058,7 +4088,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, u32 regno = BPF_REG_1 + arg; struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; enum bpf_arg_type arg_type = fn->arg_type[arg]; - const struct bpf_reg_types *compatible; enum bpf_reg_type type = reg->type; int err = 0; @@ -4098,35 +4127,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, */ goto skip_type_check; - compatible = compatible_reg_types[arg_type]; - if (!compatible) { - verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type); - return -EFAULT; - } - - err = check_reg_type(env, regno, compatible); + err = check_reg_type(env, arg, arg_type, fn); if (err) return err; - if (type == PTR_TO_BTF_ID) { - const u32 *btf_id = fn->arg_btf_id[arg]; - - if (!btf_id) { - verbose(env, "verifier internal error: missing BTF ID\n"); - return -EFAULT; - } - - if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id, *btf_id)) { - verbose(env, "R%d is of type %s but %s is expected\n", - regno, kernel_type_name(reg->btf_id), kernel_type_name(*btf_id)); - return -EACCES; - } - if (!tnum_is_const(reg->var_off) || reg->var_off.value) { - verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n", - regno); - return -EACCES; - } - } else if (type == PTR_TO_CTX) { + if (type == PTR_TO_CTX) { err = check_ctx_reg(env, reg, regno); if (err < 0) return err;
check_reg_type() checks whether a reg can be used as an arg of a func_proto. For PTR_TO_BTF_ID, the check is actually not completely done until the reg->btf_id is pointing to a kernel struct that is acceptable by the func_proto. Thus, this patch moves the btf_id check into check_reg_type(). The compatible_reg_types[] usage is localized in check_reg_type() now. The "if (!btf_id) verbose(...); " is also removed since it won't happen. Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- kernel/bpf/verifier.c | 65 +++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 30 deletions(-)