[PATCH bpf-next v1 0/6] Typeless/weak ksym for gen_loader + misc fixups

Patches (1,2,4) add typeless and weak ksym support to gen_loader. It is follow
up for the recent kfunc from modules series.

The later patches (5,6) are misc fixes for selftests, and patch 3 for libbpf
where we try to be careful to not end up with mod_btf->fd set as 0 (as that
leads to a confusing error message about btf_id not being found on load and it
is not clear what went wrong, instead we can just dup fd 0).

Kumar Kartikeya Dwivedi (6):
  bpf: Add bpf_kallsyms_lookup_name helper
  libbpf: Add typeless and weak ksym support to gen_loader
  libbpf: Ensure that module BTF fd is never 0
  bpf: selftests: Move test_ksyms_weak test to lskel, add libbpf test
  bpf: selftests: Fix fd cleanup in sk_lookup test
  bpf: selftests: Fix memory leak in test_ima

 include/linux/bpf.h                           |   1 +
 include/uapi/linux/bpf.h                      |  14 ++
 kernel/bpf/syscall.c                          |  24 ++++
 tools/include/uapi/linux/bpf.h                |  14 ++
 tools/lib/bpf/bpf_gen_internal.h              |  12 +-
 tools/lib/bpf/gen_loader.c                    | 123 ++++++++++++++++--
 tools/lib/bpf/libbpf.c                        |  27 ++--
 tools/testing/selftests/bpf/Makefile          |   2 +-
 .../selftests/bpf/prog_tests/ksyms_btf.c      |   6 +-
 .../bpf/prog_tests/ksyms_weak_libbpf.c        |  31 +++++
 .../selftests/bpf/prog_tests/sk_lookup.c      |  20 ++-
 .../selftests/bpf/prog_tests/test_ima.c       |   3 +-
 .../selftests/bpf/progs/test_ksyms_weak.c     |   3 +-
 13 files changed, 247 insertions(+), 33 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c

--
2.33.0

This helper allows us to get the address of a kernel symbol from inside
a BPF_PROG_TYPE_SYSCALL prog (used by gen_loader), so that we can
relocate weak and typeless ksym vars.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf.h            |  1 +
 include/uapi/linux/bpf.h       | 14 ++++++++++++++
 kernel/bpf/syscall.c           | 24 ++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 14 ++++++++++++++
 4 files changed, 53 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d604c8251d88..17206aae329d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h

@@ -2107,6 +2107,7 @@ extern const struct bpf_func_proto bpf_for_each_map_elem_proto;
 extern const struct bpf_func_proto bpf_btf_find_by_name_kind_proto;
 extern const struct bpf_func_proto bpf_sk_setsockopt_proto;
 extern const struct bpf_func_proto bpf_sk_getsockopt_proto;
+extern const struct bpf_func_proto bpf_kallsyms_lookup_name_proto;
 
 const struct bpf_func_proto *tracing_prog_func_proto(
   enum bpf_func_id func_id, const struct bpf_prog *prog);

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6fc59d61937a..bbd0a3f4e5f6 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h

