Re: [PATCH bpf-next v4 07/11] libbpf: Resolve invalid weak kfunc calls with imm = 0, off = 0

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 20, 2021 at 7:15 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> Preserve these calls as it allows verifier to succeed in loading the
> program if they are determined to be unreachable after dead code
> elimination during program load. If not, the verifier will fail at
> runtime. This is done for ext->is_weak symbols similar to the case for
> variable ksyms.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---

Looks good with few nits below

Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>

>  tools/lib/bpf/libbpf.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 3049dfc6088e..3c195eaadf56 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -3413,11 +3413,6 @@ static int bpf_object__collect_externs(struct bpf_object *obj)
>                                 return -ENOTSUP;
>                         }
>                 } else if (strcmp(sec_name, KSYMS_SEC) == 0) {
> -                       if (btf_is_func(t) && ext->is_weak) {
> -                               pr_warn("extern weak function %s is unsupported\n",
> -                                       ext->name);
> -                               return -ENOTSUP;
> -                       }
>                         ksym_sec = sec;
>                         ext->type = EXT_KSYM;
>                         skip_mods_and_typedefs(obj->btf, t->type,
> @@ -5366,8 +5361,12 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
>                 case RELO_EXTERN_FUNC:
>                         ext = &obj->externs[relo->sym_off];
>                         insn[0].src_reg = BPF_PSEUDO_KFUNC_CALL;
> -                       insn[0].imm = ext->ksym.kernel_btf_id;
> -                       insn[0].off = ext->ksym.offset;
> +                       if (ext->is_set) {
> +                               insn[0].imm = ext->ksym.kernel_btf_id;
> +                               insn[0].off = ext->ksym.offset;
> +                       } else { /* unresolved weak kfunc */
> +                               insn[0].imm = insn[0].off = 0;

it's a bit too easy to miss this, please write as two separate statements

> +                       }
>                         break;
>                 case RELO_SUBPROG_ADDR:
>                         if (insn[0].src_reg != BPF_PSEUDO_FUNC) {
> @@ -6768,8 +6767,10 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
>
>         kfunc_id = find_ksym_btf_id(obj, ext->name, BTF_KIND_FUNC,
>                                     &kern_btf, &kern_btf_fd);
> -       if (kfunc_id < 0) {
> -               pr_warn("extern (func ksym) '%s': not found in kernel BTF\n",
> +       if (kfunc_id == -ESRCH && ext->is_weak) {
> +               return 0;
> +       } else if (kfunc_id < 0) {

nit: there is return above, no need for "else if", please drop that
part. It would be nice if you could clean this up in
bpf_object__resolve_ksym_var_btf_id() as well. Thanks!

> +               pr_warn("extern (func ksym) '%s': not found in kernel or module BTFs\n",
>                         ext->name);
>                 return kfunc_id;
>         }
> --
> 2.33.0
>



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux