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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4Bzaw0Je1zQvbKQh9VP3f1UuWTbsLZjJpyvSCHOr_JZGjsA@mail.gmail.com>
Date:   Fri, 23 Apr 2021 10:02:19 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Yonghong Song <yhs@...com>, Andrii Nakryiko <andrii@...nel.org>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 10/17] libbpf: tighten BTF type ID rewriting
 with error checking

On Fri, Apr 23, 2021 at 9:34 AM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Fri, Apr 23, 2021 at 9:31 AM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> > > >
> > > > 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].
>
> Ok. Then why are you insisting on this micro optimization to return 0
> directly?
> That's the confusing part for me.

I'm not insisting:

  > but I'll rewrite it to a combined if if it makes it easier to follow

So I'm confused why you are confused.

>
> If it was:
> "if (new_id == 0 && *type_id != 0) { pr_warn"
> Then it would be clear what error condition is about.
> But 'return 0' messing things up in my mind,
> because it's far from obvious that first check is really a combination
> with the 2nd check and by itself it's a micro optimization to avoid
> reading id_map[0].

I didn't try to micro optimize, that's how I naturally think about the
problem. I'll rewrite the if, don't know why we are spending emails on
this still.