On Tue, Sep 19, 2023 at 6:03 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> Hi Andrii,
>
> On Wed, 20 Sept 2023 at 02:25, Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Tue, Sep 12, 2023 at 4:32 PM Kumar Kartikeya Dwivedi
> > <memxor@xxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > > 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
> > >
