On Thu, Apr 22, 2021 at 9:25 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, Apr 22, 2021 at 7:54 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Thu, Apr 22, 2021 at 11:24 AM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Thu, Apr 22, 2021 at 9:50 AM Yonghong Song <yhs@xxxxxx> 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?
> > 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?
