Re: [PATCH v2 bpf-next 10/17] libbpf: tighten BTF type ID rewriting with error checking

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andrii@kernel.org>,
	bpf <bpf@vger.kernel.org>, Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 10/17] libbpf: tighten BTF type ID rewriting with error checking
Date: Thu, 22 Apr 2021 21:25:33 -0700	[thread overview]
Message-ID: <CAEf4BzYXZOX=dmrAQAxHinSa0mxJ5gkJkpL=paVJjtrEWQex4A@mail.gmail.com> (raw)
In-Reply-To: <CAADnVQJ_PS=PH8AQySiHqn-Bm=+DxsqRkgx+2_7OxM5CQkB4Mg@mail.gmail.com>

On Thu, Apr 22, 2021 at 7:54 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Apr 22, 2021 at 11:24 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Apr 22, 2021 at 9:50 AM Yonghong Song <yhs@fb.com> wrote:
> > >
> > >
> > >
> > > On 4/16/21 1:23 PM, Andrii Nakryiko wrote:
> > > > It should never fail, but if it does, it's better to know about this rather
> > > > than end up with nonsensical type IDs.
> > >
> > > So this is defensive programming. Maybe do another round of
> > > audit of the callers and if you didn't find any issue, you
> > > do not need to check not-happening condition here?
> >
> > It's far from obvious that this will never happen, because we do a
> > decently complicated BTF processing (we skip some types altogether
> > believing that they are not used, for example) and it will only get
> > more complicated with time. Just as there are "verifier bug" checks in
> > kernel, this prevents things from going wild if non-trivial bugs will
> > inevitably happen.
>
> I agree with Yonghong. This doesn't look right.

I read it as Yonghong was asking about the entire patch. You seem to
be concerned with one particular check, right?

> The callback will be called for all non-void types, right?
> so *type_id == 0 shouldn't never happen.
> If it does there is a bug somewhere that should be investigated
> instead of ignored.

See btf_type_visit_type_ids() and btf_ext_visit_type_ids(), they call
callback for every field that contains type ID, even if it points to
VOID. So this can happen and is expected.

> The
> if (new_id == 0) pr_warn
> bit makes sense.

Right, and this is the point of this patch. id_map[] will have zeroes
for any unmapped type, so I just need to make sure I'm not false
erroring on id_map[0] (== 0, which is valid, but never used).

> My reading that it will abort the whole linking process and
> this linker bug will be reported back to us.
> So it's good.

Right, it will be propagated all the way up.