On Wed, Jan 3, 2024 at 3:43 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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.
>
I already did the change locally, as I said it's a small change, so no problem.
> > > [...]
> > >
> > > > +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.
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.
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.
>
> > > > + }
> > > > +
> > > > + /* 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.
Right, and kernel can't trust user-provided BTF info, it will have to
find a match for kernel-side BTF info. I actually add this logic in a
patch set (pending locally for now) that adds support for
PTR_TO_BTF_ID arguments for global funcs.
