| Message ID | 20230809114116.3216687-13-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Exceptions - 1/2 | expand |
On Wed, Aug 9, 2023 at 4:44 AM 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. ... > + /* After adding all programs, now pair them with their exception > + * callbacks if specified. > + */ > + if (!kernel_supports(obj, FEAT_BTF_DECL_TAG)) > + goto out; > +out: The above looks odd. Accidental leftover? > return 0; > } > > @@ -3137,6 +3148,80 @@ 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; On the first read of this patch and corresponding kernel support I started doubting my earlier suggestion to use decl_tag and reconsidered going back to fake bpf_register_except_cb() call, but after sleeping on it I think it is a useful extension for both kernel and libbpf to support such tagging. We might specify ctors and dtors with decl_tag in the future and other various callbacks that are never explicitly referenced in bpf_call, ld_imm64 or other bpf insns. So having libbpf and kernel support such tagging will help in the long run. It's not going to be limited to exceptions. Despite the extra complexity this is a good step forward. > +static int > +bpf_object__append_subprog_code(struct bpf_object *obj, struct bpf_program *main_prog, > + struct bpf_program *subprog) > +{ Please split this refactoring into a separate patch for ease of review.
On Tue, 22 Aug 2023 at 22:05, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Aug 9, 2023 at 4:44 AM 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. > > ... > > > + /* After adding all programs, now pair them with their exception > > + * callbacks if specified. > > + */ > > + if (!kernel_supports(obj, FEAT_BTF_DECL_TAG)) > > + goto out; > > +out: > > The above looks odd. Accidental leftover? > Oops, yes. I have dropped it now. > > return 0; > > } > > > > @@ -3137,6 +3148,80 @@ 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; > > On the first read of this patch and corresponding kernel support > I started doubting my earlier suggestion to use decl_tag and > reconsidered going back to fake bpf_register_except_cb() call, This is exactly what I thought when I realised it's not that simple when implementing it :). > but after sleeping on it I think it is a useful extension for both > kernel and libbpf to support such tagging. > We might specify ctors and dtors with decl_tag in the future > and other various callbacks that are never explicitly referenced > in bpf_call, ld_imm64 or other bpf insns. > So having libbpf and kernel support such tagging will help in the long run. > It's not going to be limited to exceptions. > Despite the extra complexity this is a good step forward. > I agree. This same code can also be reused to establish an edge from one BTF type to some other BTF type (by name). function -> exception_cb. struct -> ctor, struct -> dtor etc. I did have some questions though. First of all this is explicitly an unstable feature. How do we feel about putting related support for it in libbpf and making breaking changes later? Will the expectation be to pair the libbpf with its corresponding kernel release to use such features? Or do we have to make the changes in a backwards compatible fashion? Secondly, due to proliferation of BTF tag usage, do you think it's time we reserve a namespace for all tags that would be recognized by the verifier? E.g. require all of them to be prefixed with "bpf." similar to "llvm." etc.? Since they are simply attributes for a specific BTF type (or component of a type). It may be too late for some BTF tags, but we could do better going forward. It may also allow us to indicate that a tag is experimental until its effect on the program becomes stabilized. E.g. bpf.experimental.exception_callback instead of exception_callback? Or do you feel it's unnecessary? > > +static int > > +bpf_object__append_subprog_code(struct bpf_object *obj, struct bpf_program *main_prog, > > + struct bpf_program *subprog) > > +{ > > Please split this refactoring into a separate patch for ease of review. Ack, will do.
On Tue, Aug 22, 2023 at 9:58 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Tue, 22 Aug 2023 at 22:05, Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Wed, Aug 9, 2023 at 4:44 AM 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. > > > > ... > > > > > + /* After adding all programs, now pair them with their exception > > > + * callbacks if specified. > > > + */ > > > + if (!kernel_supports(obj, FEAT_BTF_DECL_TAG)) > > > + goto out; > > > +out: > > > > The above looks odd. Accidental leftover? > > > > Oops, yes. I have dropped it now. > > > > return 0; > > > } > > > > > > @@ -3137,6 +3148,80 @@ 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; > > > > On the first read of this patch and corresponding kernel support > > I started doubting my earlier suggestion to use decl_tag and > > reconsidered going back to fake bpf_register_except_cb() call, > > This is exactly what I thought when I realised it's not that simple > when implementing it :). > > > but after sleeping on it I think it is a useful extension for both > > kernel and libbpf to support such tagging. > > We might specify ctors and dtors with decl_tag in the future > > and other various callbacks that are never explicitly referenced > > in bpf_call, ld_imm64 or other bpf insns. > > So having libbpf and kernel support such tagging will help in the long run. > > It's not going to be limited to exceptions. > > Despite the extra complexity this is a good step forward. > > > > I agree. This same code can also be reused to establish an edge from > one BTF type to some other BTF type (by name). > function -> exception_cb. struct -> ctor, struct -> dtor etc. > > I did have some questions though. > First of all this is explicitly an unstable feature. How do we feel > about putting related support for it in libbpf and making breaking > changes later? > Will the expectation be to pair the libbpf with its corresponding > kernel release to use such features? Or do we have to make the changes > in a backwards compatible fashion? We should always do backwards compatible changes in both kernel and libbpf, but sooner or later we might hit an issue where we would have to break things. At that time the special prefix won't save us, so... > > Secondly, due to proliferation of BTF tag usage, do you think it's > time we reserve a namespace for all tags that would be recognized by > the verifier? E.g. require all of them to be prefixed with "bpf." > similar to "llvm." etc.? Since they are simply attributes for a > specific BTF type (or component of a type). > It may be too late for some BTF tags, but we could do better going forward. > > It may also allow us to indicate that a tag is experimental until its > effect on the program becomes stabilized. E.g. > bpf.experimental.exception_callback instead of exception_callback? Or > do you feel it's unnecessary? ... I don't think any of that is necessary. Whether btf tag is prefixed with "exception_callback:" or "bpf.experimental.debug.exception_callback:" we will be doing the same thing. We'll keep backward compat if trade-offs allow.
On Wed, Aug 9, 2023 at 4:44 AM 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> > --- Just two point before you send next version: a) it seems like this appending of exception callback logically fits bpf_object__relocate() step, where other subprogs are appended. Any reason we can't do it there? b) all the callbacks are static functions, right? Which means in the case of static linking, we can have multiple subprogs with the same name. So this whole look up by name thing doesn't guarantee unique match. At the very least libbpf should check that the match is unique and error out otherwise. Ideally though, would be great if something like this would be supported instead (but I know it's way more complex, Alexei already mentioned that in person and on the list): try (my_callback) { ... code that throws ... } try (my_other_callback) { ... some other code that throws ... } This try() macro can be implemented in a form similar to bpf_for() by using fancy for() loop. It would look and feel way more like try/catch. > tools/lib/bpf/libbpf.c | 166 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 142 insertions(+), 24 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 17883f5a44b9..7c607bac8204 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -432,9 +432,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; [...]
On Sat, 26 Aug 2023 at 00:13, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Aug 9, 2023 at 4:44 AM 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> > > --- > > Just two point before you send next version: > > a) it seems like this appending of exception callback logically fits > bpf_object__relocate() step, where other subprogs are appended. Any > reason we can't do it there? > We should be able to do it there as well. But I felt it is better to do it in bpf_object__reloc_code as the logic is similar to the handling of bpf_pseudo_func/bpf_pseudo_call insns. And then we need to recurse using bpf_object__reloc_code for exception cb again. > b) all the callbacks are static functions, right? Which means in the > case of static linking, we can have multiple subprogs with the same > name. So this whole look up by name thing doesn't guarantee unique > match. At the very least libbpf should check that the match is unique > and error out otherwise. Ack, will fix this in v3. > > Ideally though, would be great if something like this would be > supported instead (but I know it's way more complex, Alexei already > mentioned that in person and on the list): > > try (my_callback) { > ... code that throws ... > } > > try (my_other_callback) { > ... some other code that throws ... > } > > > This try() macro can be implemented in a form similar to bpf_for() by > using fancy for() loop. It would look and feel way more like > try/catch. > These try blocks are easier than having a try/catch block which the execution jumps to when the exception is thrown. I think the latter will involve some form of compiler support, because otherwise there is no control flow that is seen by the compiler into the catch block, which will mess up things, and I plan to atleast explore that approach (already looking at LLVM) once I am done with the second part of this feature. Having just these try (callback) {} blocks is easier as we can record which subprog corresponds to [begin_ip, end_ip] (per frame) and stop unwinding when we find a suitable handler for the ip of a parent frame. The callback will be invoked and will return to the parent frame (or kernel if it's the main frame). So if this seems like a more useful thing, I can make this work and send it out as a follow up to this set. > [...]
On Sat, Aug 26, 2023 at 3:42 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > Ideally though, would be great if something like this would be > > supported instead (but I know it's way more complex, Alexei already > > mentioned that in person and on the list): > > > > try (my_callback) { > > ... code that throws ... > > } > > > > try (my_other_callback) { > > ... some other code that throws ... > > } > > > > > > This try() macro can be implemented in a form similar to bpf_for() by > > using fancy for() loop. It would look and feel way more like > > try/catch. > > > > These try blocks are easier than having a try/catch block which the > execution jumps to when the exception is thrown. I think the latter > will involve some form of compiler support, because otherwise there is > no control flow that is seen by the compiler into the catch block, > which will mess up things, and I plan to atleast explore that approach > (already looking at LLVM) once I am done with the second part of this > feature. > > Having just these try (callback) {} blocks is easier as we can record > which subprog corresponds to [begin_ip, end_ip] (per frame) and stop > unwinding when we find a suitable handler for the ip of a parent > frame. The callback will be invoked and will return to the parent > frame (or kernel if it's the main frame). So if this seems like a more > useful thing, I can make this work and send it out as a follow up to > this set. I suspect even "just try {}" will not work without compiler support. {} is a semantic structure from compiler pov. It doesn't have any delineation in LLVM IR. They don't exist to optimization passes.
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 17883f5a44b9..7c607bac8204 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -432,9 +432,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; @@ -760,6 +762,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 @@ -866,20 +869,28 @@ 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++; obj->nr_programs = nr_progs; } + /* After adding all programs, now pair them with their exception + * callbacks if specified. + */ + if (!kernel_supports(obj, FEAT_BTF_DECL_TAG)) + goto out; +out: return 0; } @@ -3137,6 +3148,80 @@ 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; + } + 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; @@ -6184,14 +6269,46 @@ static int append_subprog_relos(struct bpf_program *main_prog, struct bpf_progra return 0; } +static int +bpf_object__append_subprog_code(struct bpf_object *obj, struct bpf_program *main_prog, + struct bpf_program *subprog) +{ + struct bpf_insn *insns; + size_t new_cnt; + int err; + + subprog->sub_insn_off = main_prog->insns_cnt; + + new_cnt = main_prog->insns_cnt + subprog->insns_cnt; + insns = libbpf_reallocarray(main_prog->insns, new_cnt, sizeof(*insns)); + if (!insns) { + pr_warn("prog '%s': failed to realloc prog code\n", main_prog->name); + return -ENOMEM; + } + main_prog->insns = insns; + main_prog->insns_cnt = new_cnt; + + memcpy(main_prog->insns + subprog->sub_insn_off, subprog->insns, + subprog->insns_cnt * sizeof(*insns)); + + pr_debug("prog '%s': added %zu insns from sub-prog '%s'\n", + main_prog->name, subprog->insns_cnt, subprog->name); + + /* The subprog insns are now appended. Append its relos too. */ + err = append_subprog_relos(main_prog, subprog); + if (err) + return err; + return 0; +} + 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); @@ -6266,25 +6383,7 @@ bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog, * and relocate. */ if (subprog->sub_insn_off == 0) { - subprog->sub_insn_off = main_prog->insns_cnt; - - new_cnt = main_prog->insns_cnt + subprog->insns_cnt; - insns = libbpf_reallocarray(main_prog->insns, new_cnt, sizeof(*insns)); - if (!insns) { - pr_warn("prog '%s': failed to realloc prog code\n", main_prog->name); - return -ENOMEM; - } - main_prog->insns = insns; - main_prog->insns_cnt = new_cnt; - - memcpy(main_prog->insns + subprog->sub_insn_off, subprog->insns, - subprog->insns_cnt * sizeof(*insns)); - - pr_debug("prog '%s': added %zu insns from sub-prog '%s'\n", - main_prog->name, subprog->insns_cnt, subprog->name); - - /* The subprog insns are now appended. Append its relos too. */ - err = append_subprog_relos(main_prog, subprog); + err = bpf_object__append_subprog_code(obj, main_prog, subprog); if (err) return err; err = bpf_object__reloc_code(obj, main_prog, subprog); @@ -6308,6 +6407,25 @@ bpf_object__reloc_code(struct bpf_object *obj, struct bpf_program *main_prog, prog->name, insn_idx, insn->imm, subprog->name, subprog->sub_insn_off); } + /* Now, also append exception callback if it has not been done already. */ + if (main_prog == prog && main_prog->exception_cb_idx >= 0) { + subprog = &obj->programs[main_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, main_prog, subprog); + if (err) + return err; + err = bpf_object__reloc_code(obj, main_prog, subprog); + if (err) + return err; + } + } + return 0; }
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 | 166 +++++++++++++++++++++++++++++++++++------ 1 file changed, 142 insertions(+), 24 deletions(-)