| Message ID | 20200625221304.2817194-6-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Add d_path helper | expand |
On 6/25/20 3:12 PM, Jiri Olsa wrote: > Now when we moved the helpers btf_id arrays into .BTF_ids section, > we can remove the code that resolve those IDs in runtime. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > kernel/bpf/btf.c | 90 +++++------------------------------------------- > 1 file changed, 8 insertions(+), 82 deletions(-) > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 4c3007f428b1..4da6b0770ff9 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -4079,96 +4079,22 @@ int btf_struct_access(struct bpf_verifier_log *log, > return -EINVAL; > } > > -static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn, > - int arg) > +int btf_resolve_helper_id(struct bpf_verifier_log *log, > + const struct bpf_func_proto *fn, int arg) > { > - char fnname[KSYM_SYMBOL_LEN + 4] = "btf_"; > - const struct btf_param *args; > - const struct btf_type *t; > - const char *tname, *sym; > - u32 btf_id, i; > + int id; > > - if (IS_ERR(btf_vmlinux)) { > - bpf_log(log, "btf_vmlinux is malformed\n"); > + if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID) > return -EINVAL; > - } > - > - sym = kallsyms_lookup((long)fn, NULL, NULL, NULL, fnname + 4); > - if (!sym) { > - bpf_log(log, "kernel doesn't have kallsyms\n"); > - return -EFAULT; > - } > > - for (i = 1; i <= btf_vmlinux->nr_types; i++) { > - t = btf_type_by_id(btf_vmlinux, i); > - if (BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) > - continue; > - tname = __btf_name_by_offset(btf_vmlinux, t->name_off); > - if (!strcmp(tname, fnname)) > - break; > - } > - if (i > btf_vmlinux->nr_types) { > - bpf_log(log, "helper %s type is not found\n", fnname); > - return -ENOENT; > - } > - > - t = btf_type_by_id(btf_vmlinux, t->type); > - if (!btf_type_is_ptr(t)) > - return -EFAULT; > - t = btf_type_by_id(btf_vmlinux, t->type); > - if (!btf_type_is_func_proto(t)) > - return -EFAULT; > - > - args = (const struct btf_param *)(t + 1); > - if (arg >= btf_type_vlen(t)) { > - bpf_log(log, "bpf helper %s doesn't have %d-th argument\n", > - fnname, arg); > + if (WARN_ON_ONCE(!fn->btf_id)) The original code does not have this warning. It directly did "ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID. > return -EINVAL; > - } > > - t = btf_type_by_id(btf_vmlinux, args[arg].type); > - if (!btf_type_is_ptr(t) || !t->type) { > - /* anything but the pointer to struct is a helper config bug */ > - bpf_log(log, "ARG_PTR_TO_BTF is misconfigured\n"); > - return -EFAULT; > - } > - btf_id = t->type; > - t = btf_type_by_id(btf_vmlinux, t->type); > - /* skip modifiers */ > - while (btf_type_is_modifier(t)) { > - btf_id = t->type; > - t = btf_type_by_id(btf_vmlinux, t->type); > - } > - if (!btf_type_is_struct(t)) { > - bpf_log(log, "ARG_PTR_TO_BTF is not a struct\n"); > - return -EFAULT; > - } > - bpf_log(log, "helper %s arg%d has btf_id %d struct %s\n", fnname + 4, > - arg, btf_id, __btf_name_by_offset(btf_vmlinux, t->name_off)); > - return btf_id; > -} > + id = fn->btf_id[arg]; The corresponding BTF_ID definition here is: BTF_ID_LIST(bpf_skb_output_btf_ids) BTF_ID(struct, sk_buff) The bpf helper writer needs to ensure proper declarations of BTF_IDs like the above matching helpers definition. Support we have arg1 and arg3 as BTF_ID. then the list definition may be BTF_ID_LIST(bpf_skb_output_btf_ids) BTF_ID(struct, sk_buff) BTF_ID(struct, __unused) BTF_ID(struct, task_struct) This probably okay, I guess. > > -int btf_resolve_helper_id(struct bpf_verifier_log *log, > - const struct bpf_func_proto *fn, int arg) > -{ > - int *btf_id = &fn->btf_id[arg]; > - int ret; > - > - if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID) > + if (!id || id > btf_vmlinux->nr_types) > return -EINVAL; id == 0 if btf_id cannot be resolved by resolve_btfids, right? when id may be greater than btf_vmlinux->nr_types? If resolve_btfids application did incorrect transformation? Anyway, this is to resolve helper meta btf_id. Even if you return a btf_id > btf_vmlinux->nr_types, verifier will reject since it will never be the same as the real parameter btf_id. I would drop id > btf_vmlinux->nr_types here. This should never happen for a correct tool. Even if it does, verifier will take care of it. > - > - ret = READ_ONCE(*btf_id); > - if (ret) > - return ret; > - /* ok to race the search. The result is the same */ > - ret = __btf_resolve_helper_id(log, fn->func, arg); > - if (!ret) { > - /* Function argument cannot be type 'void' */ > - bpf_log(log, "BTF resolution bug\n"); > - return -EFAULT; > - } > - WRITE_ONCE(*btf_id, ret); > - return ret; > + return id; > } > > static int __get_type_size(struct btf *btf, u32 btf_id, >
On Fri, Jun 26, 2020 at 2:37 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 6/25/20 3:12 PM, Jiri Olsa wrote: > > Now when we moved the helpers btf_id arrays into .BTF_ids section, > > we can remove the code that resolve those IDs in runtime. > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > kernel/bpf/btf.c | 90 +++++------------------------------------------- > > 1 file changed, 8 insertions(+), 82 deletions(-) > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index 4c3007f428b1..4da6b0770ff9 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -4079,96 +4079,22 @@ int btf_struct_access(struct bpf_verifier_log *log, > > return -EINVAL; > > } > > > > -static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn, > > - int arg) > > +int btf_resolve_helper_id(struct bpf_verifier_log *log, > > + const struct bpf_func_proto *fn, int arg) > > { > > - char fnname[KSYM_SYMBOL_LEN + 4] = "btf_"; > > - const struct btf_param *args; > > - const struct btf_type *t; > > - const char *tname, *sym; > > - u32 btf_id, i; > > + int id; > > > > - if (IS_ERR(btf_vmlinux)) { > > - bpf_log(log, "btf_vmlinux is malformed\n"); > > + if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID) > > return -EINVAL; > > - } > > - > > - sym = kallsyms_lookup((long)fn, NULL, NULL, NULL, fnname + 4); > > - if (!sym) { > > - bpf_log(log, "kernel doesn't have kallsyms\n"); > > - return -EFAULT; > > - } > > > > - for (i = 1; i <= btf_vmlinux->nr_types; i++) { > > - t = btf_type_by_id(btf_vmlinux, i); > > - if (BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) > > - continue; > > - tname = __btf_name_by_offset(btf_vmlinux, t->name_off); > > - if (!strcmp(tname, fnname)) > > - break; > > - } > > - if (i > btf_vmlinux->nr_types) { > > - bpf_log(log, "helper %s type is not found\n", fnname); > > - return -ENOENT; > > - } > > - > > - t = btf_type_by_id(btf_vmlinux, t->type); > > - if (!btf_type_is_ptr(t)) > > - return -EFAULT; > > - t = btf_type_by_id(btf_vmlinux, t->type); > > - if (!btf_type_is_func_proto(t)) > > - return -EFAULT; > > - > > - args = (const struct btf_param *)(t + 1); > > - if (arg >= btf_type_vlen(t)) { > > - bpf_log(log, "bpf helper %s doesn't have %d-th argument\n", > > - fnname, arg); > > + if (WARN_ON_ONCE(!fn->btf_id)) > > The original code does not have this warning. It directly did > "ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID. > > > return -EINVAL; > > - } > > > > - t = btf_type_by_id(btf_vmlinux, args[arg].type); > > - if (!btf_type_is_ptr(t) || !t->type) { > > - /* anything but the pointer to struct is a helper config bug */ > > - bpf_log(log, "ARG_PTR_TO_BTF is misconfigured\n"); > > - return -EFAULT; > > - } > > - btf_id = t->type; > > - t = btf_type_by_id(btf_vmlinux, t->type); > > - /* skip modifiers */ > > - while (btf_type_is_modifier(t)) { > > - btf_id = t->type; > > - t = btf_type_by_id(btf_vmlinux, t->type); > > - } > > - if (!btf_type_is_struct(t)) { > > - bpf_log(log, "ARG_PTR_TO_BTF is not a struct\n"); > > - return -EFAULT; > > - } > > - bpf_log(log, "helper %s arg%d has btf_id %d struct %s\n", fnname + 4, > > - arg, btf_id, __btf_name_by_offset(btf_vmlinux, t->name_off)); > > - return btf_id; > > -} > > + id = fn->btf_id[arg]; > > The corresponding BTF_ID definition here is: > BTF_ID_LIST(bpf_skb_output_btf_ids) > BTF_ID(struct, sk_buff) > > The bpf helper writer needs to ensure proper declarations > of BTF_IDs like the above matching helpers definition. > Support we have arg1 and arg3 as BTF_ID. then the list > definition may be > > BTF_ID_LIST(bpf_skb_output_btf_ids) > BTF_ID(struct, sk_buff) > BTF_ID(struct, __unused) > BTF_ID(struct, task_struct) > > This probably okay, I guess. > > > > > -int btf_resolve_helper_id(struct bpf_verifier_log *log, > > - const struct bpf_func_proto *fn, int arg) > > -{ > > - int *btf_id = &fn->btf_id[arg]; > > - int ret; > > - > > - if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID) > > + if (!id || id > btf_vmlinux->nr_types) > > return -EINVAL; > > id == 0 if btf_id cannot be resolved by resolve_btfids, right? > when id may be greater than btf_vmlinux->nr_types? If resolve_btfids > application did incorrect transformation? > > Anyway, this is to resolve helper meta btf_id. Even if you > return a btf_id > btf_vmlinux->nr_types, verifier will reject > since it will never be the same as the real parameter btf_id. > I would drop id > btf_vmlinux->nr_types here. This should never > happen for a correct tool. Even if it does, verifier will take > care of it. > I'd love to hear Alexei's thoughts about this change as well. Jiri removed not just BTF ID resolution, but also all the sanity checks. This now means more trust in helper definitions to not screw up anything. It's probably OK, but still something to consciously think about. > > - > > - ret = READ_ONCE(*btf_id); > > - if (ret) > > - return ret; > > - /* ok to race the search. The result is the same */ > > - ret = __btf_resolve_helper_id(log, fn->func, arg); > > - if (!ret) { > > - /* Function argument cannot be type 'void' */ > > - bpf_log(log, "BTF resolution bug\n"); > > - return -EFAULT; > > - } > > - WRITE_ONCE(*btf_id, ret); > > - return ret; > > + return id; > > } > > > > static int __get_type_size(struct btf *btf, u32 btf_id, > >
On 6/26/20 2:40 PM, Andrii Nakryiko wrote: > On Fri, Jun 26, 2020 at 2:37 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 6/25/20 3:12 PM, Jiri Olsa wrote: >>> Now when we moved the helpers btf_id arrays into .BTF_ids section, >>> we can remove the code that resolve those IDs in runtime. >>> >>> Signed-off-by: Jiri Olsa <jolsa@kernel.org> >>> --- >>> kernel/bpf/btf.c | 90 +++++------------------------------------------- >>> 1 file changed, 8 insertions(+), 82 deletions(-) >>> >>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c >>> index 4c3007f428b1..4da6b0770ff9 100644 >>> --- a/kernel/bpf/btf.c >>> +++ b/kernel/bpf/btf.c >>> @@ -4079,96 +4079,22 @@ int btf_struct_access(struct bpf_verifier_log *log, >>> return -EINVAL; >>> } >>> >>> -static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn, >>> - int arg) >>> +int btf_resolve_helper_id(struct bpf_verifier_log *log, >>> + const struct bpf_func_proto *fn, int arg) >>> { >>> - char fnname[KSYM_SYMBOL_LEN + 4] = "btf_"; >>> - const struct btf_param *args; >>> - const struct btf_type *t; >>> - const char *tname, *sym; >>> - u32 btf_id, i; >>> + int id; >>> >>> - if (IS_ERR(btf_vmlinux)) { >>> - bpf_log(log, "btf_vmlinux is malformed\n"); >>> + if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID) >>> return -EINVAL; >>> - } >>> - >>> - sym = kallsyms_lookup((long)fn, NULL, NULL, NULL, fnname + 4); >>> - if (!sym) { >>> - bpf_log(log, "kernel doesn't have kallsyms\n"); >>> - return -EFAULT; >>> - } >>> >>> - for (i = 1; i <= btf_vmlinux->nr_types; i++) { >>> - t = btf_type_by_id(btf_vmlinux, i); >>> - if (BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) >>> - continue; >>> - tname = __btf_name_by_offset(btf_vmlinux, t->name_off); >>> - if (!strcmp(tname, fnname)) >>> - break; >>> - } >>> - if (i > btf_vmlinux->nr_types) { >>> - bpf_log(log, "helper %s type is not found\n", fnname); >>> - return -ENOENT; >>> - } >>> - >>> - t = btf_type_by_id(btf_vmlinux, t->type); >>> - if (!btf_type_is_ptr(t)) >>> - return -EFAULT; >>> - t = btf_type_by_id(btf_vmlinux, t->type); >>> - if (!btf_type_is_func_proto(t)) >>> - return -EFAULT; >>> - >>> - args = (const struct btf_param *)(t + 1); >>> - if (arg >= btf_type_vlen(t)) { >>> - bpf_log(log, "bpf helper %s doesn't have %d-th argument\n", >>> - fnname, arg); >>> + if (WARN_ON_ONCE(!fn->btf_id)) >> >> The original code does not have this warning. It directly did >> "ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID. >> >>> return -EINVAL; >>> - } >>> >>> - t = btf_type_by_id(btf_vmlinux, args[arg].type); >>> - if (!btf_type_is_ptr(t) || !t->type) { >>> - /* anything but the pointer to struct is a helper config bug */ >>> - bpf_log(log, "ARG_PTR_TO_BTF is misconfigured\n"); >>> - return -EFAULT; >>> - } >>> - btf_id = t->type; >>> - t = btf_type_by_id(btf_vmlinux, t->type); >>> - /* skip modifiers */ >>> - while (btf_type_is_modifier(t)) { >>> - btf_id = t->type; >>> - t = btf_type_by_id(btf_vmlinux, t->type); >>> - } >>> - if (!btf_type_is_struct(t)) { >>> - bpf_log(log, "ARG_PTR_TO_BTF is not a struct\n"); >>> - return -EFAULT; >>> - } >>> - bpf_log(log, "helper %s arg%d has btf_id %d struct %s\n", fnname + 4, >>> - arg, btf_id, __btf_name_by_offset(btf_vmlinux, t->name_off)); >>> - return btf_id; >>> -} >>> + id = fn->btf_id[arg]; >> >> The corresponding BTF_ID definition here is: >> BTF_ID_LIST(bpf_skb_output_btf_ids) >> BTF_ID(struct, sk_buff) >> >> The bpf helper writer needs to ensure proper declarations >> of BTF_IDs like the above matching helpers definition. >> Support we have arg1 and arg3 as BTF_ID. then the list >> definition may be >> >> BTF_ID_LIST(bpf_skb_output_btf_ids) >> BTF_ID(struct, sk_buff) >> BTF_ID(struct, __unused) >> BTF_ID(struct, task_struct) >> >> This probably okay, I guess. >> >>> >>> -int btf_resolve_helper_id(struct bpf_verifier_log *log, >>> - const struct bpf_func_proto *fn, int arg) >>> -{ >>> - int *btf_id = &fn->btf_id[arg]; >>> - int ret; >>> - >>> - if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID) >>> + if (!id || id > btf_vmlinux->nr_types) >>> return -EINVAL; >> >> id == 0 if btf_id cannot be resolved by resolve_btfids, right? >> when id may be greater than btf_vmlinux->nr_types? If resolve_btfids >> application did incorrect transformation? >> >> Anyway, this is to resolve helper meta btf_id. Even if you >> return a btf_id > btf_vmlinux->nr_types, verifier will reject >> since it will never be the same as the real parameter btf_id. >> I would drop id > btf_vmlinux->nr_types here. This should never >> happen for a correct tool. Even if it does, verifier will take >> care of it. >> > > I'd love to hear Alexei's thoughts about this change as well. Jiri > removed not just BTF ID resolution, but also all the sanity checks. > This now means more trust in helper definitions to not screw up > anything. It's probably OK, but still something to consciously think > about. The kernel will have to trust the result. Otherwise, things may go bad. For example, if the tool incorrectly calculated the type_id to be 100 although the correct id is 50, and the bpf program happens to have the type id 100 for the parameter. Then the helper is allowed, this could crash the kernel... I do not know whether we have security issue here or not. > >>> - >>> - ret = READ_ONCE(*btf_id); >>> - if (ret) >>> - return ret; >>> - /* ok to race the search. The result is the same */ >>> - ret = __btf_resolve_helper_id(log, fn->func, arg); >>> - if (!ret) { >>> - /* Function argument cannot be type 'void' */ >>> - bpf_log(log, "BTF resolution bug\n"); >>> - return -EFAULT; >>> - } >>> - WRITE_ONCE(*btf_id, ret); >>> - return ret; >>> + return id; >>> } >>> >>> static int __get_type_size(struct btf *btf, u32 btf_id, >>>
On Fri, Jun 26, 2020 at 04:29:23PM -0700, Yonghong Song wrote: > > > > > > > > -int btf_resolve_helper_id(struct bpf_verifier_log *log, > > > > - const struct bpf_func_proto *fn, int arg) > > > > -{ > > > > - int *btf_id = &fn->btf_id[arg]; > > > > - int ret; > > > > - > > > > - if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID) > > > > + if (!id || id > btf_vmlinux->nr_types) > > > > return -EINVAL; > > > > > > id == 0 if btf_id cannot be resolved by resolve_btfids, right? > > > when id may be greater than btf_vmlinux->nr_types? If resolve_btfids > > > application did incorrect transformation? > > > > > > Anyway, this is to resolve helper meta btf_id. Even if you > > > return a btf_id > btf_vmlinux->nr_types, verifier will reject > > > since it will never be the same as the real parameter btf_id. > > > I would drop id > btf_vmlinux->nr_types here. This should never > > > happen for a correct tool. Even if it does, verifier will take > > > care of it. > > > > > > > I'd love to hear Alexei's thoughts about this change as well. Jiri > > removed not just BTF ID resolution, but also all the sanity checks. > > This now means more trust in helper definitions to not screw up > > anything. It's probably OK, but still something to consciously think > > about. > > The kernel will have to trust the result. +1 I think 'if (!id || id > btf_vmlinux->nr_types)' at run-time and other sanity checks I saw in other patches are unnecessary. resolve_btfids should do all checks and fail vmlinux linking. We trust gcc to generate correct assembly code in the first place and correct dwarf. We trust pahole do correct BTF conversion from dwarf and dedup. We should trust resolve_btfids. It's imo the simplest tool comparing to gcc. btf_parse_vmlinux() is doing basic sanity check of BTF mainly to populate 'struct btf *btf_vmlinux;' for further use. I think we can add a scan over resolved btfids to btf_parse_vmlinux() to make sure that all ids are within the range, but I don't think it's mandatory for this patch set. Would be a reasonable sanity check for the future. Of course, checking for sorted set_start/end is overkill. Basic simplest sanity is ok.
On Sun, Jun 28, 2020 at 11:50 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Jun 26, 2020 at 04:29:23PM -0700, Yonghong Song wrote: > > > > > > > > > > -int btf_resolve_helper_id(struct bpf_verifier_log *log, > > > > > - const struct bpf_func_proto *fn, int arg) > > > > > -{ > > > > > - int *btf_id = &fn->btf_id[arg]; > > > > > - int ret; > > > > > - > > > > > - if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID) > > > > > + if (!id || id > btf_vmlinux->nr_types) > > > > > return -EINVAL; > > > > > > > > id == 0 if btf_id cannot be resolved by resolve_btfids, right? > > > > when id may be greater than btf_vmlinux->nr_types? If resolve_btfids > > > > application did incorrect transformation? > > > > > > > > Anyway, this is to resolve helper meta btf_id. Even if you > > > > return a btf_id > btf_vmlinux->nr_types, verifier will reject > > > > since it will never be the same as the real parameter btf_id. > > > > I would drop id > btf_vmlinux->nr_types here. This should never > > > > happen for a correct tool. Even if it does, verifier will take > > > > care of it. > > > > > > > > > > I'd love to hear Alexei's thoughts about this change as well. Jiri > > > removed not just BTF ID resolution, but also all the sanity checks. > > > This now means more trust in helper definitions to not screw up > > > anything. It's probably OK, but still something to consciously think > > > about. > > > > The kernel will have to trust the result. > > +1 > I think 'if (!id || id > btf_vmlinux->nr_types)' at run-time and > other sanity checks I saw in other patches are unnecessary. > resolve_btfids should do all checks and fail vmlinux linking. > We trust gcc to generate correct assembly code in the first place > and correct dwarf. We trust pahole do correct BTF conversion from > dwarf and dedup. We should trust resolve_btfids. > It's imo the simplest tool comparing to gcc. > btf_parse_vmlinux() is doing basic sanity check of BTF mainly > to populate 'struct btf *btf_vmlinux;' for further use. > I think we can add a scan over resolved btfids to btf_parse_vmlinux() > to make sure that all ids are within the range, but I don't think > it's mandatory for this patch set. Would be a reasonable sanity > check for the future. Of course, checking for sorted set_start/end > is overkill. Basic simplest sanity is ok. My point was *not* about trusting resolve_btfids tool, it was about trusting BPF helper definitions. But it's more of help for developer while developing new helpers, so it's not a major thing.
On Fri, Jun 26, 2020 at 02:36:37PM -0700, Yonghong Song wrote: SNIP > > - } > > - > > - t = btf_type_by_id(btf_vmlinux, t->type); > > - if (!btf_type_is_ptr(t)) > > - return -EFAULT; > > - t = btf_type_by_id(btf_vmlinux, t->type); > > - if (!btf_type_is_func_proto(t)) > > - return -EFAULT; > > - > > - args = (const struct btf_param *)(t + 1); > > - if (arg >= btf_type_vlen(t)) { > > - bpf_log(log, "bpf helper %s doesn't have %d-th argument\n", > > - fnname, arg); > > + if (WARN_ON_ONCE(!fn->btf_id)) > > The original code does not have this warning. It directly did > "ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID. not sure why I put it in there, it's probably enough guarded by arg_type having ARG_PTR_TO_BTF_ID, will remove > > > return -EINVAL; > > - } > > - t = btf_type_by_id(btf_vmlinux, args[arg].type); > > - if (!btf_type_is_ptr(t) || !t->type) { > > - /* anything but the pointer to struct is a helper config bug */ > > - bpf_log(log, "ARG_PTR_TO_BTF is misconfigured\n"); > > - return -EFAULT; > > - } > > - btf_id = t->type; > > - t = btf_type_by_id(btf_vmlinux, t->type); > > - /* skip modifiers */ > > - while (btf_type_is_modifier(t)) { > > - btf_id = t->type; > > - t = btf_type_by_id(btf_vmlinux, t->type); > > - } > > - if (!btf_type_is_struct(t)) { > > - bpf_log(log, "ARG_PTR_TO_BTF is not a struct\n"); > > - return -EFAULT; > > - } > > - bpf_log(log, "helper %s arg%d has btf_id %d struct %s\n", fnname + 4, > > - arg, btf_id, __btf_name_by_offset(btf_vmlinux, t->name_off)); > > - return btf_id; > > -} > > + id = fn->btf_id[arg]; > > The corresponding BTF_ID definition here is: > BTF_ID_LIST(bpf_skb_output_btf_ids) > BTF_ID(struct, sk_buff) > > The bpf helper writer needs to ensure proper declarations > of BTF_IDs like the above matching helpers definition. > Support we have arg1 and arg3 as BTF_ID. then the list > definition may be > > BTF_ID_LIST(bpf_skb_output_btf_ids) > BTF_ID(struct, sk_buff) > BTF_ID(struct, __unused) > BTF_ID(struct, task_struct) > > This probably okay, I guess. right, AFAIK we don't have such case yet, but would be good to be ready and have something like BTF_ID(struct, __unused) maybe adding new type for that will be better: BTF_ID(none, unused) jirka
On 6/28/20 1:16 PM, Jiri Olsa wrote: > On Fri, Jun 26, 2020 at 02:36:37PM -0700, Yonghong Song wrote: > > SNIP > >>> - } >>> - >>> - t = btf_type_by_id(btf_vmlinux, t->type); >>> - if (!btf_type_is_ptr(t)) >>> - return -EFAULT; >>> - t = btf_type_by_id(btf_vmlinux, t->type); >>> - if (!btf_type_is_func_proto(t)) >>> - return -EFAULT; >>> - >>> - args = (const struct btf_param *)(t + 1); >>> - if (arg >= btf_type_vlen(t)) { >>> - bpf_log(log, "bpf helper %s doesn't have %d-th argument\n", >>> - fnname, arg); >>> + if (WARN_ON_ONCE(!fn->btf_id)) >> >> The original code does not have this warning. It directly did >> "ret = READ_ONCE(*btf_id);" after testing reg arg type ARG_PTR_TO_BTF_ID. > > not sure why I put it in there, it's probably enough guarded > by arg_type having ARG_PTR_TO_BTF_ID, will remove > >> >>> return -EINVAL; >>> - } >>> - t = btf_type_by_id(btf_vmlinux, args[arg].type); >>> - if (!btf_type_is_ptr(t) || !t->type) { >>> - /* anything but the pointer to struct is a helper config bug */ >>> - bpf_log(log, "ARG_PTR_TO_BTF is misconfigured\n"); >>> - return -EFAULT; >>> - } >>> - btf_id = t->type; >>> - t = btf_type_by_id(btf_vmlinux, t->type); >>> - /* skip modifiers */ >>> - while (btf_type_is_modifier(t)) { >>> - btf_id = t->type; >>> - t = btf_type_by_id(btf_vmlinux, t->type); >>> - } >>> - if (!btf_type_is_struct(t)) { >>> - bpf_log(log, "ARG_PTR_TO_BTF is not a struct\n"); >>> - return -EFAULT; >>> - } >>> - bpf_log(log, "helper %s arg%d has btf_id %d struct %s\n", fnname + 4, >>> - arg, btf_id, __btf_name_by_offset(btf_vmlinux, t->name_off)); >>> - return btf_id; >>> -} >>> + id = fn->btf_id[arg]; >> >> The corresponding BTF_ID definition here is: >> BTF_ID_LIST(bpf_skb_output_btf_ids) >> BTF_ID(struct, sk_buff) >> >> The bpf helper writer needs to ensure proper declarations >> of BTF_IDs like the above matching helpers definition. >> Support we have arg1 and arg3 as BTF_ID. then the list >> definition may be >> >> BTF_ID_LIST(bpf_skb_output_btf_ids) >> BTF_ID(struct, sk_buff) >> BTF_ID(struct, __unused) >> BTF_ID(struct, task_struct) >> >> This probably okay, I guess. > > right, AFAIK we don't have such case yet, but would be good > to be ready and have something like > > BTF_ID(struct, __unused) > > maybe adding new type for that will be better: > > BTF_ID(none, unused) Maybe we can have a separate macro BTF_ID_UNUSED macro which simply adds 4 bytes hole in the .btf_ids* section. > > jirka >
On Sun, Jun 28, 2020 at 01:59:54PM -0700, Yonghong Song wrote: SNIP > > > > > > The corresponding BTF_ID definition here is: > > > BTF_ID_LIST(bpf_skb_output_btf_ids) > > > BTF_ID(struct, sk_buff) > > > > > > The bpf helper writer needs to ensure proper declarations > > > of BTF_IDs like the above matching helpers definition. > > > Support we have arg1 and arg3 as BTF_ID. then the list > > > definition may be > > > > > > BTF_ID_LIST(bpf_skb_output_btf_ids) > > > BTF_ID(struct, sk_buff) > > > BTF_ID(struct, __unused) > > > BTF_ID(struct, task_struct) > > > > > > This probably okay, I guess. > > > > right, AFAIK we don't have such case yet, but would be good > > to be ready and have something like > > > > BTF_ID(struct, __unused) > > > > maybe adding new type for that will be better: > > > > BTF_ID(none, unused) > > Maybe we can have a separate macro BTF_ID_UNUSED macro > which simply adds 4 bytes hole in the .btf_ids* section. right, we don't need symbols for that jirka
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 4c3007f428b1..4da6b0770ff9 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -4079,96 +4079,22 @@ int btf_struct_access(struct bpf_verifier_log *log, return -EINVAL; } -static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn, - int arg) +int btf_resolve_helper_id(struct bpf_verifier_log *log, + const struct bpf_func_proto *fn, int arg) { - char fnname[KSYM_SYMBOL_LEN + 4] = "btf_"; - const struct btf_param *args; - const struct btf_type *t; - const char *tname, *sym; - u32 btf_id, i; + int id; - if (IS_ERR(btf_vmlinux)) { - bpf_log(log, "btf_vmlinux is malformed\n"); + if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID) return -EINVAL; - } - - sym = kallsyms_lookup((long)fn, NULL, NULL, NULL, fnname + 4); - if (!sym) { - bpf_log(log, "kernel doesn't have kallsyms\n"); - return -EFAULT; - } - for (i = 1; i <= btf_vmlinux->nr_types; i++) { - t = btf_type_by_id(btf_vmlinux, i); - if (BTF_INFO_KIND(t->info) != BTF_KIND_TYPEDEF) - continue; - tname = __btf_name_by_offset(btf_vmlinux, t->name_off); - if (!strcmp(tname, fnname)) - break; - } - if (i > btf_vmlinux->nr_types) { - bpf_log(log, "helper %s type is not found\n", fnname); - return -ENOENT; - } - - t = btf_type_by_id(btf_vmlinux, t->type); - if (!btf_type_is_ptr(t)) - return -EFAULT; - t = btf_type_by_id(btf_vmlinux, t->type); - if (!btf_type_is_func_proto(t)) - return -EFAULT; - - args = (const struct btf_param *)(t + 1); - if (arg >= btf_type_vlen(t)) { - bpf_log(log, "bpf helper %s doesn't have %d-th argument\n", - fnname, arg); + if (WARN_ON_ONCE(!fn->btf_id)) return -EINVAL; - } - t = btf_type_by_id(btf_vmlinux, args[arg].type); - if (!btf_type_is_ptr(t) || !t->type) { - /* anything but the pointer to struct is a helper config bug */ - bpf_log(log, "ARG_PTR_TO_BTF is misconfigured\n"); - return -EFAULT; - } - btf_id = t->type; - t = btf_type_by_id(btf_vmlinux, t->type); - /* skip modifiers */ - while (btf_type_is_modifier(t)) { - btf_id = t->type; - t = btf_type_by_id(btf_vmlinux, t->type); - } - if (!btf_type_is_struct(t)) { - bpf_log(log, "ARG_PTR_TO_BTF is not a struct\n"); - return -EFAULT; - } - bpf_log(log, "helper %s arg%d has btf_id %d struct %s\n", fnname + 4, - arg, btf_id, __btf_name_by_offset(btf_vmlinux, t->name_off)); - return btf_id; -} + id = fn->btf_id[arg]; -int btf_resolve_helper_id(struct bpf_verifier_log *log, - const struct bpf_func_proto *fn, int arg) -{ - int *btf_id = &fn->btf_id[arg]; - int ret; - - if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID) + if (!id || id > btf_vmlinux->nr_types) return -EINVAL; - - ret = READ_ONCE(*btf_id); - if (ret) - return ret; - /* ok to race the search. The result is the same */ - ret = __btf_resolve_helper_id(log, fn->func, arg); - if (!ret) { - /* Function argument cannot be type 'void' */ - bpf_log(log, "BTF resolution bug\n"); - return -EFAULT; - } - WRITE_ONCE(*btf_id, ret); - return ret; + return id; } static int __get_type_size(struct btf *btf, u32 btf_id,
Now when we moved the helpers btf_id arrays into .BTF_ids section, we can remove the code that resolve those IDs in runtime. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- kernel/bpf/btf.c | 90 +++++------------------------------------------- 1 file changed, 8 insertions(+), 82 deletions(-)