linux-kernel - Re: [PATCH bpf-next] libbpf: Let any two INT/UNION compatible if their names and sizes match

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]

Date:   Tue, 30 Nov 2021 20:17:22 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Feng zhou <zhoufeng.zf@...edance.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        duanxiongchun@...edance.com,
        Muchun Song <songmuchun@...edance.com>,
        zhouchengming@...edance.com
Subject: Re: [PATCH bpf-next] libbpf: Let any two INT/UNION compatible if
 their names and sizes match

On Tue, Nov 30, 2021 at 7:55 PM Feng zhou <zhoufeng.zf@...edance.com> wrote:
>
> From: Feng Zhou <zhoufeng.zf@...edance.com>
>
> commit:67c0496e87d193b8356d2af49ab95e8a1b954b3c(kernfs: convert
> kernfs_node->id from union kernfs_node_id to u64).
>
> The bpf program compiles on the kernel version after this commit and
> then tries to run on the kernel before this commit, libbpf will report
> an error. The reverse is also same.
>
> libbpf: prog 'tcp_retransmit_synack_tp': relo #4: kind <byte_off> (0),
> spec is [342] struct kernfs_node.id (0:9 @ offset 104)
> libbpf: prog 'tcp_retransmit_synack_tp': relo #4: non-matching candidate
> libbpf: prog 'tcp_retransmit_synack_tp': relo #4: non-matching candidate
> libbpf: prog 'tcp_retransmit_synack_tp': relo #4: no matching targets
> found
>
> The type before this commit:
>         union kernfs_node_id    id;
>         union kernfs_node_id {
>                 struct {
>                         u32             ino;
>                         u32             generation;
>                 };
>                 u64                     id;
>         };
>
> The type after this commit:
>         u64 id;
>
> We can find that the variable name and size have not changed except for
> the type change.
> So I added some judgment to let any two INT/UNION are compatible, if
> their names and sizes match.
>
> Reported-by: Chengming Zhou <zhouchengming@...edance.com>
> Tested-by: Chengming Zhou <zhouchengming@...edance.com>
> Signed-off-by: Feng Zhou <zhoufeng.zf@...edance.com>
> ---

This should be handled by application, not by hacking libbpf's CO-RE
relocation logic. See [0] for how this should be done with existing
BPF CO-RE mechanisms.

  [0] https://nakryiko.com/posts/bpf-core-reference-guide/#handling-incompatible-field-and-type-changes

>  tools/lib/bpf/relo_core.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
> index b5b8956a1be8..ff7f4e97bafb 100644
> --- a/tools/lib/bpf/relo_core.c
> +++ b/tools/lib/bpf/relo_core.c
> @@ -294,6 +294,7 @@ static int bpf_core_parse_spec(const struct btf *btf,
>   *   - any two FLOATs are always compatible;
>   *   - for ARRAY, dimensionality is ignored, element types are checked for
>   *     compatibility recursively;
> + *   - any two INT/UNION are compatible, if their names and sizes match;
>   *   - everything else shouldn't be ever a target of relocation.
>   * These rules are not set in stone and probably will be adjusted as we get
>   * more experience with using BPF CO-RE relocations.
> @@ -313,8 +314,14 @@ static int bpf_core_fields_are_compat(const struct btf *local_btf,
>
>         if (btf_is_composite(local_type) && btf_is_composite(targ_type))
>                 return 1;
> -       if (btf_kind(local_type) != btf_kind(targ_type))
> -               return 0;
> +       if (btf_kind(local_type) != btf_kind(targ_type)) {
> +               if (local_type->size == targ_type->size &&
> +                   (btf_is_union(local_type) || btf_is_union(targ_type)) &&
> +                   (btf_is_int(local_type) || btf_is_int(targ_type)))
> +                       return 1;
> +               else
> +                       return 0;
> +       }
>
>         switch (btf_kind(local_type)) {
>         case BTF_KIND_PTR:
> @@ -384,11 +391,17 @@ static int bpf_core_match_member(const struct btf *local_btf,
>         targ_type = skip_mods_and_typedefs(targ_btf, targ_id, &targ_id);
>         if (!targ_type)
>                 return -EINVAL;
> -       if (!btf_is_composite(targ_type))
> -               return 0;
>
>         local_id = local_acc->type_id;
>         local_type = btf__type_by_id(local_btf, local_id);
> +       if (!btf_is_composite(targ_type)) {
> +               if (local_type->size == targ_type->size &&
> +                   btf_is_union(local_type) && btf_is_int(targ_type))
> +                       return 1;
> +               else
> +                       return 0;
> +       }
> +
>         local_member = btf_members(local_type) + local_acc->idx;
>         local_name = btf__name_by_offset(local_btf, local_member->name_off);
>
> --
> 2.11.0
>