@@ -4909,6 +4909,19 @@ union bpf_attr {
  *	Return
  *		The number of bytes written to the buffer, or a negative error
  *		in case of failure.
+ *
+ * long bpf_kallsyms_lookup_name(const char *name, int name_sz, int flags, u64 *res)
+ *	Description
+ *		Get the address of a kernel symbol, returned in *res*. *res* is
+ *		set to 0 if the symbol is not found.
+ *	Return
+ *		On success, zero. On error, a negative value.
+ *
+ *		**-EINVAL** if *flags* is not zero.
+ *
+ *		**-EINVAL** if string *name* is not the same size as *name_sz*.
+ *
+ *		**-ENOENT** if symbol is not found.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -5089,6 +5102,7 @@ union bpf_attr {
 	FN(task_pt_regs),		\
 	FN(get_branch_snapshot),	\
 	FN(trace_vprintk),		\
+	FN(kallsyms_lookup_name),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4e50c0bfdb7d..073ca9ebe58b 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c

@@ -4753,6 +4753,28 @@ static const struct bpf_func_proto bpf_sys_close_proto = {
 	.arg1_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_4(bpf_kallsyms_lookup_name, const char *, name, int, name_sz, int, flags, u64 *, res)
+{
+	if (flags)
+		return -EINVAL;
+
+	if (name_sz <= 1 || name[name_sz - 1])
+		return -EINVAL;
+
+	*res = kallsyms_lookup_name(name);
+	return *res ? 0 : -ENOENT;
+}
+
+const struct bpf_func_proto bpf_kallsyms_lookup_name_proto = {
+	.func		= bpf_kallsyms_lookup_name,
+	.gpl_only	= true,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_MEM,
+	.arg2_type	= ARG_CONST_SIZE,
+	.arg3_type	= ARG_ANYTHING,
+	.arg4_type	= ARG_PTR_TO_LONG,
+};
+
 static const struct bpf_func_proto *
 syscall_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {

@@ -4763,6 +4785,8 @@ syscall_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_btf_find_by_name_kind_proto;
 	case BPF_FUNC_sys_close:
 		return &bpf_sys_close_proto;
+	case BPF_FUNC_kallsyms_lookup_name:
+		return &bpf_kallsyms_lookup_name_proto;
 	default:
 		return tracing_prog_func_proto(func_id, prog);
 	}

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 6fc59d61937a..bbd0a3f4e5f6 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h

@@ -4909,6 +4909,19 @@ union bpf_attr {
  *	Return
  *		The number of bytes written to the buffer, or a negative error
  *		in case of failure.
+ *
+ * long bpf_kallsyms_lookup_name(const char *name, int name_sz, int flags, u64 *res)
+ *	Description
+ *		Get the address of a kernel symbol, returned in *res*. *res* is
+ *		set to 0 if the symbol is not found.
+ *	Return
+ *		On success, zero. On error, a negative value.
+ *
+ *		**-EINVAL** if *flags* is not zero.
+ *
+ *		**-EINVAL** if string *name* is not the same size as *name_sz*.
+ *
+ *		**-ENOENT** if symbol is not found.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\

@@ -5089,6 +5102,7 @@ union bpf_attr {
 	FN(task_pt_regs),		\
 	FN(get_branch_snapshot),	\
 	FN(trace_vprintk),		\
+	FN(kallsyms_lookup_name),	\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper

-- 
2.33.0

This patch adds typeless and weak ksym support to BTF_KIND_VAR
relocation code in gen_loader. For typeless ksym, we use the newly added
bpf_kallsyms_lookup_name helper.

For weak ksym, we simply skip error check, and fix up the srg_reg for
the insn, as keeping it as BPF_PSEUDO_BTF_ID for weak ksym with its
insn[0].imm and insn[1].imm set as 0 will cause a failure.  This is
consistent with how libbpf relocates these two cases of BTF_KIND_VAR.

We also modify cleanup_relos to check for typeless ksyms in fd closing
loop, since those have no fd associated with the ksym. For this we can
reuse the unused 'off' member of ksym_desc.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/lib/bpf/bpf_gen_internal.h |  12 ++-
 tools/lib/bpf/gen_loader.c       | 123 ++++++++++++++++++++++++++++---
 tools/lib/bpf/libbpf.c           |  13 ++--
 3 files changed, 128 insertions(+), 20 deletions(-)

diff --git a/tools/lib/bpf/bpf_gen_internal.h b/tools/lib/bpf/bpf_gen_internal.h
index 70eccbffefb1..9f328d044636 100644
--- a/tools/lib/bpf/bpf_gen_internal.h
+++ b/tools/lib/bpf/bpf_gen_internal.h

@@ -8,13 +8,19 @@ struct ksym_relo_desc {
 	int kind;
 	int insn_idx;
 	bool is_weak;
+	bool is_typeless;
 };
 
 struct ksym_desc {
 	const char *name;
 	int ref;
 	int kind;
-	int off;
+	union {
+		/* used for kfunc */
+		int off;
+		/* used for typeless ksym */
+		bool typeless;
+	};
 	int insn;
 };
 

@@ -49,7 +55,7 @@ void bpf_gen__prog_load(struct bpf_gen *gen, struct bpf_prog_load_params *load_a
 void bpf_gen__map_update_elem(struct bpf_gen *gen, int map_idx, void *value, __u32 value_size);
 void bpf_gen__map_freeze(struct bpf_gen *gen, int map_idx);
 void bpf_gen__record_attach_target(struct bpf_gen *gen, const char *name, enum bpf_attach_type type);
-void bpf_gen__record_extern(struct bpf_gen *gen, const char *name, bool is_weak, int kind,
-			    int insn_idx);
+void bpf_gen__record_extern(struct bpf_gen *gen, const char *name, bool is_weak,
+			    bool is_typeless, int kind, int insn_idx);
 
 #endif

diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
index 937bfc7db41e..da1fcb0e3bcb 100644
--- a/tools/lib/bpf/gen_loader.c
+++ b/tools/lib/bpf/gen_loader.c

@@ -13,6 +13,7 @@
 #include "hashmap.h"
 #include "bpf_gen_internal.h"
 #include "skel_internal.h"
+#include <asm/byteorder.h>
 
 #define MAX_USED_MAPS	64
 #define MAX_USED_PROGS	32

@@ -559,7 +560,7 @@ static void emit_find_attach_target(struct bpf_gen *gen)
 }
 
 void bpf_gen__record_extern(struct bpf_gen *gen, const char *name, bool is_weak,
-			    int kind, int insn_idx)
+			    bool is_typeless, int kind, int insn_idx)
 {
 	struct ksym_relo_desc *relo;
 

@@ -572,6 +573,7 @@ void bpf_gen__record_extern(struct bpf_gen *gen, const char *name, bool is_weak,
 	relo += gen->relo_cnt;
 	relo->name = name;
 	relo->is_weak = is_weak;
+	relo->is_typeless = is_typeless;
 	relo->kind = kind;
 	relo->insn_idx = insn_idx;
 	gen->relo_cnt++;

@@ -621,6 +623,33 @@ static void emit_bpf_find_by_name_kind(struct bpf_gen *gen, struct ksym_relo_des
 	debug_ret(gen, "find_by_name_kind(%s,%d)", relo->name, relo->kind);
 }
 
+/* Overwrites BPF_REG_{0, 1, 2, 3, 4, 7}
+ * Returns result in BPF_REG_7
+ * Returns u64 symbol addr in BPF_REG_9
+ */
+static void emit_bpf_kallsyms_lookup_name(struct bpf_gen *gen, struct ksym_relo_desc *relo)
+{
+	int name_off, len = strlen(relo->name) + 1, res_off;
+
+	name_off = add_data(gen, relo->name, len);
+	if (!name_off)
+		return;
+	res_off = add_data(gen, NULL, 8); /* res is u64 */
+	if (!res_off)
+		return;
+	emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_1, BPF_PSEUDO_MAP_IDX_VALUE,
+					 0, 0, 0, name_off));
+	emit(gen, BPF_MOV64_IMM(BPF_REG_2, len));
+	emit(gen, BPF_MOV64_IMM(BPF_REG_3, 0));
+	emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_4, BPF_PSEUDO_MAP_IDX_VALUE,
+					 0, 0, 0, res_off));
+	emit(gen, BPF_MOV64_REG(BPF_REG_7, BPF_REG_4));
+	emit(gen, BPF_EMIT_CALL(BPF_FUNC_kallsyms_lookup_name));
+	emit(gen, BPF_LDX_MEM(BPF_DW, BPF_REG_9, BPF_REG_7, 0));
+	emit(gen, BPF_MOV64_REG(BPF_REG_7, BPF_REG_0));
+	debug_ret(gen, "kallsyms_lookup_name(%s,%d)", relo->name, relo->kind);
+}
+
 /* Expects:
  * BPF_REG_8 - pointer to instruction
  *

@@ -703,7 +732,8 @@ static void emit_relo_kfunc_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo
 /* Expects:
  * BPF_REG_8 - pointer to instruction
  */
-static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo, int insn)
+static void emit_relo_ksym_typeless(struct bpf_gen *gen,
+				    struct ksym_relo_desc *relo, int insn)
 {
 	struct ksym_desc *kdesc;
 

@@ -718,25 +748,96 @@ static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo,
 			       kdesc->insn + sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm));
 		goto log;
 	}
