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

On Thu, Apr 22, 2021 at 9:25 PM Andrii Nakryiko
[off-list ref] wrote:

On Thu, Apr 22, 2021 at 7:54 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Thu, Apr 22, 2021 at 11:24 AM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Thu, Apr 22, 2021 at 9:50 AM Yonghong Song [off-list ref] wrote:

quoted

On 4/16/21 1:23 PM, Andrii Nakryiko wrote:

quoted

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?

quoted

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?

quoted

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?