On Wed, Jan 3, 2024 at 4:09 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Wed, 2024-01-03 at 15:59 -0800, Andrii Nakryiko wrote:
> [...]
> > > > > > + 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.
> > >
> >
> > I already did the change locally, as I said it's a small change, so no problem.
>
> Oh, ok, thanks.
>
np
> [...]
>
> > > > > > + /* 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.
> >
> > Note the "/* check if existing parameter already matches verifier
> > expectations */", if program is using correct types, we don't touch
> > BTF for that subprog. If there was `void *ctx`, we don't really lose
> > any information.
>
> But `void *ctx` would be changed to `struct bpf_user_pt_regs_t *ctx`, right?
> And that might be a bit surprising. But whatever, if you think that adding
> log entry here is too much of hassle -- let's leave it as is.
>
Ok.
> > If they use `struct pt_regs *ctx __arg_ctx`, then yeah, it will be
> > updated to `struct bpf_user_pt_regs_t *ctx __arg_ctx`, but even then,
> > original BTF has original FUNC -> FUNC_PROTO definition. You'd need to
> > fetch func_info and follow BTF IDs (I'm not sure if bpftool even shows
> > this today).
> >
> > In short, I don't see why this would be a problem, but perhaps I
> > should just bite a bullet and do feature detector for this support.
>
> I like that current implementation does the transformation unconditionally,
> it does no harm and avoids unnecessary branching.
>
ack
> [...]
