On Sun, Jun 28, 2020 at 11:50 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Fri, Jun 26, 2020 at 04:29:23PM -0700, Yonghong Song wrote:
> > > > >
> > > > > -int btf_resolve_helper_id(struct bpf_verifier_log *log,
> > > > > - const struct bpf_func_proto *fn, int arg)
> > > > > -{
> > > > > - int *btf_id = &fn->btf_id[arg];
> > > > > - int ret;
> > > > > -
> > > > > - if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
> > > > > + if (!id || id > btf_vmlinux->nr_types)
> > > > > return -EINVAL;
> > > >
> > > > id == 0 if btf_id cannot be resolved by resolve_btfids, right?
> > > > when id may be greater than btf_vmlinux->nr_types? If resolve_btfids
> > > > application did incorrect transformation?
> > > >
> > > > Anyway, this is to resolve helper meta btf_id. Even if you
> > > > return a btf_id > btf_vmlinux->nr_types, verifier will reject
> > > > since it will never be the same as the real parameter btf_id.
> > > > I would drop id > btf_vmlinux->nr_types here. This should never
> > > > happen for a correct tool. Even if it does, verifier will take
> > > > care of it.
> > > >
> > >
> > > I'd love to hear Alexei's thoughts about this change as well. Jiri
> > > removed not just BTF ID resolution, but also all the sanity checks.
> > > This now means more trust in helper definitions to not screw up
> > > anything. It's probably OK, but still something to consciously think
> > > about.
> >
> > The kernel will have to trust the result.
>
> +1
> I think 'if (!id || id > btf_vmlinux->nr_types)' at run-time and
> other sanity checks I saw in other patches are unnecessary.
> resolve_btfids should do all checks and fail vmlinux linking.
> We trust gcc to generate correct assembly code in the first place
> and correct dwarf. We trust pahole do correct BTF conversion from
> dwarf and dedup. We should trust resolve_btfids.
> It's imo the simplest tool comparing to gcc.
> btf_parse_vmlinux() is doing basic sanity check of BTF mainly
> to populate 'struct btf *btf_vmlinux;' for further use.
> I think we can add a scan over resolved btfids to btf_parse_vmlinux()
> to make sure that all ids are within the range, but I don't think
> it's mandatory for this patch set. Would be a reasonable sanity
> check for the future. Of course, checking for sorted set_start/end
> is overkill. Basic simplest sanity is ok.
My point was *not* about trusting resolve_btfids tool, it was about
trusting BPF helper definitions. But it's more of help for developer
while developing new helpers, so it's not a major thing.