+	/* remember insn offset, so we can copy ksym addr later */
+	kdesc->insn = insn;
+	/* skip typeless ksym_desc in fd closing loop in cleanup_relos */
+	kdesc->typeless = true;
+	emit_bpf_kallsyms_lookup_name(gen, relo);
+	emit(gen, BPF_JMP_IMM(BPF_JEQ, BPF_REG_7, -ENOENT, 1));
+	emit_check_err(gen);
+	/* store lower half of addr into insn[insn_idx].imm */
+	emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_9, offsetof(struct bpf_insn, imm)));
+	/* store upper half of addr into insn[insn_idx + 1].imm */
+	emit(gen, BPF_ALU64_IMM(BPF_RSH, BPF_REG_9, 32));
+	emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_9,
+		      sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm)));
+log:
+	if (!gen->log_level)
+		return;
+	emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_8,
+			      offsetof(struct bpf_insn, imm)));
+	emit(gen, BPF_LDX_MEM(BPF_H, BPF_REG_9, BPF_REG_8, sizeof(struct bpf_insn) +
+			      offsetof(struct bpf_insn, imm)));
+	debug_regs(gen, BPF_REG_7, BPF_REG_9, " var t=0 w=%d (%s:count=%d): imm[0]: %%d, imm[1]: %%d",
+		   relo->is_weak, relo->name, kdesc->ref);
+	emit(gen, BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_8, offsetofend(struct bpf_insn, code)));
+	debug_regs(gen, BPF_REG_9, -1, " var t=0 w=%d (%s:count=%d): insn.reg",
+		   relo->is_weak, relo->name, kdesc->ref);
+}
+
+/* Expects:
+ * BPF_REG_8 - pointer to instruction
+ */
+static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo, int insn)
+{
+	struct ksym_desc *kdesc;
+	__u32 reg_mask;
+
+	kdesc = get_ksym_desc(gen, relo);
+	if (!kdesc)
+		return;
+	/* try to copy from existing ldimm64 insn */
+	if (kdesc->ref > 1) {
+		move_blob2blob(gen, insn + offsetof(struct bpf_insn, imm), 4,
+			       kdesc->insn + offsetof(struct bpf_insn, imm));
+		move_blob2blob(gen, insn + sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm), 4,
+			       kdesc->insn + sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm));
+		emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_9, BPF_REG_8, offsetof(struct bpf_insn, imm)));
+		/* jump over src_reg adjustment if imm is not 0 */
+		emit(gen, BPF_JMP_IMM(BPF_JNE, BPF_REG_9, 0, 3));
+		goto clear_src_reg;
+	}
 	/* remember insn offset, so we can copy BTF ID and FD later */
 	kdesc->insn = insn;
 	emit_bpf_find_by_name_kind(gen, relo);
-	emit_check_err(gen);
+	if (!relo->is_weak)
+		emit_check_err(gen);
+	/* set default values as 0 */
+	emit(gen, BPF_ST_MEM(BPF_W, BPF_REG_8, offsetof(struct bpf_insn, imm), 0));
+	emit(gen, BPF_ST_MEM(BPF_W, BPF_REG_8, sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm), 0));
+	/* skip success case stores if ret < 0 */
+	emit(gen, BPF_JMP_IMM(BPF_JSLT, BPF_REG_7, 0, 4));
 	/* store btf_id into insn[insn_idx].imm */
 	emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_7, offsetof(struct bpf_insn, imm)));
 	/* store btf_obj_fd into insn[insn_idx + 1].imm */
 	emit(gen, BPF_ALU64_IMM(BPF_RSH, BPF_REG_7, 32));
 	emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_7,
 			      sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm)));
-log:
+	emit(gen, BPF_JMP_IMM(BPF_JSGE, BPF_REG_7, 0, 3));
+clear_src_reg:
+	/* clear bpf_object__relocate_data's src_reg assignment, otherwise we get a verifier failure */
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	reg_mask = 0x0f; /* src_reg,dst_reg,... */
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	reg_mask = 0xf0; /* dst_reg,src_reg,... */
+#else
+#error "Unsupported bit endianness, cannot proceed"
+#endif
+	emit(gen, BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_8, offsetofend(struct bpf_insn, code)));
+	emit(gen, BPF_ALU32_IMM(BPF_AND, BPF_REG_9, reg_mask));
+	emit(gen, BPF_STX_MEM(BPF_B, BPF_REG_8, BPF_REG_9, offsetofend(struct bpf_insn, code)));
+
 	if (!gen->log_level)
 		return;
 	emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_8,
 			      offsetof(struct bpf_insn, imm)));
 	emit(gen, BPF_LDX_MEM(BPF_H, BPF_REG_9, BPF_REG_8, sizeof(struct bpf_insn) +
 			      offsetof(struct bpf_insn, imm)));
-	debug_regs(gen, BPF_REG_7, BPF_REG_9, " var (%s:count=%d): imm: %%d, fd: %%d",
-		   relo->name, kdesc->ref);
+	debug_regs(gen, BPF_REG_7, BPF_REG_9, " var t=1 w=%d (%s:count=%d): imm[0]: %%d, imm[1]: %%d",
+		   relo->is_weak, relo->name, kdesc->ref);
+	emit(gen, BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_8, offsetofend(struct bpf_insn, code)));
+	debug_regs(gen, BPF_REG_9, -1, " var t=1 w=%d (%s:count=%d): insn.reg",
+		   relo->is_weak, relo->name, kdesc->ref);
 }
 
 static void emit_relo(struct bpf_gen *gen, struct ksym_relo_desc *relo, int insns)

