On Wed, 2024-01-03 at 15:10 -0800, Andrii Nakryiko wrote:
> 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.
Noted, well, maybe just skip this nit in such a case.
> > [...]
> >
> > > +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).
I was thinking about someone finding out that actual in-kernel BTF
is different from that in the program object file, while debugging
something. Might be a bit surprising. I'm not insisting on this, though.
> > > + }
> > > +
> > > + /* 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.
I was thinking about possible interaction with btf_struct_access(),
but that is not used to verify context access at the moment.
So, probably not important.
