| Message ID | 20230912233214.1518551-16-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7e2925f6723702bcfcfdf8f73d5e85f7514d4b9f |
| Delegated to: | BPF |
| Headers | show |
| Series | Exceptions - 1/2 | expand |
On Tue, Sep 12, 2023 at 4:32 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > Add support to libbpf to append exception callbacks when loading a > program. The exception callback is found by discovering the declaration > tag 'exception_callback:<value>' and finding the callback in the value > of the tag. > > The process is done in two steps. First, for each main program, the > bpf_object__sanitize_and_load_btf function finds and marks its > corresponding exception callback as defined by the declaration tag on > it. Second, bpf_object__reloc_code is modified to append the indicated > exception callback at the end of the instruction iteration (since > exception callback will never be appended in that loop, as it is not > directly referenced). > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > tools/lib/bpf/libbpf.c | 114 +++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 109 insertions(+), 5 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index afc07a8f7dc7..3a6108e3238b 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -436,9 +436,11 @@ struct bpf_program { > int fd; > bool autoload; > bool autoattach; > + bool sym_global; > bool mark_btf_static; > enum bpf_prog_type type; > enum bpf_attach_type expected_attach_type; > + int exception_cb_idx; > > int prog_ifindex; > __u32 attach_btf_obj_fd; > @@ -765,6 +767,7 @@ bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog, > > prog->type = BPF_PROG_TYPE_UNSPEC; > prog->fd = -1; > + prog->exception_cb_idx = -1; > > /* libbpf's convention for SEC("?abc...") is that it's just like > * SEC("abc...") but the corresponding bpf_program starts out with > @@ -871,14 +874,16 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, > if (err) > return err; > > + if (ELF64_ST_BIND(sym->st_info) != STB_LOCAL) > + prog->sym_global = true; > + > /* if function is a global/weak symbol, but has restricted > * (STV_HIDDEN or STV_INTERNAL) visibility, mark its BTF FUNC > * as static to enable more permissive BPF verification mode > * with more outside context available to BPF verifier > */ > - if (ELF64_ST_BIND(sym->st_info) != STB_LOCAL > - && (ELF64_ST_VISIBILITY(sym->st_other) == STV_HIDDEN > - || ELF64_ST_VISIBILITY(sym->st_other) == STV_INTERNAL)) > + if (prog->sym_global && (ELF64_ST_VISIBILITY(sym->st_other) == STV_HIDDEN > + || ELF64_ST_VISIBILITY(sym->st_other) == STV_INTERNAL)) > prog->mark_btf_static = true; > > nr_progs++; > @@ -3142,6 +3147,86 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj) > } > } > > + if (!kernel_supports(obj, FEAT_BTF_DECL_TAG)) > + goto skip_exception_cb; > + for (i = 0; i < obj->nr_programs; i++) { I'm not sure why you chose to do these very inefficient three nested for loops, tbh. Can you please send a follow up patch to make this a bit more sane? There is no reason to iterate over BTF multiple times. In general BPF object's BTF can have tons of information (especially with vmlinux.h), so minimizing unnecessary linear searches here is worth doing. How about this structure: for each btf type in btf: if not decl_tag and not "exception_callback:" one, continue prog_name = <find from decl_tag's referenced func> subprog_name = <find from decl_Tag's name> prog = find_by_name(prog_name); subprog = find_by_name(subprog_name); <check conditions> <remember idx; if it's already set, emit human-readable error and exit, don't rely on BPF verifier to complain > Thanks. > + struct bpf_program *prog = &obj->programs[i]; > + int j, k, n; > + > + if (prog_is_subprog(obj, prog)) > + continue; > + n = btf__type_cnt(obj->btf); > + for (j = 1; j < n; j++) { > + const char *str = "exception_callback:", *name; > + size_t len = strlen(str); > + struct btf_type *t; > + > + t = btf_type_by_id(obj->btf, j); > + if (!btf_is_decl_tag(t) || btf_decl_tag(t)->component_idx != -1) > + continue; > + > + name = btf__str_by_offset(obj->btf, t->name_off); > + if (strncmp(name, str, len)) > + continue; > + > + t = btf_type_by_id(obj->btf, t->type); > + if (!btf_is_func(t) || btf_func_linkage(t) != BTF_FUNC_GLOBAL) { > + pr_warn("prog '%s': exception_callback:<value> decl tag not applied to the main program\n", > + prog->name); > + return -EINVAL; > + } > + if (strcmp(prog->name, btf__str_by_offset(obj->btf, t->name_off))) > + continue; > + /* Multiple callbacks are specified for the same prog, > + * the verifier will eventually return an error for this > + * case, hence simply skip appending a subprog. > + */ > + if (prog->exception_cb_idx >= 0) { > + prog->exception_cb_idx = -1; > + break; > + } you check this condition three times and handle it in three different ways, it's bizarre. Why? > + > + name += len; > + if (str_is_empty(name)) { > + pr_warn("prog '%s': exception_callback:<value> decl tag contains empty value\n", > + prog->name); > + return -EINVAL; > + } > + > + for (k = 0; k < obj->nr_programs; k++) { > + struct bpf_program *subprog = &obj->programs[k]; > + > + if (!prog_is_subprog(obj, subprog)) > + continue; > + if (strcmp(name, subprog->name)) > + continue; > + /* Enforce non-hidden, as from verifier point of > + * view it expects global functions, whereas the > + * mark_btf_static fixes up linkage as static. > + */ > + if (!subprog->sym_global || subprog->mark_btf_static) { > + pr_warn("prog '%s': exception callback %s must be a global non-hidden function\n", > + prog->name, subprog->name); > + return -EINVAL; > + } > + /* Let's see if we already saw a static exception callback with the same name */ > + if (prog->exception_cb_idx >= 0) { > + pr_warn("prog '%s': multiple subprogs with same name as exception callback '%s'\n", > + prog->name, subprog->name); > + return -EINVAL; > + } > + prog->exception_cb_idx = k; > + break; > + } > + > + if (prog->exception_cb_idx >= 0) > + continue; > + pr_warn("prog '%s': cannot find exception callback '%s'\n", prog->name, name); > + return -ENOENT; > + } > + } > +skip_exception_cb: > + > sanitize = btf_needs_sanitization(obj); > if (sanitize) { > const void *raw_data; > @@ -6270,10 +6355,10 @@ static int > bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog, > struct bpf_program *prog) > { > - size_t sub_insn_idx, insn_idx, new_cnt; > + size_t sub_insn_idx, insn_idx; > struct bpf_program *subprog; > - struct bpf_insn *insns, *insn; > struct reloc_desc *relo; > + struct bpf_insn *insn; > int err; > > err = reloc_prog_func_and_line_info(obj, main_prog, prog); > @@ -6582,6 +6667,25 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path) > prog->name, err); > return err; > } > + > + /* Now, also append exception callback if it has not been done already. */ > + if (prog->exception_cb_idx >= 0) { > + struct bpf_program *subprog = &obj->programs[prog->exception_cb_idx]; > + > + /* Calling exception callback directly is disallowed, which the > + * verifier will reject later. In case it was processed already, > + * we can skip this step, otherwise for all other valid cases we > + * have to append exception callback now. > + */ > + if (subprog->sub_insn_off == 0) { > + err = bpf_object__append_subprog_code(obj, prog, subprog); > + if (err) > + return err; > + err = bpf_object__reloc_code(obj, prog, subprog); > + if (err) > + return err; > + } > + } > } > /* Process data relos for main programs */ > for (i = 0; i < obj->nr_programs; i++) { > -- > 2.41.0 >
Hi Andrii, On Wed, 20 Sept 2023 at 02:25, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Sep 12, 2023 at 4:32 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > Add support to libbpf to append exception callbacks when loading a > > program. The exception callback is found by discovering the declaration > > tag 'exception_callback:<value>' and finding the callback in the value > > of the tag. > > > > The process is done in two steps. First, for each main program, the > > bpf_object__sanitize_and_load_btf function finds and marks its > > corresponding exception callback as defined by the declaration tag on > > it. Second, bpf_object__reloc_code is modified to append the indicated > > exception callback at the end of the instruction iteration (since > > exception callback will never be appended in that loop, as it is not > > directly referenced). > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > tools/lib/bpf/libbpf.c | 114 +++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 109 insertions(+), 5 deletions(-) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index afc07a8f7dc7..3a6108e3238b 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -436,9 +436,11 @@ struct bpf_program { > > int fd; > > bool autoload; > > bool autoattach; > > + bool sym_global; > > bool mark_btf_static; > > enum bpf_prog_type type; > > enum bpf_attach_type expected_attach_type; > > + int exception_cb_idx; > > > > int prog_ifindex; > > __u32 attach_btf_obj_fd; > > @@ -765,6 +767,7 @@ bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog, > > > > prog->type = BPF_PROG_TYPE_UNSPEC; > > prog->fd = -1; > > + prog->exception_cb_idx = -1; > > > > /* libbpf's convention for SEC("?abc...") is that it's just like > > * SEC("abc...") but the corresponding bpf_program starts out with > > @@ -871,14 +874,16 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, > > if (err) > > return err; > > > > + if (ELF64_ST_BIND(sym->st_info) != STB_LOCAL) > > + prog->sym_global = true; > > + > > /* if function is a global/weak symbol, but has restricted > > * (STV_HIDDEN or STV_INTERNAL) visibility, mark its BTF FUNC > > * as static to enable more permissive BPF verification mode > > * with more outside context available to BPF verifier > > */ > > - if (ELF64_ST_BIND(sym->st_info) != STB_LOCAL > > - && (ELF64_ST_VISIBILITY(sym->st_other) == STV_HIDDEN > > - || ELF64_ST_VISIBILITY(sym->st_other) == STV_INTERNAL)) > > + if (prog->sym_global && (ELF64_ST_VISIBILITY(sym->st_other) == STV_HIDDEN > > + || ELF64_ST_VISIBILITY(sym->st_other) == STV_INTERNAL)) > > prog->mark_btf_static = true; > > > > nr_progs++; > > @@ -3142,6 +3147,86 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj) > > } > > } > > > > + if (!kernel_supports(obj, FEAT_BTF_DECL_TAG)) > > + goto skip_exception_cb; > > + for (i = 0; i < obj->nr_programs; i++) { > > I'm not sure why you chose to do these very inefficient three nested > for loops, tbh. Can you please send a follow up patch to make this a > bit more sane? There is no reason to iterate over BTF multiple times. > In general BPF object's BTF can have tons of information (especially > with vmlinux.h), so minimizing unnecessary linear searches here is > worth doing. > > How about this structure: > > > for each btf type in btf: > if not decl_tag and not "exception_callback:" one, continue > > prog_name = <find from decl_tag's referenced func> > subprog_name = <find from decl_Tag's name> > > prog = find_by_name(prog_name); > subprog = find_by_name(subprog_name); > > <check conditions> > > <remember idx; if it's already set, emit human-readable error and > exit, don't rely on BPF verifier to complain > > > Thanks. > Yes, I think this looks better. I will rework and send a follow up fix. I was actually under the impression (based on dumping BTF of objects in selftests) that usually the count is somewhere like 30 or 100 for user BTFs. Even when vmlinux.h is included the unused types are dropped from the BTF. So I didn't pay much attention to looping over the user BTF over and over. But I do see in some objects it is up to 500 and I guess it goes higher in huge BPF objects that have a lot of programs (or many objects linked together). > > + struct bpf_program *prog = &obj->programs[i]; > > + int j, k, n; > > + > > + if (prog_is_subprog(obj, prog)) > > + continue; > > + n = btf__type_cnt(obj->btf); > > + for (j = 1; j < n; j++) { > > + const char *str = "exception_callback:", *name; > > + size_t len = strlen(str); > > + struct btf_type *t; > > + > > + t = btf_type_by_id(obj->btf, j); > > + if (!btf_is_decl_tag(t) || btf_decl_tag(t)->component_idx != -1) > > + continue; > > + > > + name = btf__str_by_offset(obj->btf, t->name_off); > > + if (strncmp(name, str, len)) > > + continue; > > + > > + t = btf_type_by_id(obj->btf, t->type); > > + if (!btf_is_func(t) || btf_func_linkage(t) != BTF_FUNC_GLOBAL) { > > + pr_warn("prog '%s': exception_callback:<value> decl tag not applied to the main program\n", > > + prog->name); > > + return -EINVAL; > > + } > > + if (strcmp(prog->name, btf__str_by_offset(obj->btf, t->name_off))) > > + continue; > > + /* Multiple callbacks are specified for the same prog, > > + * the verifier will eventually return an error for this > > + * case, hence simply skip appending a subprog. > > + */ > > + if (prog->exception_cb_idx >= 0) { > > + prog->exception_cb_idx = -1; > > + break; > > + } > > you check this condition three times and handle it in three different > ways, it's bizarre. Why? > I agree it looks confusing. The first check happens when for a given main program, we are going through all types and we already saw a exception cb satisfying the conditions previously. The second one is to catch multiple subprogs that are static and have the same name. So in the loop with k as iterator, if we already found a satisfying subprog, we still continue to catch other cases by matching on the name and linkage. The third one is to just check whether the loop over subprogs for a given main prog actually set the exception_cb_idx or not, otherwise we could not find a subprog with the target name in the decl tag string. I hope this clears up some confusion. But I will rework it as you suggested. It's very late here today but I can send it out tomorrow. > > > + > > + name += len; > > + if (str_is_empty(name)) { > > + pr_warn("prog '%s': exception_callback:<value> decl tag contains empty value\n", > > + prog->name); > > + return -EINVAL; > > + } > > + > > + for (k = 0; k < obj->nr_programs; k++) { > > + struct bpf_program *subprog = &obj->programs[k]; > > + > > + if (!prog_is_subprog(obj, subprog)) > > + continue; > > + if (strcmp(name, subprog->name)) > > + continue; > > + /* Enforce non-hidden, as from verifier point of > > + * view it expects global functions, whereas the > > + * mark_btf_static fixes up linkage as static. > > + */ > > + if (!subprog->sym_global || subprog->mark_btf_static) { > > + pr_warn("prog '%s': exception callback %s must be a global non-hidden function\n", > > + prog->name, subprog->name); > > + return -EINVAL; > > + } > > + /* Let's see if we already saw a static exception callback with the same name */ > > + if (prog->exception_cb_idx >= 0) { > > + pr_warn("prog '%s': multiple subprogs with same name as exception callback '%s'\n", > > + prog->name, subprog->name); > > + return -EINVAL; > > + } > > + prog->exception_cb_idx = k; > > + break; > > + } > > + > > + if (prog->exception_cb_idx >= 0) > > + continue; > > + pr_warn("prog '%s': cannot find exception callback '%s'\n", prog->name, name); > > + return -ENOENT; > > + } > > + } > > +skip_exception_cb: > > + > > sanitize = btf_needs_sanitization(obj); > > if (sanitize) { > > const void *raw_data; > > @@ -6270,10 +6355,10 @@ static int > > bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog, > > struct bpf_program *prog) > > { > > - size_t sub_insn_idx, insn_idx, new_cnt; > > + size_t sub_insn_idx, insn_idx; > > struct bpf_program *subprog; > > - struct bpf_insn *insns, *insn; > > struct reloc_desc *relo; > > + struct bpf_insn *insn; > > int err; > > > > err = reloc_prog_func_and_line_info(obj, main_prog, prog); > > @@ -6582,6 +6667,25 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path) > > prog->name, err); > > return err; > > } > > + > > + /* Now, also append exception callback if it has not been done already. */ > > + if (prog->exception_cb_idx >= 0) { > > + struct bpf_program *subprog = &obj->programs[prog->exception_cb_idx]; > > + > > + /* Calling exception callback directly is disallowed, which the > > + * verifier will reject later. In case it was processed already, > > + * we can skip this step, otherwise for all other valid cases we > > + * have to append exception callback now. > > + */ > > + if (subprog->sub_insn_off == 0) { > > + err = bpf_object__append_subprog_code(obj, prog, subprog); > > + if (err) > > + return err; > > + err = bpf_object__reloc_code(obj, prog, subprog); > > + if (err) > > + return err; > > + } > > + } > > } > > /* Process data relos for main programs */ > > for (i = 0; i < obj->nr_programs; i++) { > > -- > > 2.41.0 > >
On Tue, Sep 19, 2023 at 6:03 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > Hi Andrii, > > On Wed, 20 Sept 2023 at 02:25, Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Sep 12, 2023 at 4:32 PM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > > > > > > Add support to libbpf to append exception callbacks when loading a > > > program. The exception callback is found by discovering the declaration > > > tag 'exception_callback:<value>' and finding the callback in the value > > > of the tag. > > > > > > The process is done in two steps. First, for each main program, the > > > bpf_object__sanitize_and_load_btf function finds and marks its > > > corresponding exception callback as defined by the declaration tag on > > > it. Second, bpf_object__reloc_code is modified to append the indicated > > > exception callback at the end of the instruction iteration (since > > > exception callback will never be appended in that loop, as it is not > > > directly referenced). > > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > --- > > > tools/lib/bpf/libbpf.c | 114 +++++++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 109 insertions(+), 5 deletions(-) > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > index afc07a8f7dc7..3a6108e3238b 100644 > > > --- a/tools/lib/bpf/libbpf.c > > > +++ b/tools/lib/bpf/libbpf.c > > > @@ -436,9 +436,11 @@ struct bpf_program { > > > int fd; > > > bool autoload; > > > bool autoattach; > > > + bool sym_global; > > > bool mark_btf_static; > > > enum bpf_prog_type type; > > > enum bpf_attach_type expected_attach_type; > > > + int exception_cb_idx; > > > > > > int prog_ifindex; > > > __u32 attach_btf_obj_fd; > > > @@ -765,6 +767,7 @@ bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog, > > > > > > prog->type = BPF_PROG_TYPE_UNSPEC; > > > prog->fd = -1; > > > + prog->exception_cb_idx = -1; > > > > > > /* libbpf's convention for SEC("?abc...") is that it's just like > > > * SEC("abc...") but the corresponding bpf_program starts out with > > > @@ -871,14 +874,16 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, > > > if (err) > > > return err; > > > > > > + if (ELF64_ST_BIND(sym->st_info) != STB_LOCAL) > > > + prog->sym_global = true; > > > + > > > /* if function is a global/weak symbol, but has restricted > > > * (STV_HIDDEN or STV_INTERNAL) visibility, mark its BTF FUNC > > > * as static to enable more permissive BPF verification mode > > > * with more outside context available to BPF verifier > > > */ > > > - if (ELF64_ST_BIND(sym->st_info) != STB_LOCAL > > > - && (ELF64_ST_VISIBILITY(sym->st_other) == STV_HIDDEN > > > - || ELF64_ST_VISIBILITY(sym->st_other) == STV_INTERNAL)) > > > + if (prog->sym_global && (ELF64_ST_VISIBILITY(sym->st_other) == STV_HIDDEN > > > + || ELF64_ST_VISIBILITY(sym->st_other) == STV_INTERNAL)) > > > prog->mark_btf_static = true; > > > > > > nr_progs++; > > > @@ -3142,6 +3147,86 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj) > > > } > > > } > > > > > > + if (!kernel_supports(obj, FEAT_BTF_DECL_TAG)) > > > + goto skip_exception_cb; > > > + for (i = 0; i < obj->nr_programs; i++) { > > > > I'm not sure why you chose to do these very inefficient three nested > > for loops, tbh. Can you please send a follow up patch to make this a > > bit more sane? There is no reason to iterate over BTF multiple times. > > In general BPF object's BTF can have tons of information (especially > > with vmlinux.h), so minimizing unnecessary linear searches here is > > worth doing. > > > > How about this structure: > > > > > > for each btf type in btf: > > if not decl_tag and not "exception_callback:" one, continue > > > > prog_name = <find from decl_tag's referenced func> > > subprog_name = <find from decl_Tag's name> > > > > prog = find_by_name(prog_name); > > subprog = find_by_name(subprog_name); > > > > <check conditions> > > > > <remember idx; if it's already set, emit human-readable error and > > exit, don't rely on BPF verifier to complain > > > > > Thanks. > > > > Yes, I think this looks better. I will rework and send a follow up fix. > I was actually under the impression (based on dumping BTF of objects > in selftests) that usually the count is somewhere like 30 or 100 for > user BTFs. > Even when vmlinux.h is included the unused types are dropped from the > BTF. So I didn't pay much attention to looping over the user BTF over > and over. > But I do see in some objects it is up to 500 and I guess it goes > higher in huge BPF objects that have a lot of programs (or many > objects linked together). Right. And I think it's just more straightforward to follow as well. > > > > + struct bpf_program *prog = &obj->programs[i]; > > > + int j, k, n; > > > + > > > + if (prog_is_subprog(obj, prog)) > > > + continue; > > > + n = btf__type_cnt(obj->btf); > > > + for (j = 1; j < n; j++) { > > > + const char *str = "exception_callback:", *name; > > > + size_t len = strlen(str); > > > + struct btf_type *t; > > > + > > > + t = btf_type_by_id(obj->btf, j); > > > + if (!btf_is_decl_tag(t) || btf_decl_tag(t)->component_idx != -1) > > > + continue; > > > + > > > + name = btf__str_by_offset(obj->btf, t->name_off); > > > + if (strncmp(name, str, len)) > > > + continue; > > > + > > > + t = btf_type_by_id(obj->btf, t->type); > > > + if (!btf_is_func(t) || btf_func_linkage(t) != BTF_FUNC_GLOBAL) { > > > + pr_warn("prog '%s': exception_callback:<value> decl tag not applied to the main program\n", > > > + prog->name); > > > + return -EINVAL; > > > + } > > > + if (strcmp(prog->name, btf__str_by_offset(obj->btf, t->name_off))) > > > + continue; > > > + /* Multiple callbacks are specified for the same prog, > > > + * the verifier will eventually return an error for this > > > + * case, hence simply skip appending a subprog. > > > + */ > > > + if (prog->exception_cb_idx >= 0) { > > > + prog->exception_cb_idx = -1; > > > + break; > > > + } > > > > you check this condition three times and handle it in three different > > ways, it's bizarre. Why? > > > > I agree it looks confusing. The first check happens when for a given > main program, we are going through all types and we already saw a > exception cb satisfying the conditions previously. > The second one is to catch multiple subprogs that are static and have > the same name. So in the loop with k as iterator, if we already found > a satisfying subprog, we still continue to catch other cases by > matching on the name and linkage. btw, given that we expect callback to be global, you should ignore static subprogs, even if they have the same name. I think it is valid during static linking to have static func with a conflicting name with some other global func or static func. So we shouldn't error out on static funcs there. Let's add the test for this condition. > The third one is to just check whether the loop over subprogs for a > given main prog actually set the exception_cb_idx or not, otherwise we > could not find a subprog with the target name in the decl tag string. > > I hope this clears up some confusion. But I will rework it as you > suggested. It's very late here today but I can send it out tomorrow. I wasn't confused, but to me that was a sign that this code needs a bit more thought :) thanks for agreeing to follow up and improve it > > > > > > + > > > + name += len; > > > + if (str_is_empty(name)) { > > > + pr_warn("prog '%s': exception_callback:<value> decl tag contains empty value\n", > > > + prog->name); > > > + return -EINVAL; > > > + } > > > + > > > + for (k = 0; k < obj->nr_programs; k++) { > > > + struct bpf_program *subprog = &obj->programs[k]; > > > + > > > + if (!prog_is_subprog(obj, subprog)) > > > + continue; > > > + if (strcmp(name, subprog->name)) > > > + continue; > > > + /* Enforce non-hidden, as from verifier point of > > > + * view it expects global functions, whereas the > > > + * mark_btf_static fixes up linkage as static. > > > + */ > > > + if (!subprog->sym_global || subprog->mark_btf_static) { > > > + pr_warn("prog '%s': exception callback %s must be a global non-hidden function\n", > > > + prog->name, subprog->name); > > > + return -EINVAL; > > > + } > > > + /* Let's see if we already saw a static exception callback with the same name */ > > > + if (prog->exception_cb_idx >= 0) { > > > + pr_warn("prog '%s': multiple subprogs with same name as exception callback '%s'\n", > > > + prog->name, subprog->name); > > > + return -EINVAL; > > > + } > > > + prog->exception_cb_idx = k; > > > + break; > > > + } > > > + > > > + if (prog->exception_cb_idx >= 0) > > > + continue; > > > + pr_warn("prog '%s': cannot find exception callback '%s'\n", prog->name, name); > > > + return -ENOENT; > > > + } > > > + } > > > +skip_exception_cb: > > > + > > > sanitize = btf_needs_sanitization(obj); > > > if (sanitize) { > > > const void *raw_data; > > > @@ -6270,10 +6355,10 @@ static int > > > bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog, > > > struct bpf_program *prog) > > > { > > > - size_t sub_insn_idx, insn_idx, new_cnt; > > > + size_t sub_insn_idx, insn_idx; > > > struct bpf_program *subprog; > > > - struct bpf_insn *insns, *insn; > > > struct reloc_desc *relo; > > > + struct bpf_insn *insn; > > > int err; > > > > > > err = reloc_prog_func_and_line_info(obj, main_prog, prog); > > > @@ -6582,6 +6667,25 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path) > > > prog->name, err); > > > return err; > > > } > > > + > > > + /* Now, also append exception callback if it has not been done already. */ > > > + if (prog->exception_cb_idx >= 0) { > > > + struct bpf_program *subprog = &obj->programs[prog->exception_cb_idx]; > > > + > > > + /* Calling exception callback directly is disallowed, which the > > > + * verifier will reject later. In case it was processed already, > > > + * we can skip this step, otherwise for all other valid cases we > > > + * have to append exception callback now. > > > + */ > > > + if (subprog->sub_insn_off == 0) { > > > + err = bpf_object__append_subprog_code(obj, prog, subprog); > > > + if (err) > > > + return err; > > > + err = bpf_object__reloc_code(obj, prog, subprog); > > > + if (err) > > > + return err; > > > + } > > > + } > > > } > > > /* Process data relos for main programs */ > > > for (i = 0; i < obj->nr_programs; i++) { > > > -- > > > 2.41.0 > > >
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index afc07a8f7dc7..3a6108e3238b 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -436,9 +436,11 @@ struct bpf_program { int fd; bool autoload; bool autoattach; + bool sym_global; bool mark_btf_static; enum bpf_prog_type type; enum bpf_attach_type expected_attach_type; + int exception_cb_idx; int prog_ifindex; __u32 attach_btf_obj_fd; @@ -765,6 +767,7 @@ bpf_object__init_prog(struct bpf_object *obj, struct bpf_program *prog, prog->type = BPF_PROG_TYPE_UNSPEC; prog->fd = -1; + prog->exception_cb_idx = -1; /* libbpf's convention for SEC("?abc...") is that it's just like * SEC("abc...") but the corresponding bpf_program starts out with @@ -871,14 +874,16 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data, if (err) return err; + if (ELF64_ST_BIND(sym->st_info) != STB_LOCAL) + prog->sym_global = true; + /* if function is a global/weak symbol, but has restricted * (STV_HIDDEN or STV_INTERNAL) visibility, mark its BTF FUNC * as static to enable more permissive BPF verification mode * with more outside context available to BPF verifier */ - if (ELF64_ST_BIND(sym->st_info) != STB_LOCAL - && (ELF64_ST_VISIBILITY(sym->st_other) == STV_HIDDEN - || ELF64_ST_VISIBILITY(sym->st_other) == STV_INTERNAL)) + if (prog->sym_global && (ELF64_ST_VISIBILITY(sym->st_other) == STV_HIDDEN + || ELF64_ST_VISIBILITY(sym->st_other) == STV_INTERNAL)) prog->mark_btf_static = true; nr_progs++; @@ -3142,6 +3147,86 @@ static int bpf_object__sanitize_and_load_btf(struct bpf_object *obj) } } + if (!kernel_supports(obj, FEAT_BTF_DECL_TAG)) + goto skip_exception_cb; + for (i = 0; i < obj->nr_programs; i++) { + struct bpf_program *prog = &obj->programs[i]; + int j, k, n; + + if (prog_is_subprog(obj, prog)) + continue; + n = btf__type_cnt(obj->btf); + for (j = 1; j < n; j++) { + const char *str = "exception_callback:", *name; + size_t len = strlen(str); + struct btf_type *t; + + t = btf_type_by_id(obj->btf, j); + if (!btf_is_decl_tag(t) || btf_decl_tag(t)->component_idx != -1) + continue; + + name = btf__str_by_offset(obj->btf, t->name_off); + if (strncmp(name, str, len)) + continue; + + t = btf_type_by_id(obj->btf, t->type); + if (!btf_is_func(t) || btf_func_linkage(t) != BTF_FUNC_GLOBAL) { + pr_warn("prog '%s': exception_callback:<value> decl tag not applied to the main program\n", + prog->name); + return -EINVAL; + } + if (strcmp(prog->name, btf__str_by_offset(obj->btf, t->name_off))) + continue; + /* Multiple callbacks are specified for the same prog, + * the verifier will eventually return an error for this + * case, hence simply skip appending a subprog. + */ + if (prog->exception_cb_idx >= 0) { + prog->exception_cb_idx = -1; + break; + } + + name += len; + if (str_is_empty(name)) { + pr_warn("prog '%s': exception_callback:<value> decl tag contains empty value\n", + prog->name); + return -EINVAL; + } + + for (k = 0; k < obj->nr_programs; k++) { + struct bpf_program *subprog = &obj->programs[k]; + + if (!prog_is_subprog(obj, subprog)) + continue; + if (strcmp(name, subprog->name)) + continue; + /* Enforce non-hidden, as from verifier point of + * view it expects global functions, whereas the + * mark_btf_static fixes up linkage as static. + */ + if (!subprog->sym_global || subprog->mark_btf_static) { + pr_warn("prog '%s': exception callback %s must be a global non-hidden function\n", + prog->name, subprog->name); + return -EINVAL; + } + /* Let's see if we already saw a static exception callback with the same name */ + if (prog->exception_cb_idx >= 0) { + pr_warn("prog '%s': multiple subprogs with same name as exception callback '%s'\n", + prog->name, subprog->name); + return -EINVAL; + } + prog->exception_cb_idx = k; + break; + } + + if (prog->exception_cb_idx >= 0) + continue; + pr_warn("prog '%s': cannot find exception callback '%s'\n", prog->name, name); + return -ENOENT; + } + } +skip_exception_cb: + sanitize = btf_needs_sanitization(obj); if (sanitize) { const void *raw_data; @@ -6270,10 +6355,10 @@ static int bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog, struct bpf_program *prog) { - size_t sub_insn_idx, insn_idx, new_cnt; + size_t sub_insn_idx, insn_idx; struct bpf_program *subprog; - struct bpf_insn *insns, *insn; struct reloc_desc *relo; + struct bpf_insn *insn; int err; err = reloc_prog_func_and_line_info(obj, main_prog, prog); @@ -6582,6 +6667,25 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path) prog->name, err); return err; } + + /* Now, also append exception callback if it has not been done already. */ + if (prog->exception_cb_idx >= 0) { + struct bpf_program *subprog = &obj->programs[prog->exception_cb_idx]; + + /* Calling exception callback directly is disallowed, which the + * verifier will reject later. In case it was processed already, + * we can skip this step, otherwise for all other valid cases we + * have to append exception callback now. + */ + if (subprog->sub_insn_off == 0) { + err = bpf_object__append_subprog_code(obj, prog, subprog); + if (err) + return err; + err = bpf_object__reloc_code(obj, prog, subprog); + if (err) + return err; + } + } } /* Process data relos for main programs */ for (i = 0; i < obj->nr_programs; i++) {
Add support to libbpf to append exception callbacks when loading a program. The exception callback is found by discovering the declaration tag 'exception_callback:<value>' and finding the callback in the value of the tag. The process is done in two steps. First, for each main program, the bpf_object__sanitize_and_load_btf function finds and marks its corresponding exception callback as defined by the declaration tag on it. Second, bpf_object__reloc_code is modified to append the indicated exception callback at the end of the instruction iteration (since exception callback will never be appended in that loop, as it is not directly referenced). Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- tools/lib/bpf/libbpf.c | 114 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 109 insertions(+), 5 deletions(-)