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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Apr 23, 2021 at 9:18 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Apr 23, 2021 at 9:09 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Apr 22, 2021 at 10:11 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Thu, Apr 22, 2021 at 9:25 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > 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.
> > >
> > > I see. So something like 'extern cosnt void foo __ksym' would
> > > point to void type?
> > > But then why is it not a part of the id_map[] and has
> > > to be handled explicitly?
> >
> > const void foo will be VAR -> CONST -> VOID. But any `void *` anywhere
> > will be PTR -> VOID. Any void bla(int x) would have return type VOID
> > (0), and so on. There are a lot of cases when we use VOID as type_id.
> > VOID always is handled specially, because it stays zero despite any
> > transformation: during BTF concatenation, BTF dedup, BTF generation,
> > etc.
> >
> > >
> > > > > 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).
> > >
> > > Right, id_map[0] should be 0.
> > > I'm still missing something in this combination of 'if's.
> > > May be do it as:
> > > if (new_id == 0 && *type_id != 0) { pr_warn
> > > ?
> > > That was the idea?
> >
> > That's the idea, there is just no need to do VOID -> VOID
> > transformation, but I'll rewrite it to a combined if if it makes it
> > easier to follow. Here's full source of remap_type_id with few
> > comments to added:
> >
> > static int remap_type_id(__u32 *type_id, void *ctx)
> > {
> >         int *id_map = ctx;
> >         int new_id = id_map[*type_id];
> >
> >
> > /* Here VOID stays VOID, that's all */
> >
> >         if (*type_id == 0)
> >                 return 0;
>
> Does it mean that id_map[0] is a garbage value?
> and all other code that might be doing id_map[idx] might be reading
> garbage if it doesn't have a check for idx == 0 ?

No, id_map[0] == 0 by construction (id_map is obj->btf_type_map and is
calloc()'ed) and can be used as id_map[idx].

>
> > /* This means whatever type we are trying to remap didn't get a new ID
> > assigned in linker->btf and that's an error */
> >         if (new_id == 0) {
> >                 pr_warn("failed to find new ID mapping for original
> > BTF type ID %u\n", *type_id);
> >                 return -EINVAL;
> >         }
> >
> >         *type_id = id_map[*type_id];
> >
> >         return 0;
> > }