@@ -748,7 +849,10 @@ static void emit_relo(struct bpf_gen *gen, struct ksym_relo_desc *relo, int insn
 	emit2(gen, BPF_LD_IMM64_RAW_FULL(BPF_REG_8, BPF_PSEUDO_MAP_IDX_VALUE, 0, 0, 0, insn));
 	switch (relo->kind) {
 	case BTF_KIND_VAR:
-		emit_relo_ksym_btf(gen, relo, insn);
+		if (relo->is_typeless)
+			emit_relo_ksym_typeless(gen, relo, insn);
+		else
+			emit_relo_ksym_btf(gen, relo, insn);
 		break;
 	case BTF_KIND_FUNC:
 		emit_relo_kfunc_btf(gen, relo, insn);

@@ -773,12 +877,13 @@ static void cleanup_relos(struct bpf_gen *gen, int insns)
 	int i, insn;
 
 	for (i = 0; i < gen->nr_ksyms; i++) {
-		if (gen->ksyms[i].kind == BTF_KIND_VAR) {
+		/* only close fds for typed ksyms and kfuncs */
+		if (gen->ksyms[i].kind == BTF_KIND_VAR && !gen->ksyms[i].typeless) {
 			/* close fd recorded in insn[insn_idx + 1].imm */
 			insn = gen->ksyms[i].insn;
 			insn += sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm);
 			emit_sys_close_blob(gen, insn);
-		} else { /* BTF_KIND_FUNC */
+		} else if (gen->ksyms[i].kind == BTF_KIND_FUNC) {
 			emit_sys_close_blob(gen, blob_fd_array_off(gen, gen->ksyms[i].off));
 			if (gen->ksyms[i].off < MAX_FD_ARRAY_SZ)
 				gen->nr_fd_array--;

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index f32fa51b1e63..d286dec73b5f 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -6355,17 +6355,14 @@ static int bpf_program__record_externs(struct bpf_program *prog)
 		case RELO_EXTERN_VAR:
 			if (ext->type != EXT_KSYM)
 				continue;
-			if (!ext->ksym.type_id) {
-				pr_warn("typeless ksym %s is not supported yet\n",
-					ext->name);
-				return -ENOTSUP;
-			}
-			bpf_gen__record_extern(obj->gen_loader, ext->name, ext->is_weak,
+			bpf_gen__record_extern(obj->gen_loader, ext->name,
+					       ext->is_weak, !ext->ksym.type_id,
 					       BTF_KIND_VAR, relo->insn_idx);
 			break;
 		case RELO_EXTERN_FUNC:
-			bpf_gen__record_extern(obj->gen_loader, ext->name, ext->is_weak,
-					       BTF_KIND_FUNC, relo->insn_idx);
+			bpf_gen__record_extern(obj->gen_loader, ext->name,
+					       ext->is_weak, 0, BTF_KIND_FUNC,
+					       relo->insn_idx);
 			break;
 		default:
 			continue;

-- 
2.33.0

Since the code assumes in various places that BTF fd for modules is
never 0, if we end up getting fd as 0, obtain a new fd > 0. Even though
fd 0 being free for allocation is usually an application error, it is
still possible that we end up getting fd 0 if the application explicitly
closes its stdin. Deal with this by getting a new fd using dup and
closing fd 0.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/lib/bpf/libbpf.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d286dec73b5f..3e5e460fe63e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -4975,6 +4975,20 @@ static int load_module_btfs(struct bpf_object *obj)
 			pr_warn("failed to get BTF object #%d FD: %d\n", id, err);
 			return err;
 		}
+		/* Make sure module BTF fd is never 0, as kernel depends on it
+		 * being > 0 to distinguish between vmlinux and module BTFs,
+		 * e.g. for BPF_PSEUDO_BTF_ID ld_imm64 insns (ksyms).
+		 */
+		if (!fd) {
+			fd = dup(0);
+			if (fd < 0) {
+				err = -errno;
+				pr_warn("failed to dup BTF object #%d FD 0 to FD > 0: %d\n", id, err);
+				close(0);
+				return err;
+			}
+			close(0);
+		}
 
 		len = sizeof(info);
 		memset(&info, 0, sizeof(info));

-- 
2.33.0

Also, avoid using CO-RE features, as lskel doesn't support CO-RE, yet.
Create a file for testing libbpf skeleton as well, so that both
gen_loader and libbpf get tested.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/testing/selftests/bpf/Makefile          |  2 +-
 .../selftests/bpf/prog_tests/ksyms_btf.c      |  6 ++--
 .../bpf/prog_tests/ksyms_weak_libbpf.c        | 31 +++++++++++++++++++
 .../selftests/bpf/progs/test_ksyms_weak.c     |  3 +-
 4 files changed, 36 insertions(+), 6 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index c5c9a9f50d8d..4ae5bc852ca8 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile

@@ -317,7 +317,7 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h		\
 LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
 	test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c
 # Generate both light skeleton and libbpf skeleton for these
-LSKELS_EXTRA := test_ksyms_module.c
+LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c
 SKEL_BLACKLIST += $$(LSKELS)
 
 test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o

diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c
index cf3acfa5a91d..21ff02d47076 100644
--- a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c

@@ -6,7 +6,7 @@
 #include <bpf/btf.h>
 #include "test_ksyms_btf.skel.h"
 #include "test_ksyms_btf_null_check.skel.h"
-#include "test_ksyms_weak.skel.h"
+#include "test_ksyms_weak.lskel.h"
 
 static int duration;
 

@@ -89,11 +89,11 @@ static void test_weak_syms(void)
 	int err;
 
 	skel = test_ksyms_weak__open_and_load();
-	if (CHECK(!skel, "test_ksyms_weak__open_and_load", "failed\n"))
+	if (!ASSERT_OK_PTR(skel, "test_ksyms_weak__open_and_load"))
 		return;
 
 	err = test_ksyms_weak__attach(skel);
-	if (CHECK(err, "test_ksyms_weak__attach", "skeleton attach failed: %d\n", err))
+	if (!ASSERT_OK(err, "test_ksyms_weak__attach"))
 		goto cleanup;
 
 	/* trigger tracepoint */

diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c
new file mode 100644
index 000000000000..b75725e28647
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/ksyms_weak_libbpf.c

@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "test_ksyms_weak.skel.h"
+
+void test_ksyms_weak_libbpf(void)
+{
+	struct test_ksyms_weak *skel;
+	struct test_ksyms_weak__data *data;
+	int err;
+
+	skel = test_ksyms_weak__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_ksyms_weak__open_and_load"))
+		return;
+
+	err = test_ksyms_weak__attach(skel);
+	if (!ASSERT_OK(err, "test_ksyms_weak__attach"))
+		goto cleanup;
+
+	/* trigger tracepoint */
+	usleep(1);
+
+	data = skel->data;
+	ASSERT_EQ(data->out__existing_typed, 0, "existing typed ksym");
+	ASSERT_NEQ(data->out__existing_typeless, -1, "existing typeless ksym");
+	ASSERT_EQ(data->out__non_existent_typeless, 0, "nonexistent typeless ksym");
+	ASSERT_EQ(data->out__non_existent_typed, 0, "nonexistent typed ksym");
+
+cleanup:
+	test_ksyms_weak__destroy(skel);
+}

diff --git a/tools/testing/selftests/bpf/progs/test_ksyms_weak.c b/tools/testing/selftests/bpf/progs/test_ksyms_weak.c
index 5f8379aadb29..521e7b99db08 100644
--- a/tools/testing/selftests/bpf/progs/test_ksyms_weak.c
+++ b/tools/testing/selftests/bpf/progs/test_ksyms_weak.c

@@ -21,7 +21,6 @@ __u64 out__non_existent_typed = -1;
 extern const struct rq runqueues __ksym __weak; /* typed */
 extern const void bpf_prog_active __ksym __weak; /* typeless */
 
-
 /* non-existent weak symbols. */
 
 /* typeless symbols, default to zero. */

@@ -38,7 +37,7 @@ int pass_handler(const void *ctx)
 	/* tests existing symbols. */
 	rq = (struct rq *)bpf_per_cpu_ptr(&runqueues, 0);
 	if (rq)
-		out__existing_typed = rq->cpu;
+		out__existing_typed = 0;
 	out__existing_typeless = (__u64)&bpf_prog_active;
 
 	/* tests non-existent symbols. */

-- 
2.33.0

Similar to the fix in commit:
e31eec77e4ab (bpf: selftests: Fix fd cleanup in get_branch_snapshot)

We use memset to set fds to -1 without breaking on future changes to
the array size (when MAX_SERVER constant is bumped).

The particular close(0) occurs on non-reuseport tests, so it can be seen
with -n 115/{2,3} but not 115/4. This can cause problems with future
tests if they depend on BTF fd never being acquired as fd 0, breaking
internal libbpf assumptions.

Cc: Jakub Sitnicki <jakub@cloudflare.com>
Fixes: 0ab5539f8584 (selftests/bpf: Tests for BPF_SK_LOOKUP attach point)
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/prog_tests/sk_lookup.c      | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
index aee41547e7f4..572220065bdf 100644
--- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
+++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c

@@ -598,11 +598,14 @@ static void query_lookup_prog(struct test_sk_lookup *skel)
 
 static void run_lookup_prog(const struct test *t)
 {
-	int server_fds[MAX_SERVERS] = { -1 };
 	int client_fd, reuse_conn_fd = -1;
 	struct bpf_link *lookup_link;
+	int server_fds[MAX_SERVERS];
 	int i, err;
 
+	/* set all fds to -1 */
+	memset(server_fds, 0xFF, sizeof(server_fds));
+
 	lookup_link = attach_lookup_prog(t->lookup_prog);
 	if (!lookup_link)
 		return;

@@ -663,8 +666,9 @@ static void run_lookup_prog(const struct test *t)
 	if (reuse_conn_fd != -1)
 		close(reuse_conn_fd);
 	for (i = 0; i < ARRAY_SIZE(server_fds); i++) {
-		if (server_fds[i] != -1)
-			close(server_fds[i]);
+		if (server_fds[i] == -1)
+			break;
+		close(server_fds[i]);
 	}
 	bpf_link__destroy(lookup_link);
 }

@@ -1053,11 +1057,14 @@ static void run_sk_assign(struct test_sk_lookup *skel,
 			  struct bpf_program *lookup_prog,
 			  const char *remote_ip, const char *local_ip)
 {
-	int server_fds[MAX_SERVERS] = { -1 };
+	int server_fds[MAX_SERVERS];
 	struct bpf_sk_lookup ctx;
 	__u64 server_cookie;
 	int i, err;
 
+	/* set all fds to -1 */
+	memset(server_fds, 0xFF, sizeof(server_fds));
+
 	DECLARE_LIBBPF_OPTS(bpf_test_run_opts, opts,
 		.ctx_in = &ctx,
 		.ctx_size_in = sizeof(ctx),

@@ -1097,8 +1104,9 @@ static void run_sk_assign(struct test_sk_lookup *skel,
 
 close_servers:
 	for (i = 0; i < ARRAY_SIZE(server_fds); i++) {
-		if (server_fds[i] != -1)
-			close(server_fds[i]);
+		if (server_fds[i] == -1)
+			break;
+		close(server_fds[i]);
 	}
 }
 

-- 
2.33.0

The allocated ring buffer is never freed, do so in the cleanup path.

Fixes: f446b570ac7e (bpf/selftests: Update the IMA test to use BPF ring buffer)
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/test_ima.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c
index 0252f61d611a..97d8a6f84f4a 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_ima.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c

@@ -43,7 +43,7 @@ static int process_sample(void *ctx, void *data, size_t len)
 void test_test_ima(void)
 {
 	char measured_dir_template[] = "/tmp/ima_measuredXXXXXX";
-	struct ring_buffer *ringbuf;
+	struct ring_buffer *ringbuf = NULL;
 	const char *measured_dir;
 	char cmd[256];
 

@@ -85,5 +85,6 @@ void test_test_ima(void)
 	err = system(cmd);
 	CHECK(err, "failed to run command", "%s, errno = %d\n", cmd, errno);
 close_prog:
+	ring_buffer__free(ringbuf);
 	ima__destroy(skel);
 }

-- 
2.33.0

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

This is not the only place where we make assumptions that fd > 0 but
technically can get fd == 0. Instead of doing such a check in every
such place, would it be possible to open (cheaply) some FD (/dev/null
or whatever, don't know what's the best file to open), if we detect
that FD == 0 is not allocated? Can we detect that fd 0 is not
allocated?

Doing something like that in bpf_object__open() or bpf_object__load()
would make everything much simpler and we'll have a guarantee that fd
== 0 is not going to be allocated (unless someone accidentally or not
accidentally does close(0), but that's entirely different story).

+                       if (fd < 0) {
+                               err = -errno;
+                               pr_warn("failed to dup BTF object #%d FD 0 to FD > 0: %d\n", id, err);
+                               close(0);
+                               return err;
+                       }
+                       close(0);
+               }

                len = sizeof(info);
                memset(&info, 0, sizeof(info));
--
2.33.0

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

The allocated ring buffer is never freed, do so in the cleanup path.

Fixes: f446b570ac7e (bpf/selftests: Update the IMA test to use BPF ring buffer)
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---

Please stick to "selftests/bpf: " prefix which we use consistently for
BPF selftests patches.

Other than that LGTM.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote:

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

quoted

Since the code assumes in various places that BTF fd for modules is
never 0, if we end up getting fd as 0, obtain a new fd > 0. Even though
fd 0 being free for allocation is usually an application error, it is
still possible that we end up getting fd 0 if the application explicitly
closes its stdin. Deal with this by getting a new fd using dup and
closing fd 0.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/lib/bpf/libbpf.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d286dec73b5f..3e5e460fe63e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -4975,6 +4975,20 @@ static int load_module_btfs(struct bpf_object *obj)
                        pr_warn("failed to get BTF object #%d FD: %d\n", id, err);
                        return err;
                }
+               /* Make sure module BTF fd is never 0, as kernel depends on it
+                * being > 0 to distinguish between vmlinux and module BTFs,
+                * e.g. for BPF_PSEUDO_BTF_ID ld_imm64 insns (ksyms).
+                */
+               if (!fd) {
+                       fd = dup(0);

This is not the only place where we make assumptions that fd > 0 but
technically can get fd == 0. Instead of doing such a check in every
such place, would it be possible to open (cheaply) some FD (/dev/null
or whatever, don't know what's the best file to open), if we detect
that FD == 0 is not allocated? Can we detect that fd 0 is not
allocated?

We can, e.g. using access("/proc/self/fd/0", F_OK), but I think just calling
open unconditonally and doing if (ret > 0) close(ret) is better. Also, do I
leave it lingering, or should I close(0) if we created it on destroy?

Doing something like that in bpf_object__open() or bpf_object__load()
would make everything much simpler and we'll have a guarantee that fd
== 0 is not going to be allocated (unless someone accidentally or not
accidentally does close(0), but that's entirely different story).

quoted

+                       if (fd < 0) {
+                               err = -errno;
+                               pr_warn("failed to dup BTF object #%d FD 0 to FD > 0: %d\n", id, err);
+                               close(0);
+                               return err;
+                       }
+                       close(0);
+               }

                len = sizeof(info);
                memset(&info, 0, sizeof(info));
--
2.33.0

--
Kartikeya

On Wed, Oct 06, 2021 at 02:28 AM CEST, Kumar Kartikeya Dwivedi wrote:

My mistake. Thanks for the fix.

Alternatively we can use an extended designated initializer:

int server_fds[] = { [0 ... MAX_SERVERS] = -1 };

Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>

On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote:

quoted

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

quoted

Since the code assumes in various places that BTF fd for modules is
never 0, if we end up getting fd as 0, obtain a new fd > 0. Even though
fd 0 being free for allocation is usually an application error, it is
still possible that we end up getting fd 0 if the application explicitly
closes its stdin. Deal with this by getting a new fd using dup and
closing fd 0.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/lib/bpf/libbpf.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d286dec73b5f..3e5e460fe63e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -4975,6 +4975,20 @@ static int load_module_btfs(struct bpf_object *obj)
                        pr_warn("failed to get BTF object #%d FD: %d\n", id, err);
                        return err;
                }
+               /* Make sure module BTF fd is never 0, as kernel depends on it
+                * being > 0 to distinguish between vmlinux and module BTFs,
+                * e.g. for BPF_PSEUDO_BTF_ID ld_imm64 insns (ksyms).
+                */
+               if (!fd) {
+                       fd = dup(0);

This is not the only place where we make assumptions that fd > 0 but
technically can get fd == 0. Instead of doing such a check in every
such place, would it be possible to open (cheaply) some FD (/dev/null
or whatever, don't know what's the best file to open), if we detect
that FD == 0 is not allocated? Can we detect that fd 0 is not
allocated?

We can, e.g. using access("/proc/self/fd/0", F_OK), but I think just calling
open unconditonally and doing if (ret > 0) close(ret) is better. Also, do I

yeah, I like this idea, let's go with it

leave it lingering, or should I close(0) if we created it on destroy?

I don't mind leaving it open indefinitely, but can you please check
that it doesn't trigger LeakSanitizer errors?

quoted

Doing something like that in bpf_object__open() or bpf_object__load()
would make everything much simpler and we'll have a guarantee that fd
== 0 is not going to be allocated (unless someone accidentally or not
accidentally does close(0), but that's entirely different story).

quoted

+                       if (fd < 0) {
+                               err = -errno;
+                               pr_warn("failed to dup BTF object #%d FD 0 to FD > 0: %d\n", id, err);
+                               close(0);
+                               return err;
+                       }
+                       close(0);
+               }

                len = sizeof(info);
                memset(&info, 0, sizeof(info));
--
2.33.0

--
Kartikeya

On Wed, Oct 6, 2021 at 9:43 AM Andrii Nakryiko
[off-list ref] wrote:

On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

quoted

On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote:

quoted

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

quoted

Since the code assumes in various places that BTF fd for modules is
never 0, if we end up getting fd as 0, obtain a new fd > 0. Even though
fd 0 being free for allocation is usually an application error, it is
still possible that we end up getting fd 0 if the application explicitly
closes its stdin. Deal with this by getting a new fd using dup and
closing fd 0.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/lib/bpf/libbpf.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d286dec73b5f..3e5e460fe63e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -4975,6 +4975,20 @@ static int load_module_btfs(struct bpf_object *obj)
                        pr_warn("failed to get BTF object #%d FD: %d\n", id, err);
                        return err;
                }
+               /* Make sure module BTF fd is never 0, as kernel depends on it
+                * being > 0 to distinguish between vmlinux and module BTFs,
+                * e.g. for BPF_PSEUDO_BTF_ID ld_imm64 insns (ksyms).
+                */
+               if (!fd) {
+                       fd = dup(0);

This is not the only place where we make assumptions that fd > 0 but
technically can get fd == 0. Instead of doing such a check in every
such place, would it be possible to open (cheaply) some FD (/dev/null
or whatever, don't know what's the best file to open), if we detect
that FD == 0 is not allocated? Can we detect that fd 0 is not
allocated?

We can, e.g. using access("/proc/self/fd/0", F_OK), but I think just calling
open unconditonally and doing if (ret > 0) close(ret) is better. Also, do I

yeah, I like this idea, let's go with it

FYI some production environments may detect that FDs 0,1,2 are not
pointing to stdin, stdout, stderr and will force close whatever files are there
and open 0,1,2 with canonical files.

libbpf doesn't have to resort to such measures, but it would be prudent to
make libbpf operate on FDs > 2 for all bpf objects to make sure other
frameworks don't ruin libbpf's view of FDs.

On Wed, Oct 6, 2021 at 12:09 PM Alexei Starovoitov
[off-list ref] wrote:

On Wed, Oct 6, 2021 at 9:43 AM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

quoted

On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote:

quoted

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

quoted

Since the code assumes in various places that BTF fd for modules is
never 0, if we end up getting fd as 0, obtain a new fd > 0. Even though
fd 0 being free for allocation is usually an application error, it is
still possible that we end up getting fd 0 if the application explicitly
closes its stdin. Deal with this by getting a new fd using dup and
closing fd 0.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/lib/bpf/libbpf.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d286dec73b5f..3e5e460fe63e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -4975,6 +4975,20 @@ static int load_module_btfs(struct bpf_object *obj)
                        pr_warn("failed to get BTF object #%d FD: %d\n", id, err);
                        return err;
                }
+               /* Make sure module BTF fd is never 0, as kernel depends on it
+                * being > 0 to distinguish between vmlinux and module BTFs,
+                * e.g. for BPF_PSEUDO_BTF_ID ld_imm64 insns (ksyms).
+                */
+               if (!fd) {
+                       fd = dup(0);

This is not the only place where we make assumptions that fd > 0 but
technically can get fd == 0. Instead of doing such a check in every
such place, would it be possible to open (cheaply) some FD (/dev/null
or whatever, don't know what's the best file to open), if we detect
that FD == 0 is not allocated? Can we detect that fd 0 is not
allocated?

We can, e.g. using access("/proc/self/fd/0", F_OK), but I think just calling
open unconditonally and doing if (ret > 0) close(ret) is better. Also, do I

yeah, I like this idea, let's go with it

FYI some production environments may detect that FDs 0,1,2 are not
pointing to stdin, stdout, stderr and will force close whatever files are there
and open 0,1,2 with canonical files.

libbpf doesn't have to resort to such measures, but it would be prudent to
make libbpf operate on FDs > 2 for all bpf objects to make sure other
frameworks don't ruin libbpf's view of FDs.

oh well, even without those production complications this would be a
bit fragile, e.g., if the application temporarily opened FD 0 and then
closed it.

Ok, Kumar, can you please do it as a simple helper that would
dup()'ing until we have FD>2, and use it in as few places as possible
to make sure that all FDs (not just module BTF) are covered. I'd
suggest doing that only in low-level helpers in btf.c, I think
libbpf's logic always goes through those anyways (but please
double-check that we don't call bpf syscall directly anywhere else).

Andrii Nakryiko [off-list ref] writes:

On Wed, Oct 6, 2021 at 12:09 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Wed, Oct 6, 2021 at 9:43 AM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

quoted

On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote:

quoted

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

quoted

Since the code assumes in various places that BTF fd for modules is
never 0, if we end up getting fd as 0, obtain a new fd > 0. Even though
fd 0 being free for allocation is usually an application error, it is
still possible that we end up getting fd 0 if the application explicitly
closes its stdin. Deal with this by getting a new fd using dup and
closing fd 0.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/lib/bpf/libbpf.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d286dec73b5f..3e5e460fe63e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -4975,6 +4975,20 @@ static int load_module_btfs(struct bpf_object *obj)
                        pr_warn("failed to get BTF object #%d FD: %d\n", id, err);
                        return err;
                }
+               /* Make sure module BTF fd is never 0, as kernel depends on it
+                * being > 0 to distinguish between vmlinux and module BTFs,
+                * e.g. for BPF_PSEUDO_BTF_ID ld_imm64 insns (ksyms).
+                */
+               if (!fd) {
+                       fd = dup(0);

This is not the only place where we make assumptions that fd > 0 but
technically can get fd == 0. Instead of doing such a check in every
such place, would it be possible to open (cheaply) some FD (/dev/null
or whatever, don't know what's the best file to open), if we detect
that FD == 0 is not allocated? Can we detect that fd 0 is not
allocated?

We can, e.g. using access("/proc/self/fd/0", F_OK), but I think just calling
open unconditonally and doing if (ret > 0) close(ret) is better. Also, do I

yeah, I like this idea, let's go with it

FYI some production environments may detect that FDs 0,1,2 are not
pointing to stdin, stdout, stderr and will force close whatever files are there
and open 0,1,2 with canonical files.

libbpf doesn't have to resort to such measures, but it would be prudent to
make libbpf operate on FDs > 2 for all bpf objects to make sure other
frameworks don't ruin libbpf's view of FDs.

oh well, even without those production complications this would be a
bit fragile, e.g., if the application temporarily opened FD 0 and then
closed it.

Ok, Kumar, can you please do it as a simple helper that would
dup()'ing until we have FD>2, and use it in as few places as possible
to make sure that all FDs (not just module BTF) are covered. I'd
suggest doing that only in low-level helpers in btf.c, I think
libbpf's logic always goes through those anyways (but please
double-check that we don't call bpf syscall directly anywhere else).

FYI, you can use fcntl() with F_DUPFD{,_CLOEXEC} and tell it the minimum
fd number you're interested in for the clone. We do that in libxdp to
protect against fd 0:

https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L1184

Given Alexei's comments above, maybe we should be '3' for the last arg
instead of 1...

-Toke

On Thu, Oct 07, 2021 at 03:54:34PM IST, Toke Høiland-Jørgensen wrote:

Andrii Nakryiko [off-list ref] writes:

quoted

On Wed, Oct 6, 2021 at 12:09 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Wed, Oct 6, 2021 at 9:43 AM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Tue, Oct 5, 2021 at 10:24 PM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

quoted

On Wed, Oct 06, 2021 at 10:11:29AM IST, Andrii Nakryiko wrote:

quoted

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

quoted

Since the code assumes in various places that BTF fd for modules is
never 0, if we end up getting fd as 0, obtain a new fd > 0. Even though
fd 0 being free for allocation is usually an application error, it is
still possible that we end up getting fd 0 if the application explicitly
closes its stdin. Deal with this by getting a new fd using dup and
closing fd 0.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 tools/lib/bpf/libbpf.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d286dec73b5f..3e5e460fe63e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -4975,6 +4975,20 @@ static int load_module_btfs(struct bpf_object *obj)
                        pr_warn("failed to get BTF object #%d FD: %d\n", id, err);
                        return err;
                }
+               /* Make sure module BTF fd is never 0, as kernel depends on it
+                * being > 0 to distinguish between vmlinux and module BTFs,
+                * e.g. for BPF_PSEUDO_BTF_ID ld_imm64 insns (ksyms).
+                */
+               if (!fd) {
+                       fd = dup(0);

This is not the only place where we make assumptions that fd > 0 but
technically can get fd == 0. Instead of doing such a check in every
such place, would it be possible to open (cheaply) some FD (/dev/null
or whatever, don't know what's the best file to open), if we detect
that FD == 0 is not allocated? Can we detect that fd 0 is not
allocated?

We can, e.g. using access("/proc/self/fd/0", F_OK), but I think just calling
open unconditonally and doing if (ret > 0) close(ret) is better. Also, do I

yeah, I like this idea, let's go with it

FYI some production environments may detect that FDs 0,1,2 are not
pointing to stdin, stdout, stderr and will force close whatever files are there
and open 0,1,2 with canonical files.

libbpf doesn't have to resort to such measures, but it would be prudent to
make libbpf operate on FDs > 2 for all bpf objects to make sure other
frameworks don't ruin libbpf's view of FDs.

oh well, even without those production complications this would be a
bit fragile, e.g., if the application temporarily opened FD 0 and then
closed it.

Ok, Kumar, can you please do it as a simple helper that would
dup()'ing until we have FD>2, and use it in as few places as possible
to make sure that all FDs (not just module BTF) are covered. I'd
suggest doing that only in low-level helpers in btf.c, I think
libbpf's logic always goes through those anyways (but please
double-check that we don't call bpf syscall directly anywhere else).

Just to make sure I am on the same page:

I have to...
1. Add a small wrapper (currently named fd_gt_2, any other suggestions?)
   that takes in the fd, and dups it to fd >= 3 if in range [0, 2] (and closes
   original fd in this case).
   Use this for all fd returning bpf syscalls in bpf.c (btf.c is a typo?).
   Audit other places directly calling syscall(__NR_bpf, ...).
2. Assume that the situation Alexei mentioned only occurs at startup, or
   sometime later, not in parallel (which would race with us, and not sure
   we can deal with it). I'm thinking of a case where such an fd gets passed
   to an exec'd binary which closes invalids fds on startup (so keeping them
   >= 3 allows proper inheritance).
3. gen_loader can hit the same case, so short of adding a bpf_sys_fcntl (or the
   helper only exposing F_DUPFD), next best option is to reserve the three fds from
   skel_internal.h or gen_trace (in bpftool) and close later after loading is done.

Feedback needed on 3 (and whether a generic bpf_sys_dup providing functionality of
existing fcntl and dup{,2,3} is better than simply reserving the three fds at
load time).

FYI, you can use fcntl() with F_DUPFD{,_CLOEXEC} and tell it the minimum
fd number you're interested in for the clone. We do that in libxdp to
protect against fd 0:

Thanks, will switch the dup to fcntl in the next version.

https://github.com/xdp-project/xdp-tools/blob/master/lib/libxdp/libxdp.c#L1184

Given Alexei's comments above, maybe we should be '3' for the last arg
instead of 1...

-Toke

--
Kartikeya

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

This helper allows us to get the address of a kernel symbol from inside
a BPF_PROG_TYPE_SYSCALL prog (used by gen_loader), so that we can
relocate weak and typeless ksym vars.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

LGTM.

Acked-by: Song Liu <redacted>

On Tue, Oct 5, 2021 at 5:29 PM Kumar Kartikeya Dwivedi [off-list ref] wrote:

Also, avoid using CO-RE features, as lskel doesn't support CO-RE, yet.
Create a file for testing libbpf skeleton as well, so that both
gen_loader and libbpf get tested.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

[...]

This is (almost?) the same as test_weak_syms(), right? Why do we need both?

+{
+       struct test_ksyms_weak *skel;
+       struct test_ksyms_weak__data *data;
+       int err;
+
+       skel = test_ksyms_weak__open_and_load();
+       if (!ASSERT_OK_PTR(skel, "test_ksyms_weak__open_and_load"))
+               return;

[...]