libbpf: improve handling of unresolved kfuncs

Currently, libbpf leaves `call #0` instruction for __weak unresolved
kfuncs, which might lead to a confusing verifier log situations, where
invalid `call #0` will be treated as successfully validated.

We can do better. Libbpf already has an established mechanism of
poisoning instructions that failed some form of resolution (e.g., CO-RE
relocation and BPF map set to not be auto-created). Libbpf doesn't fail
them outright to allow users to guard them through other means, and as
long as BPF verifier can prove that such poisoned instructions cannot be
ever reached, this doesn't consistute an invalid BPF program. If user
didn't guard such code, libbpf will extract few pieces of information to
tie such poisoned instructions back to additional information about what
entitity wasn't resolved (e.g., BPF map name, or CO-RE relocation
information).

__weak unresolved kfuncs fit this model well, so this patch extends
libbpf with poisioning and log fixup logic for kfunc calls.

Note, this poisoning is done only for kfunc *calls*, not kfunc address
resolution (ldimm64 instructions). The former cannot be ever valid, if
reached, so it's safe to poison them. The latter is a valid mechanism to
check if __weak kfunc ksym was resolved, and do necessary guarding and
work arounds based on this result, supported in most recent kernels. As
such, libbpf keeps such ldimm64 instructions as loading zero, never
poisoning them.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Link: https://lore.kernel.org/r/20230418002148.3255690-4-andrii@kernel.org
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 0a11563..2600d83 100644 (file)
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -5841,6 +5841,30 @@ static void poison_map_ldimm64(struct bpf_program *prog, int relo_idx,
        }
 }
 
+/* unresolved kfunc call special constant, used also for log fixup logic */
+#define POISON_CALL_KFUNC_BASE 2002000000
+#define POISON_CALL_KFUNC_PFX "2002"
+
+static void poison_kfunc_call(struct bpf_program *prog, int relo_idx,
+                             int insn_idx, struct bpf_insn *insn,
+                             int ext_idx, const struct extern_desc *ext)
+{
+       pr_debug("prog '%s': relo #%d: poisoning insn #%d that calls kfunc '%s'\n",
+                prog->name, relo_idx, insn_idx, ext->name);
+
+       /* we turn kfunc call into invalid helper call with identifiable constant */
+       insn->code = BPF_JMP | BPF_CALL;
+       insn->dst_reg = 0;
+       insn->src_reg = 0;
+       insn->off = 0;
+       /* if this instruction is reachable (not a dead code),
+        * verifier will complain with something like:
+        * invalid func unknown#2001000123
+        * where lower 123 is extern index into obj->externs[] array
+        */
+       insn->imm = POISON_CALL_KFUNC_BASE + ext_idx;
+}
+
 /* Relocate data references within program code:
  *  - map references;
  *  - global variable references;
@@ -5913,9 +5937,9 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
                        if (ext->is_set) {
                                insn[0].imm = ext->ksym.kernel_btf_id;
                                insn[0].off = ext->ksym.btf_fd_idx;
-                       } else { /* unresolved weak kfunc */
-                               insn[0].imm = 0;
-                               insn[0].off = 0;
+                       } else { /* unresolved weak kfunc call */
+                               poison_kfunc_call(prog, i, relo->insn_idx, insn,
+                                                 relo->ext_idx, ext);
                        }
                        break;
                case RELO_SUBPROG_ADDR:
@@ -7052,6 +7076,39 @@ static void fixup_log_missing_map_load(struct bpf_program *prog,
        patch_log(buf, buf_sz, log_sz, line1, line3 - line1, patch);
 }
 
+static void fixup_log_missing_kfunc_call(struct bpf_program *prog,
+                                        char *buf, size_t buf_sz, size_t log_sz,
+                                        char *line1, char *line2, char *line3)
+{
+       /* Expected log for failed and not properly guarded kfunc call:
+        * line1 -> 123: (85) call unknown#2002000345
+        * line2 -> invalid func unknown#2002000345
+        * line3 -> <anything else or end of buffer>
+        *
+        * "123" is the index of the instruction that was poisoned.
+        * "345" in "2002000345" is an extern index in obj->externs to fetch kfunc name.
+        */
+       struct bpf_object *obj = prog->obj;
+       const struct extern_desc *ext;
+       int insn_idx, ext_idx;
+       char patch[128];
+
+       if (sscanf(line1, "%d: (%*d) call unknown#%d\n", &insn_idx, &ext_idx) != 2)
+               return;
+
+       ext_idx -= POISON_CALL_KFUNC_BASE;
+       if (ext_idx < 0 || ext_idx >= obj->nr_extern)
+               return;
+       ext = &obj->externs[ext_idx];
+
+       snprintf(patch, sizeof(patch),
+                "%d: <invalid kfunc call>\n"
+                "kfunc '%s' is referenced but wasn't resolved\n",
+                insn_idx, ext->name);
+
+       patch_log(buf, buf_sz, log_sz, line1, line3 - line1, patch);
+}
+
 static void fixup_verifier_log(struct bpf_program *prog, char *buf, size_t buf_sz)
 {
        /* look for familiar error patterns in last N lines of the log */
@@ -7089,6 +7146,15 @@ static void fixup_verifier_log(struct bpf_program *prog, char *buf, size_t buf_s
                        fixup_log_missing_map_load(prog, buf, buf_sz, log_sz,
                                                   prev_line, cur_line, next_line);
                        return;
+               } else if (str_has_pfx(cur_line, "invalid func unknown#"POISON_CALL_KFUNC_PFX)) {
+                       prev_line = find_prev_line(buf, cur_line);
+                       if (!prev_line)
+                               continue;
+
+                       /* reference to unresolved kfunc */
+                       fixup_log_missing_kfunc_call(prog, buf, buf_sz, log_sz,
+                                                    prev_line, cur_line, next_line);
+                       return;
                }
        }
 }