[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4BzZxivU8zNfzuQQJyVs=HF7ckjuWZFa=HgpPD-VgpWyVWQ@mail.gmail.com>
Date: Sun, 28 Jun 2020 13:00:46 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Yonghong Song <yhs@...com>, Jiri Olsa <jolsa@...nel.org>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
Song Liu <songliubraving@...com>,
Martin KaFai Lau <kafai@...com>,
David Miller <davem@...hat.com>,
John Fastabend <john.fastabend@...il.com>,
Wenbo Zhang <ethercflow@...il.com>,
KP Singh <kpsingh@...omium.org>,
Andrii Nakryiko <andriin@...com>,
Brendan Gregg <bgregg@...flix.com>,
Florent Revest <revest@...omium.org>,
Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving
On Sun, Jun 28, 2020 at 11:50 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> 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.
Powered by blists - more mailing lists