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

On Fri, Apr 23, 2021 at 9:18 AM Alexei Starovoitov
[off-list ref] wrote:

On Fri, Apr 23, 2021 at 9:09 AM Andrii Nakryiko
[off-list ref] wrote:

quoted

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

quoted

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

quoted

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?

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.

quoted

quoted

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?

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

quoted

/* 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;
}