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

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Yonghong Song <yhs@fb.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.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 11:50:46 -0700	[thread overview]
Message-ID: <20200628185046.vryhuc23f6yu5fy4@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <b9258020-cd38-b818-e3a9-4f6d9cdf6b88@fb.com>

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.