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
>
