Re: [PATCH v2 bpf-next 8/9] libbpf: implement __arg_ctx fallback logic

[Eduard Zingerman <eddyz87@gmail.com>]

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.

[...]

> > > > > +             /* 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.

> 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.

[...]