Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Yonghong Song <yhs@fb.com>, Jiri Olsa <jolsa@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	Song Liu <songliubraving@fb.com>, Martin KaFai Lau <kafai@fb.com>,
	David Miller <davem@redhat.com>,
	John Fastabend <john.fastabend@gmail.com>,
	Wenbo Zhang <ethercflow@gmail.com>,
	KP Singh <kpsingh@chromium.org>, Andrii Nakryiko <andriin@fb.com>,
	Brendan Gregg <bgregg@netflix.com>,
	Florent Revest <revest@chromium.org>,
	Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH v4 bpf-next 05/14] bpf: Remove btf_id helpers resolving
Date: Sun, 28 Jun 2020 13:00:46 -0700	[thread overview]
Message-ID: <CAEf4BzZxivU8zNfzuQQJyVs=HF7ckjuWZFa=HgpPD-VgpWyVWQ@mail.gmail.com> (raw)
In-Reply-To: <20200628185046.vryhuc23f6yu5fy4@ast-mbp.dhcp.thefacebook.com>

On Sun, Jun 28, 2020 at 11:50 AM Alexei Starovoitov
<alexei.starovoitov@gmail.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.