On Wed, Jan 3, 2024 at 12:57 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Tue, 2024-01-02 at 11:00 -0800, Andrii Nakryiko wrote:
>
> [...]
>
> > +static int clone_func_btf_info(struct btf *btf, int orig_fn_id, struct bpf_program *prog)
> > +{
>
> [...]
>
> > + /* clone FUNC first, btf__add_func() enforces
> > + * non-empty name, so use entry program's name as
> > + * a placeholder, which we replace immediately
> > + */
> > + fn_id = btf__add_func(btf, prog->name, btf_func_linkage(fn_t), fn_t->type);
>
> Nit: Why not call this function near the end, when fn_proto_id is available?
> Thus avoiding need to "guess" fn_t->type.
>
I think I did it to not have to remember btf_func_linkage(fn_t)
(because fn_t will be invalidated) and because name_off will be reused
for parameters. Neither is a big deal, I'll adjust to your suggestion.
But note, we are not guessing ID, it's guaranteed to be +1, it's an
API contract of btf__add_xxx() APIs.
> [...]
>
> > +static int bpf_program_fixup_func_info(struct bpf_object *obj, struct bpf_program *prog)
> > +{
>
> [...]
>
> > + for (i = 1, n = btf__type_cnt(btf); i < n; i++) {
>
> [...]
>
> > +
> > + /* clone fn/fn_proto, unless we already did it for another arg */
> > + if (func_rec->type_id == orig_fn_id) {
> > + int fn_id;
> > +
> > + fn_id = clone_func_btf_info(btf, orig_fn_id, prog);
> > + if (fn_id < 0) {
> > + err = fn_id;
> > + goto err_out;
> > + }
> > +
> > + /* point func_info record to a cloned FUNC type */
> > + func_rec->type_id = fn_id;
>
> Would it be helpful to add a log here, saying that BTF for function
> so and so is changed before load?
Would it? We don't have global subprog's name readily available, it
seems. So I'd need to refetch it by fn_id, then btf__str_by_offset()
just to emit cryptic (for most users) notifications that something
about some func info was adjusted. And then the user would get this
same message for the same subprog but in the context of a different
entry program. Just confusing, tbh.
Unless you insist, I'd leave it as is. This logic is supposed to be
bullet-proof, so I'm not worried about debugging regressions with it
(but maybe I'm delusional).
>
> > + }
> > +
> > + /* create PTR -> STRUCT type chain to mark PTR_TO_CTX argument;
> > + * we do it just once per main BPF program, as all global
> > + * funcs share the same program type, so need only PTR ->
> > + * STRUCT type chain
> > + */
> > + if (ptr_id == 0) {
> > + struct_id = btf__add_struct(btf, ctx_name, 0);
>
> Nit: Maybe try looking up existing id for type ctx_name first?
It didn't feel important and I didn't want to do another linear BTF
search for each such argument. It's trivial to look it up, but I still
feel like that's a waste... I tried to avoid many linear searches,
which is why I structured the logic to do one pass over BTF to find
all decl_tags instead of going over each function and arg and
searching for decl_tag.
Let's keep it as is, if there are any reasons to try to reuse struct
(if it is at all present, which for kprobe, for example, is quite
unlikely due to fancy bpf_user_pt_regs_t name), then we can easily add
it with no regressions.
>
> > + ptr_id = btf__add_ptr(btf, struct_id);
> > + if (ptr_id < 0 || struct_id < 0) {
> > + err = -EINVAL;
> > + goto err_out;
> > + }
> > + }
> > +
>
> [...]
>
>
