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. > 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.
