* [PATCH bpf-next v4 1/8] bpf: Add bpf_kallsyms_lookup_name helper
2021-10-20 19:15 [PATCH bpf-next v4 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
@ 2021-10-20 19:15 ` Kumar Kartikeya Dwivedi
2021-10-20 19:15 ` [PATCH bpf-next v4 2/8] libbpf: Add typeless ksym support to gen_loader Kumar Kartikeya Dwivedi
` (6 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-20 19:15 UTC (permalink / raw)
To: bpf
Cc: Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Yonghong Song, Jesper Dangaard Brouer,
Toke Høiland-Jørgensen, netdev
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 typeless ksym vars.
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
include/linux/bpf.h | 1 +
include/uapi/linux/bpf.h | 16 ++++++++++++++++
kernel/bpf/syscall.c | 27 +++++++++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 16 ++++++++++++++++
4 files changed, 60 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..03da5511e3bf 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4909,6 +4909,21 @@ 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.
+ *
+ * **-EPERM** if caller does not have permission to obtain kernel address.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -5089,6 +5104,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..95214e536db6 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4753,6 +4753,31 @@ 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;
+
+ if (!bpf_dump_raw_ok(current_cred()))
+ return -EPERM;
+
+ *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 = false,
+ .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 +4788,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..03da5511e3bf 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4909,6 +4909,21 @@ 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.
+ *
+ * **-EPERM** if caller does not have permission to obtain kernel address.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -5089,6 +5104,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.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH bpf-next v4 2/8] libbpf: Add typeless ksym support to gen_loader
2021-10-20 19:15 [PATCH bpf-next v4 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
2021-10-20 19:15 ` [PATCH bpf-next v4 1/8] bpf: Add bpf_kallsyms_lookup_name helper Kumar Kartikeya Dwivedi
@ 2021-10-20 19:15 ` Kumar Kartikeya Dwivedi
2021-10-20 19:15 ` [PATCH bpf-next v4 3/8] libbpf: Add weak " Kumar Kartikeya Dwivedi
` (5 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-20 19:15 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen, netdev
This uses the bpf_kallsyms_lookup_name helper added in previous patches
to relocate typeless ksyms. The return value ENOENT can be ignored, and
the value written to 'res' can be directly stored to the insn, as it is
overwritten to 0 on lookup failure. For repeating symbols, we can simply
copy the previously populated bpf_insn.
Also, we need to take care to not close fds for typeless ksym_desc, so
reuse the 'off' member's space to add a marker for typeless ksym and use
that to skip them in cleanup_relos.
We add a emit_ksym_relo_log helper that avoids duplicating common
logging instructions between typeless and weak ksym (for future commit).
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/lib/bpf/bpf_gen_internal.h | 12 +++-
tools/lib/bpf/gen_loader.c | 97 ++++++++++++++++++++++++++++----
tools/lib/bpf/libbpf.c | 13 ++---
3 files changed, 99 insertions(+), 23 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..11172a868180 100644
--- a/tools/lib/bpf/gen_loader.c
+++ b/tools/lib/bpf/gen_loader.c
@@ -559,7 +559,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 +572,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 +622,29 @@ 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);
+ res_off = add_data(gen, NULL, 8); /* res is u64 */
+ 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
*
@@ -700,6 +724,58 @@ static void emit_relo_kfunc_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo
relo->name, kdesc->ref);
}
+static void emit_ksym_relo_log(struct bpf_gen *gen, struct ksym_relo_desc *relo,
+ int ref)
+{
+ 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=%d w=%d (%s:count=%d): imm[0]: %%d, imm[1]: %%d",
+ relo->is_typeless, relo->is_weak, relo->name, 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=%d w=%d (%s:count=%d): insn.reg",
+ relo->is_typeless, relo->is_weak, relo->name, ref);
+}
+
+/* Expects:
+ * BPF_REG_8 - pointer to instruction
+ */
+static void emit_relo_ksym_typeless(struct bpf_gen *gen,
+ struct ksym_relo_desc *relo, int insn)
+{
+ struct ksym_desc *kdesc;
+
+ 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));
+ 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:
+ emit_ksym_relo_log(gen, relo, kdesc->ref);
+}
+
/* Expects:
* BPF_REG_8 - pointer to instruction
*/
@@ -729,14 +805,7 @@ static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo,
emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_7,
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 (%s:count=%d): imm: %%d, fd: %%d",
- relo->name, kdesc->ref);
+ emit_ksym_relo_log(gen, relo, kdesc->ref);
}
static void emit_relo(struct bpf_gen *gen, struct ksym_relo_desc *relo, int insns)
@@ -748,7 +817,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 +845,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 760c7e346603..02cd7a6738da 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, false, BTF_KIND_FUNC,
+ relo->insn_idx);
break;
default:
continue;
--
2.33.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH bpf-next v4 3/8] libbpf: Add weak ksym support to gen_loader
2021-10-20 19:15 [PATCH bpf-next v4 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
2021-10-20 19:15 ` [PATCH bpf-next v4 1/8] bpf: Add bpf_kallsyms_lookup_name helper Kumar Kartikeya Dwivedi
2021-10-20 19:15 ` [PATCH bpf-next v4 2/8] libbpf: Add typeless ksym support to gen_loader Kumar Kartikeya Dwivedi
@ 2021-10-20 19:15 ` Kumar Kartikeya Dwivedi
2021-10-20 19:15 ` [PATCH bpf-next v4 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2 Kumar Kartikeya Dwivedi
` (4 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-20 19:15 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen, netdev
This extends existing ksym relocation code to also support relocating
weak ksyms. Care needs to be taken to zero out the src_reg (currently
BPF_PSEUOD_BTF_ID, always set for gen_loader by bpf_object__relocate_data)
when the BTF ID lookup fails at runtime. This is not a problem for
libbpf as it only sets ext->is_set when BTF ID lookup succeeds (and only
proceeds in case of failure if ext->is_weak, leading to src_reg
remaining as 0 for weak unresolved ksym).
A pattern similar to emit_relo_kfunc_btf is followed of first storing
the default values and then jumping over actual stores in case of an
error. For src_reg adjustment, we also need to perform it when copying
the populated instruction, so depending on if copied insn[0].imm is 0 or
not, we decide to jump over the adjustment.
We cannot reach that point unless the ksym was weak and resolved and
zeroed out, as the emit_check_err will cause us to jump to cleanup
label, so we do not need to recheck whether the ksym is weak before
doing the adjustment after copying BTF ID and BTF FD.
This is consistent with how libbpf relocates weak ksym. Logging
statements are added to show the relocation result and aid debugging.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/lib/bpf/gen_loader.c | 35 ++++++++++++++++++++++++++++++++---
1 file changed, 32 insertions(+), 3 deletions(-)
diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
index 11172a868180..1c404752e565 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
@@ -776,12 +777,24 @@ static void emit_relo_ksym_typeless(struct bpf_gen *gen,
emit_ksym_relo_log(gen, relo, kdesc->ref);
}
+static __u32 src_reg_mask(void)
+{
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+ return 0x0f; /* src_reg,dst_reg,... */
+#elif defined(__BIG_ENDIAN_BITFIELD)
+ return 0xf0; /* dst_reg,src_reg,... */
+#else
+#error "Unsupported bit endianness, cannot proceed"
+#endif
+}
+
/* 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)
@@ -792,19 +805,35 @@ static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo,
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));
- goto log;
+ 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 */
+ reg_mask = src_reg_mask();
+ 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)));
+
emit_ksym_relo_log(gen, relo, kdesc->ref);
}
--
2.33.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH bpf-next v4 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2
2021-10-20 19:15 [PATCH bpf-next v4 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
` (2 preceding siblings ...)
2021-10-20 19:15 ` [PATCH bpf-next v4 3/8] libbpf: Add weak " Kumar Kartikeya Dwivedi
@ 2021-10-20 19:15 ` Kumar Kartikeya Dwivedi
2021-10-20 22:09 ` Andrii Nakryiko
2021-10-20 19:15 ` [PATCH bpf-next v4 5/8] libbpf: Use O_CLOEXEC uniformly when opening fds Kumar Kartikeya Dwivedi
` (3 subsequent siblings)
7 siblings, 1 reply; 16+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-20 19:15 UTC (permalink / raw)
To: bpf
Cc: Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Yonghong Song, Jesper Dangaard Brouer,
Toke Høiland-Jørgensen, netdev
Add a simple wrapper for passing an fd and getting a new one >= 3 if it
is one of 0, 1, or 2. There are two primary reasons to make this change:
First, libbpf relies on the assumption a certain BPF fd is never 0 (e.g.
most recently noticed in [0]). Second, Alexei pointed out in [1] that
some environments reset stdin, stdout, and stderr if they notice an
invalid fd at these numbers. To protect against both these cases, switch
all internal BPF syscall wrappers in libbpf to always return an fd >= 3.
We only need to modify the syscall wrappers and not other code that
assumes a valid fd by doing >= 0, to avoid pointless churn, and because
it is still a valid assumption. The cost paid is two additional syscalls
if fd is in range [0, 2].
This is only done for fds that are held by objects created using libbpf,
or returned from libbpf functions, not for intermediate fds closed soon
after their use is over. This implies that one of the assumptions is
that the environment resetting fds in the starting range [0, 2] only do
so before libbpf function call or after, but never in parallel, since
that would close fds while we dup them.
[0]: e31eec77e4ab ("bpf: selftests: Fix fd cleanup in get_branch_snapshot")
[1]: https://lore.kernel.org/bpf/CAADnVQKVKY8o_3aU8Gzke443+uHa-eGoM0h7W4srChMXU1S4Bg@mail.gmail.com
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/lib/bpf/bpf.c | 28 ++++++++++++-------------
tools/lib/bpf/libbpf.c | 36 ++++++++++++++++-----------------
tools/lib/bpf/libbpf_internal.h | 23 +++++++++++++++++++++
tools/lib/bpf/linker.c | 2 +-
tools/lib/bpf/ringbuf.c | 2 +-
tools/lib/bpf/skel_internal.h | 35 +++++++++++++++++++++++++++++++-
6 files changed, 91 insertions(+), 35 deletions(-)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 7d1741ceaa32..0e1dedd94ebf 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -74,7 +74,7 @@ static inline int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size)
fd = sys_bpf(BPF_PROG_LOAD, attr, size);
} while (fd < 0 && errno == EAGAIN && retries-- > 0);
- return fd;
+ return ensure_good_fd(fd);
}
int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr)
@@ -104,7 +104,7 @@ int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr)
attr.inner_map_fd = create_attr->inner_map_fd;
fd = sys_bpf(BPF_MAP_CREATE, &attr, sizeof(attr));
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_create_map_node(enum bpf_map_type map_type, const char *name,
@@ -182,7 +182,7 @@ int bpf_create_map_in_map_node(enum bpf_map_type map_type, const char *name,
}
fd = sys_bpf(BPF_MAP_CREATE, &attr, sizeof(attr));
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_create_map_in_map(enum bpf_map_type map_type, const char *name,
@@ -330,7 +330,7 @@ int libbpf__bpf_prog_load(const struct bpf_prog_load_params *load_attr)
/* free() doesn't affect errno, so we don't need to restore it */
free(finfo);
free(linfo);
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
@@ -610,7 +610,7 @@ int bpf_obj_get(const char *pathname)
attr.pathname = ptr_to_u64((void *)pathname);
fd = sys_bpf(BPF_OBJ_GET, &attr, sizeof(attr));
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
@@ -721,7 +721,7 @@ int bpf_link_create(int prog_fd, int target_fd,
}
proceed:
fd = sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_link_detach(int link_fd)
@@ -764,7 +764,7 @@ int bpf_iter_create(int link_fd)
attr.iter_create.link_fd = link_fd;
fd = sys_bpf(BPF_ITER_CREATE, &attr, sizeof(attr));
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_prog_query(int target_fd, enum bpf_attach_type type, __u32 query_flags,
@@ -922,7 +922,7 @@ int bpf_prog_get_fd_by_id(__u32 id)
attr.prog_id = id;
fd = sys_bpf(BPF_PROG_GET_FD_BY_ID, &attr, sizeof(attr));
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_map_get_fd_by_id(__u32 id)
@@ -934,7 +934,7 @@ int bpf_map_get_fd_by_id(__u32 id)
attr.map_id = id;
fd = sys_bpf(BPF_MAP_GET_FD_BY_ID, &attr, sizeof(attr));
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_btf_get_fd_by_id(__u32 id)
@@ -946,7 +946,7 @@ int bpf_btf_get_fd_by_id(__u32 id)
attr.btf_id = id;
fd = sys_bpf(BPF_BTF_GET_FD_BY_ID, &attr, sizeof(attr));
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_link_get_fd_by_id(__u32 id)
@@ -958,7 +958,7 @@ int bpf_link_get_fd_by_id(__u32 id)
attr.link_id = id;
fd = sys_bpf(BPF_LINK_GET_FD_BY_ID, &attr, sizeof(attr));
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len)
@@ -989,7 +989,7 @@ int bpf_raw_tracepoint_open(const char *name, int prog_fd)
attr.raw_tracepoint.prog_fd = prog_fd;
fd = sys_bpf(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_load_btf(const void *btf, __u32 btf_size, char *log_buf, __u32 log_buf_size,
@@ -1015,7 +1015,7 @@ int bpf_load_btf(const void *btf, __u32 btf_size, char *log_buf, __u32 log_buf_s
goto retry;
}
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf, __u32 *buf_len,
@@ -1051,7 +1051,7 @@ int bpf_enable_stats(enum bpf_stats_type type)
attr.enable_stats.type = type;
fd = sys_bpf(BPF_ENABLE_STATS, &attr, sizeof(attr));
- return libbpf_err_errno(fd);
+ return libbpf_err_errno(ensure_good_fd(fd));
}
int bpf_prog_bind_map(int prog_fd, int map_fd,
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 02cd7a6738da..d4d2842e31ea 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1223,7 +1223,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
obj->efile.elf = elf_memory((char *)obj->efile.obj_buf,
obj->efile.obj_buf_sz);
} else {
- obj->efile.fd = open(obj->path, O_RDONLY);
+ obj->efile.fd = ensure_good_fd(open(obj->path, O_RDONLY));
if (obj->efile.fd < 0) {
char errmsg[STRERR_BUFSIZE], *cp;
@@ -9310,10 +9310,10 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
attr.config2 = offset; /* kprobe_addr or probe_offset */
/* pid filter is meaningful only for uprobes */
- pfd = syscall(__NR_perf_event_open, &attr,
- pid < 0 ? -1 : pid /* pid */,
- pid == -1 ? 0 : -1 /* cpu */,
- -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
+ pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr,
+ pid < 0 ? -1 : pid /* pid */,
+ pid == -1 ? 0 : -1 /* cpu */,
+ -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
if (pfd < 0) {
err = -errno;
pr_warn("%s perf_event_open() failed: %s\n",
@@ -9404,10 +9404,10 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe,
attr.config = type;
attr.type = PERF_TYPE_TRACEPOINT;
- pfd = syscall(__NR_perf_event_open, &attr,
- pid < 0 ? -1 : pid, /* pid */
- pid == -1 ? 0 : -1, /* cpu */
- -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
+ pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr,
+ pid < 0 ? -1 : pid, /* pid */
+ pid == -1 ? 0 : -1, /* cpu */
+ -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
if (pfd < 0) {
err = -errno;
pr_warn("legacy kprobe perf_event_open() failed: %s\n",
@@ -9599,10 +9599,10 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
attr.config = type;
attr.type = PERF_TYPE_TRACEPOINT;
- pfd = syscall(__NR_perf_event_open, &attr,
- pid < 0 ? -1 : pid, /* pid */
- pid == -1 ? 0 : -1, /* cpu */
- -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
+ pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr,
+ pid < 0 ? -1 : pid, /* pid */
+ pid == -1 ? 0 : -1, /* cpu */
+ -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
if (pfd < 0) {
err = -errno;
pr_warn("legacy uprobe perf_event_open() failed: %d\n", err);
@@ -9731,8 +9731,8 @@ static int perf_event_open_tracepoint(const char *tp_category,
attr.size = sizeof(attr);
attr.config = tp_id;
- pfd = syscall(__NR_perf_event_open, &attr, -1 /* pid */, 0 /* cpu */,
- -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
+ pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr, -1 /* pid */, 0 /* cpu */,
+ -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
if (pfd < 0) {
err = -errno;
pr_warn("tracepoint '%s/%s' perf_event_open() failed: %s\n",
@@ -10251,8 +10251,8 @@ perf_buffer__open_cpu_buf(struct perf_buffer *pb, struct perf_event_attr *attr,
cpu_buf->cpu = cpu;
cpu_buf->map_key = map_key;
- cpu_buf->fd = syscall(__NR_perf_event_open, attr, -1 /* pid */, cpu,
- -1, PERF_FLAG_FD_CLOEXEC);
+ cpu_buf->fd = ensure_good_fd(syscall(__NR_perf_event_open, attr, -1 /* pid */, cpu,
+ -1, PERF_FLAG_FD_CLOEXEC));
if (cpu_buf->fd < 0) {
err = -errno;
pr_warn("failed to open perf buffer event on cpu #%d: %s\n",
@@ -10378,7 +10378,7 @@ static struct perf_buffer *__perf_buffer__new(int map_fd, size_t page_cnt,
pb->mmap_size = pb->page_size * page_cnt;
pb->map_fd = map_fd;
- pb->epoll_fd = epoll_create1(EPOLL_CLOEXEC);
+ pb->epoll_fd = ensure_good_fd(epoll_create1(EPOLL_CLOEXEC));
if (pb->epoll_fd < 0) {
err = -errno;
pr_warn("failed to create epoll instance: %s\n",
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index f6a5748dd318..a4e776cafaaa 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -13,6 +13,8 @@
#include <limits.h>
#include <errno.h>
#include <linux/err.h>
+#include <fcntl.h>
+#include <unistd.h>
#include "libbpf_legacy.h"
#include "relo_core.h"
@@ -472,4 +474,25 @@ static inline bool is_ldimm64_insn(struct bpf_insn *insn)
return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
}
+/* if fd is stdin, stdout, or stderr, dup to a fd greater than 2
+ * Takes ownership of the fd passed in, and closes it if calling
+ * fcntl(fd, F_DUPFD_CLOEXEC, 3).
+ */
+static inline int ensure_good_fd(int fd)
+{
+ int old_fd = fd, save_errno;
+
+ if (unlikely(fd >= 0 && fd < 3)) {
+ fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
+ if (fd < 0) {
+ save_errno = errno;
+ pr_warn("failed to dup FD %d to FD > 2: %d\n", old_fd, -errno);
+ }
+ close(old_fd);
+ if (fd < 0)
+ errno = save_errno;
+ }
+ return fd;
+}
+
#endif /* __LIBBPF_LIBBPF_INTERNAL_H */
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index 2df880cefdae..6106a0b5572a 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -302,7 +302,7 @@ static int init_output_elf(struct bpf_linker *linker, const char *file)
if (!linker->filename)
return -ENOMEM;
- linker->fd = open(file, O_WRONLY | O_CREAT | O_TRUNC, 0644);
+ linker->fd = ensure_good_fd(open(file, O_WRONLY | O_CREAT | O_TRUNC, 0644));
if (linker->fd < 0) {
err = -errno;
pr_warn("failed to create '%s': %d\n", file, err);
diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
index 8bc117bcc7bc..40bb33ae548b 100644
--- a/tools/lib/bpf/ringbuf.c
+++ b/tools/lib/bpf/ringbuf.c
@@ -173,7 +173,7 @@ ring_buffer__new(int map_fd, ring_buffer_sample_fn sample_cb, void *ctx,
rb->page_size = getpagesize();
- rb->epoll_fd = epoll_create1(EPOLL_CLOEXEC);
+ rb->epoll_fd = ensure_good_fd(epoll_create1(EPOLL_CLOEXEC));
if (rb->epoll_fd < 0) {
err = -errno;
pr_warn("ringbuf: failed to create epoll instance: %d\n", err);
diff --git a/tools/lib/bpf/skel_internal.h b/tools/lib/bpf/skel_internal.h
index 9cf66702fa8d..1322c4de15e2 100644
--- a/tools/lib/bpf/skel_internal.h
+++ b/tools/lib/bpf/skel_internal.h
@@ -6,6 +6,7 @@
#include <unistd.h>
#include <sys/syscall.h>
#include <sys/mman.h>
+#include <fcntl.h>
/* This file is a base header for auto-generated *.lskel.h files.
* Its contents will change and may become part of auto-generation in the future.
@@ -60,11 +61,39 @@ static inline int skel_closenz(int fd)
return -EINVAL;
}
+static inline int skel_reserve_bad_fds(struct bpf_load_and_run_opts *opts, int *fds)
+{
+ int fd, err, i;
+
+ for (i = 0; i < 3; i++) {
+ fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
+ if (fd < 0) {
+ opts->errstr = "failed to reserve fd 0, 1, and 2";
+ err = -errno;
+ return err;
+ }
+ if (__builtin_expect(fd >= 3, 1)) {
+ close(fd);
+ break;
+ }
+ fds[i] = fd;
+ }
+ return 0;
+}
+
static inline int bpf_load_and_run(struct bpf_load_and_run_opts *opts)
{
- int map_fd = -1, prog_fd = -1, key = 0, err;
+ int map_fd = -1, prog_fd = -1, key = 0, err, i;
+ int res_fds[3] = { -1, -1, -1 };
union bpf_attr attr;
+ /* ensures that we don't open fd 0, 1, or 2 from here on out */
+ err = skel_reserve_bad_fds(opts, res_fds);
+ if (err < 0) {
+ errno = -err;
+ goto out;
+ }
+
map_fd = bpf_create_map_name(BPF_MAP_TYPE_ARRAY, "__loader.map", 4,
opts->data_sz, 1, 0);
if (map_fd < 0) {
@@ -115,6 +144,10 @@ static inline int bpf_load_and_run(struct bpf_load_and_run_opts *opts)
}
err = 0;
out:
+ for (i = 0; i < 3; i++) {
+ if (res_fds[i] >= 0)
+ close(res_fds[i]);
+ }
if (map_fd >= 0)
close(map_fd);
if (prog_fd >= 0)
--
2.33.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next v4 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2
2021-10-20 19:15 ` [PATCH bpf-next v4 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2 Kumar Kartikeya Dwivedi
@ 2021-10-20 22:09 ` Andrii Nakryiko
2021-10-20 22:33 ` Kumar Kartikeya Dwivedi
0 siblings, 1 reply; 16+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 22:09 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Song Liu, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Yonghong Song,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen,
Networking
On Wed, Oct 20, 2021 at 12:15 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Add a simple wrapper for passing an fd and getting a new one >= 3 if it
> is one of 0, 1, or 2. There are two primary reasons to make this change:
> First, libbpf relies on the assumption a certain BPF fd is never 0 (e.g.
> most recently noticed in [0]). Second, Alexei pointed out in [1] that
> some environments reset stdin, stdout, and stderr if they notice an
> invalid fd at these numbers. To protect against both these cases, switch
> all internal BPF syscall wrappers in libbpf to always return an fd >= 3.
> We only need to modify the syscall wrappers and not other code that
> assumes a valid fd by doing >= 0, to avoid pointless churn, and because
> it is still a valid assumption. The cost paid is two additional syscalls
> if fd is in range [0, 2].
>
> This is only done for fds that are held by objects created using libbpf,
> or returned from libbpf functions, not for intermediate fds closed soon
> after their use is over. This implies that one of the assumptions is
> that the environment resetting fds in the starting range [0, 2] only do
> so before libbpf function call or after, but never in parallel, since
> that would close fds while we dup them.
>
> [0]: e31eec77e4ab ("bpf: selftests: Fix fd cleanup in get_branch_snapshot")
> [1]: https://lore.kernel.org/bpf/CAADnVQKVKY8o_3aU8Gzke443+uHa-eGoM0h7W4srChMXU1S4Bg@mail.gmail.com
>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
> tools/lib/bpf/bpf.c | 28 ++++++++++++-------------
> tools/lib/bpf/libbpf.c | 36 ++++++++++++++++-----------------
> tools/lib/bpf/libbpf_internal.h | 23 +++++++++++++++++++++
> tools/lib/bpf/linker.c | 2 +-
> tools/lib/bpf/ringbuf.c | 2 +-
> tools/lib/bpf/skel_internal.h | 35 +++++++++++++++++++++++++++++++-
> 6 files changed, 91 insertions(+), 35 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index 7d1741ceaa32..0e1dedd94ebf 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -74,7 +74,7 @@ static inline int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size)
> fd = sys_bpf(BPF_PROG_LOAD, attr, size);
> } while (fd < 0 && errno == EAGAIN && retries-- > 0);
>
> - return fd;
> + return ensure_good_fd(fd);
> }
>
> int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr)
> @@ -104,7 +104,7 @@ int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr)
> attr.inner_map_fd = create_attr->inner_map_fd;
>
> fd = sys_bpf(BPF_MAP_CREATE, &attr, sizeof(attr));
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
I think it will be cleaner in all the places to do
fd = ensure_good_fd(fd);
That's especially true for more complicated multi-line invocations,
like the ones with perf_event_open syscall.
But also, I feel like adding a sys_bpf_fd() wrapper to use for all bpf
syscalls that can return FDs would be cleaner still and will make it
more obvious which APIs return "objects" (that is, FDs). Then you'll
have ensure_good_fd() business in one or maybe just a few places,
instead of spread around a dozen functions.
> }
>
> int bpf_create_map_node(enum bpf_map_type map_type, const char *name,
> @@ -182,7 +182,7 @@ int bpf_create_map_in_map_node(enum bpf_map_type map_type, const char *name,
> }
>
> fd = sys_bpf(BPF_MAP_CREATE, &attr, sizeof(attr));
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
> }
>
> int bpf_create_map_in_map(enum bpf_map_type map_type, const char *name,
> @@ -330,7 +330,7 @@ int libbpf__bpf_prog_load(const struct bpf_prog_load_params *load_attr)
> /* free() doesn't affect errno, so we don't need to restore it */
> free(finfo);
> free(linfo);
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
this is "higher-level" function, it uses sys_bpf_prog_load() which
already ensured "good" fd, so no need for ensure_good_fd() here
> }
>
> int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> @@ -610,7 +610,7 @@ int bpf_obj_get(const char *pathname)
> attr.pathname = ptr_to_u64((void *)pathname);
>
> fd = sys_bpf(BPF_OBJ_GET, &attr, sizeof(attr));
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
> }
>
> int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
> @@ -721,7 +721,7 @@ int bpf_link_create(int prog_fd, int target_fd,
> }
> proceed:
> fd = sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
> }
>
> int bpf_link_detach(int link_fd)
> @@ -764,7 +764,7 @@ int bpf_iter_create(int link_fd)
> attr.iter_create.link_fd = link_fd;
>
> fd = sys_bpf(BPF_ITER_CREATE, &attr, sizeof(attr));
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
> }
>
> int bpf_prog_query(int target_fd, enum bpf_attach_type type, __u32 query_flags,
> @@ -922,7 +922,7 @@ int bpf_prog_get_fd_by_id(__u32 id)
> attr.prog_id = id;
>
> fd = sys_bpf(BPF_PROG_GET_FD_BY_ID, &attr, sizeof(attr));
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
> }
>
> int bpf_map_get_fd_by_id(__u32 id)
> @@ -934,7 +934,7 @@ int bpf_map_get_fd_by_id(__u32 id)
> attr.map_id = id;
>
> fd = sys_bpf(BPF_MAP_GET_FD_BY_ID, &attr, sizeof(attr));
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
> }
>
> int bpf_btf_get_fd_by_id(__u32 id)
> @@ -946,7 +946,7 @@ int bpf_btf_get_fd_by_id(__u32 id)
> attr.btf_id = id;
>
> fd = sys_bpf(BPF_BTF_GET_FD_BY_ID, &attr, sizeof(attr));
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
> }
>
> int bpf_link_get_fd_by_id(__u32 id)
> @@ -958,7 +958,7 @@ int bpf_link_get_fd_by_id(__u32 id)
> attr.link_id = id;
>
> fd = sys_bpf(BPF_LINK_GET_FD_BY_ID, &attr, sizeof(attr));
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
> }
>
> int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len)
> @@ -989,7 +989,7 @@ int bpf_raw_tracepoint_open(const char *name, int prog_fd)
> attr.raw_tracepoint.prog_fd = prog_fd;
>
> fd = sys_bpf(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
> }
>
> int bpf_load_btf(const void *btf, __u32 btf_size, char *log_buf, __u32 log_buf_size,
> @@ -1015,7 +1015,7 @@ int bpf_load_btf(const void *btf, __u32 btf_size, char *log_buf, __u32 log_buf_s
> goto retry;
> }
>
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
> }
>
> int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf, __u32 *buf_len,
> @@ -1051,7 +1051,7 @@ int bpf_enable_stats(enum bpf_stats_type type)
> attr.enable_stats.type = type;
>
> fd = sys_bpf(BPF_ENABLE_STATS, &attr, sizeof(attr));
> - return libbpf_err_errno(fd);
> + return libbpf_err_errno(ensure_good_fd(fd));
this FD is never passed as prog_fd/bpf_fd/etc, so no need here
> }
>
> int bpf_prog_bind_map(int prog_fd, int map_fd,
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 02cd7a6738da..d4d2842e31ea 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1223,7 +1223,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
> obj->efile.elf = elf_memory((char *)obj->efile.obj_buf,
> obj->efile.obj_buf_sz);
> } else {
> - obj->efile.fd = open(obj->path, O_RDONLY);
> + obj->efile.fd = ensure_good_fd(open(obj->path, O_RDONLY));
no need here, it's just internal ELF file FD
> if (obj->efile.fd < 0) {
> char errmsg[STRERR_BUFSIZE], *cp;
>
> @@ -9310,10 +9310,10 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> attr.config2 = offset; /* kprobe_addr or probe_offset */
>
> /* pid filter is meaningful only for uprobes */
> - pfd = syscall(__NR_perf_event_open, &attr,
> - pid < 0 ? -1 : pid /* pid */,
> - pid == -1 ? 0 : -1 /* cpu */,
> - -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> + pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr,
> + pid < 0 ? -1 : pid /* pid */,
> + pid == -1 ? 0 : -1 /* cpu */,
> + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
see above, pfd = ensure_good_fd(pfd) on a separate line after
syscall() is much cleaner
but thinking about this some more... do we ever pass perf_event_fd
into BPF UAPI? I don't think so, so there is no need to enforce FD >
0. Let's drop that everywhere for perf_event_open().
> if (pfd < 0) {
> err = -errno;
> pr_warn("%s perf_event_open() failed: %s\n",
> @@ -9404,10 +9404,10 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe,
> attr.config = type;
> attr.type = PERF_TYPE_TRACEPOINT;
>
> - pfd = syscall(__NR_perf_event_open, &attr,
> - pid < 0 ? -1 : pid, /* pid */
> - pid == -1 ? 0 : -1, /* cpu */
> - -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> + pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr,
> + pid < 0 ? -1 : pid, /* pid */
> + pid == -1 ? 0 : -1, /* cpu */
> + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
> if (pfd < 0) {
> err = -errno;
> pr_warn("legacy kprobe perf_event_open() failed: %s\n",
> @@ -9599,10 +9599,10 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
> attr.config = type;
> attr.type = PERF_TYPE_TRACEPOINT;
>
> - pfd = syscall(__NR_perf_event_open, &attr,
> - pid < 0 ? -1 : pid, /* pid */
> - pid == -1 ? 0 : -1, /* cpu */
> - -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> + pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr,
> + pid < 0 ? -1 : pid, /* pid */
> + pid == -1 ? 0 : -1, /* cpu */
> + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
> if (pfd < 0) {
> err = -errno;
> pr_warn("legacy uprobe perf_event_open() failed: %d\n", err);
> @@ -9731,8 +9731,8 @@ static int perf_event_open_tracepoint(const char *tp_category,
> attr.size = sizeof(attr);
> attr.config = tp_id;
>
> - pfd = syscall(__NR_perf_event_open, &attr, -1 /* pid */, 0 /* cpu */,
> - -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> + pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr, -1 /* pid */, 0 /* cpu */,
> + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
> if (pfd < 0) {
> err = -errno;
> pr_warn("tracepoint '%s/%s' perf_event_open() failed: %s\n",
> @@ -10251,8 +10251,8 @@ perf_buffer__open_cpu_buf(struct perf_buffer *pb, struct perf_event_attr *attr,
> cpu_buf->cpu = cpu;
> cpu_buf->map_key = map_key;
>
> - cpu_buf->fd = syscall(__NR_perf_event_open, attr, -1 /* pid */, cpu,
> - -1, PERF_FLAG_FD_CLOEXEC);
> + cpu_buf->fd = ensure_good_fd(syscall(__NR_perf_event_open, attr, -1 /* pid */, cpu,
> + -1, PERF_FLAG_FD_CLOEXEC));
> if (cpu_buf->fd < 0) {
> err = -errno;
> pr_warn("failed to open perf buffer event on cpu #%d: %s\n",
> @@ -10378,7 +10378,7 @@ static struct perf_buffer *__perf_buffer__new(int map_fd, size_t page_cnt,
> pb->mmap_size = pb->page_size * page_cnt;
> pb->map_fd = map_fd;
>
> - pb->epoll_fd = epoll_create1(EPOLL_CLOEXEC);
> + pb->epoll_fd = ensure_good_fd(epoll_create1(EPOLL_CLOEXEC));
same, no need
> if (pb->epoll_fd < 0) {
> err = -errno;
> pr_warn("failed to create epoll instance: %s\n",
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index f6a5748dd318..a4e776cafaaa 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -13,6 +13,8 @@
> #include <limits.h>
> #include <errno.h>
> #include <linux/err.h>
> +#include <fcntl.h>
> +#include <unistd.h>
> #include "libbpf_legacy.h"
> #include "relo_core.h"
>
> @@ -472,4 +474,25 @@ static inline bool is_ldimm64_insn(struct bpf_insn *insn)
> return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
> }
>
> +/* if fd is stdin, stdout, or stderr, dup to a fd greater than 2
> + * Takes ownership of the fd passed in, and closes it if calling
> + * fcntl(fd, F_DUPFD_CLOEXEC, 3).
> + */
> +static inline int ensure_good_fd(int fd)
> +{
> + int old_fd = fd, save_errno;
> +
> + if (unlikely(fd >= 0 && fd < 3)) {
> + fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
> + if (fd < 0) {
> + save_errno = errno;
> + pr_warn("failed to dup FD %d to FD > 2: %d\n", old_fd, -errno);
> + }
> + close(old_fd);
> + if (fd < 0)
> + errno = save_errno;
> + }
> + return fd;
with all the if nestedness it looks more complicated than it needs to.
How about something like this:
if (fd < 0)
return fd;
if (fd < 3) {
fd = fcnt(...);
save_errno = errno;
close(old_fd);
if (fd < 0) {
pr_warn(...);
errno = saved_errno;
}
}
return fd;
WDYT?
> +}
> +
> #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index 2df880cefdae..6106a0b5572a 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -302,7 +302,7 @@ static int init_output_elf(struct bpf_linker *linker, const char *file)
> if (!linker->filename)
> return -ENOMEM;
>
> - linker->fd = open(file, O_WRONLY | O_CREAT | O_TRUNC, 0644);
> + linker->fd = ensure_good_fd(open(file, O_WRONLY | O_CREAT | O_TRUNC, 0644));
again, nothing to do with BPF UAPI
> if (linker->fd < 0) {
> err = -errno;
> pr_warn("failed to create '%s': %d\n", file, err);
> diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> index 8bc117bcc7bc..40bb33ae548b 100644
> --- a/tools/lib/bpf/ringbuf.c
> +++ b/tools/lib/bpf/ringbuf.c
> @@ -173,7 +173,7 @@ ring_buffer__new(int map_fd, ring_buffer_sample_fn sample_cb, void *ctx,
>
> rb->page_size = getpagesize();
>
> - rb->epoll_fd = epoll_create1(EPOLL_CLOEXEC);
> + rb->epoll_fd = ensure_good_fd(epoll_create1(EPOLL_CLOEXEC));
here as well
> if (rb->epoll_fd < 0) {
> err = -errno;
> pr_warn("ringbuf: failed to create epoll instance: %d\n", err);
> diff --git a/tools/lib/bpf/skel_internal.h b/tools/lib/bpf/skel_internal.h
> index 9cf66702fa8d..1322c4de15e2 100644
> --- a/tools/lib/bpf/skel_internal.h
> +++ b/tools/lib/bpf/skel_internal.h
> @@ -6,6 +6,7 @@
> #include <unistd.h>
> #include <sys/syscall.h>
> #include <sys/mman.h>
> +#include <fcntl.h>
>
> /* This file is a base header for auto-generated *.lskel.h files.
> * Its contents will change and may become part of auto-generation in the future.
> @@ -60,11 +61,39 @@ static inline int skel_closenz(int fd)
> return -EINVAL;
> }
>
> +static inline int skel_reserve_bad_fds(struct bpf_load_and_run_opts *opts, int *fds)
> +{
> + int fd, err, i;
> +
> + for (i = 0; i < 3; i++) {
> + fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
> + if (fd < 0) {
> + opts->errstr = "failed to reserve fd 0, 1, and 2";
> + err = -errno;
> + return err;
> + }
> + if (__builtin_expect(fd >= 3, 1)) {
please drop the whole likely/unlikely business, we are not dealing
with millions of FDs being open and closed, this micro optimization is
completely unnecessary
But also, why do still need to "reserve" bad FDs at all with all the
ensure_good_fd() business?
> + close(fd);
> + break;
> + }
> + fds[i] = fd;
> + }
> + return 0;
> +}
> +
> static inline int bpf_load_and_run(struct bpf_load_and_run_opts *opts)
> {
> - int map_fd = -1, prog_fd = -1, key = 0, err;
> + int map_fd = -1, prog_fd = -1, key = 0, err, i;
> + int res_fds[3] = { -1, -1, -1 };
> union bpf_attr attr;
>
> + /* ensures that we don't open fd 0, 1, or 2 from here on out */
> + err = skel_reserve_bad_fds(opts, res_fds);
> + if (err < 0) {
> + errno = -err;
> + goto out;
> + }
> +
> map_fd = bpf_create_map_name(BPF_MAP_TYPE_ARRAY, "__loader.map", 4,
> opts->data_sz, 1, 0);
> if (map_fd < 0) {
> @@ -115,6 +144,10 @@ static inline int bpf_load_and_run(struct bpf_load_and_run_opts *opts)
> }
> err = 0;
> out:
> + for (i = 0; i < 3; i++) {
> + if (res_fds[i] >= 0)
> + close(res_fds[i]);
> + }
> if (map_fd >= 0)
> close(map_fd);
> if (prog_fd >= 0)
> --
> 2.33.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next v4 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2
2021-10-20 22:09 ` Andrii Nakryiko
@ 2021-10-20 22:33 ` Kumar Kartikeya Dwivedi
2021-10-20 23:02 ` Andrii Nakryiko
0 siblings, 1 reply; 16+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-20 22:33 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Song Liu, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Yonghong Song,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen,
Networking
On Thu, Oct 21, 2021 at 03:39:08AM IST, Andrii Nakryiko wrote:
> On Wed, Oct 20, 2021 at 12:15 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > Add a simple wrapper for passing an fd and getting a new one >= 3 if it
> > is one of 0, 1, or 2. There are two primary reasons to make this change:
> > First, libbpf relies on the assumption a certain BPF fd is never 0 (e.g.
> > most recently noticed in [0]). Second, Alexei pointed out in [1] that
> > some environments reset stdin, stdout, and stderr if they notice an
> > invalid fd at these numbers. To protect against both these cases, switch
> > all internal BPF syscall wrappers in libbpf to always return an fd >= 3.
> > We only need to modify the syscall wrappers and not other code that
> > assumes a valid fd by doing >= 0, to avoid pointless churn, and because
> > it is still a valid assumption. The cost paid is two additional syscalls
> > if fd is in range [0, 2].
> >
> > This is only done for fds that are held by objects created using libbpf,
> > or returned from libbpf functions, not for intermediate fds closed soon
> > after their use is over. This implies that one of the assumptions is
> > that the environment resetting fds in the starting range [0, 2] only do
> > so before libbpf function call or after, but never in parallel, since
> > that would close fds while we dup them.
> >
> > [0]: e31eec77e4ab ("bpf: selftests: Fix fd cleanup in get_branch_snapshot")
> > [1]: https://lore.kernel.org/bpf/CAADnVQKVKY8o_3aU8Gzke443+uHa-eGoM0h7W4srChMXU1S4Bg@mail.gmail.com
> >
> > Acked-by: Song Liu <songliubraving@fb.com>
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> > tools/lib/bpf/bpf.c | 28 ++++++++++++-------------
> > tools/lib/bpf/libbpf.c | 36 ++++++++++++++++-----------------
> > tools/lib/bpf/libbpf_internal.h | 23 +++++++++++++++++++++
> > tools/lib/bpf/linker.c | 2 +-
> > tools/lib/bpf/ringbuf.c | 2 +-
> > tools/lib/bpf/skel_internal.h | 35 +++++++++++++++++++++++++++++++-
> > 6 files changed, 91 insertions(+), 35 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index 7d1741ceaa32..0e1dedd94ebf 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -74,7 +74,7 @@ static inline int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size)
> > fd = sys_bpf(BPF_PROG_LOAD, attr, size);
> > } while (fd < 0 && errno == EAGAIN && retries-- > 0);
> >
> > - return fd;
> > + return ensure_good_fd(fd);
> > }
> >
> > int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr)
> > @@ -104,7 +104,7 @@ int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr)
> > attr.inner_map_fd = create_attr->inner_map_fd;
> >
> > fd = sys_bpf(BPF_MAP_CREATE, &attr, sizeof(attr));
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
>
> I think it will be cleaner in all the places to do
>
> fd = ensure_good_fd(fd);
>
> That's especially true for more complicated multi-line invocations,
> like the ones with perf_event_open syscall.
>
Ok, will fix.
> But also, I feel like adding a sys_bpf_fd() wrapper to use for all bpf
> syscalls that can return FDs would be cleaner still and will make it
> more obvious which APIs return "objects" (that is, FDs). Then you'll
> have ensure_good_fd() business in one or maybe just a few places,
> instead of spread around a dozen functions.
>
Can you give a short example?
> > }
> >
> > int bpf_create_map_node(enum bpf_map_type map_type, const char *name,
> > @@ -182,7 +182,7 @@ int bpf_create_map_in_map_node(enum bpf_map_type map_type, const char *name,
> > }
> >
> > fd = sys_bpf(BPF_MAP_CREATE, &attr, sizeof(attr));
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
> > }
> >
> > int bpf_create_map_in_map(enum bpf_map_type map_type, const char *name,
> > @@ -330,7 +330,7 @@ int libbpf__bpf_prog_load(const struct bpf_prog_load_params *load_attr)
> > /* free() doesn't affect errno, so we don't need to restore it */
> > free(finfo);
> > free(linfo);
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
>
> this is "higher-level" function, it uses sys_bpf_prog_load() which
> already ensured "good" fd, so no need for ensure_good_fd() here
>
Correct, no need here.
> > }
> >
> > int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> > @@ -610,7 +610,7 @@ int bpf_obj_get(const char *pathname)
> > attr.pathname = ptr_to_u64((void *)pathname);
> >
> > fd = sys_bpf(BPF_OBJ_GET, &attr, sizeof(attr));
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
> > }
> >
> > int bpf_prog_attach(int prog_fd, int target_fd, enum bpf_attach_type type,
> > @@ -721,7 +721,7 @@ int bpf_link_create(int prog_fd, int target_fd,
> > }
> > proceed:
> > fd = sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
> > }
> >
> > int bpf_link_detach(int link_fd)
> > @@ -764,7 +764,7 @@ int bpf_iter_create(int link_fd)
> > attr.iter_create.link_fd = link_fd;
> >
> > fd = sys_bpf(BPF_ITER_CREATE, &attr, sizeof(attr));
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
> > }
> >
> > int bpf_prog_query(int target_fd, enum bpf_attach_type type, __u32 query_flags,
> > @@ -922,7 +922,7 @@ int bpf_prog_get_fd_by_id(__u32 id)
> > attr.prog_id = id;
> >
> > fd = sys_bpf(BPF_PROG_GET_FD_BY_ID, &attr, sizeof(attr));
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
> > }
> >
> > int bpf_map_get_fd_by_id(__u32 id)
> > @@ -934,7 +934,7 @@ int bpf_map_get_fd_by_id(__u32 id)
> > attr.map_id = id;
> >
> > fd = sys_bpf(BPF_MAP_GET_FD_BY_ID, &attr, sizeof(attr));
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
> > }
> >
> > int bpf_btf_get_fd_by_id(__u32 id)
> > @@ -946,7 +946,7 @@ int bpf_btf_get_fd_by_id(__u32 id)
> > attr.btf_id = id;
> >
> > fd = sys_bpf(BPF_BTF_GET_FD_BY_ID, &attr, sizeof(attr));
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
> > }
> >
> > int bpf_link_get_fd_by_id(__u32 id)
> > @@ -958,7 +958,7 @@ int bpf_link_get_fd_by_id(__u32 id)
> > attr.link_id = id;
> >
> > fd = sys_bpf(BPF_LINK_GET_FD_BY_ID, &attr, sizeof(attr));
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
> > }
> >
> > int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len)
> > @@ -989,7 +989,7 @@ int bpf_raw_tracepoint_open(const char *name, int prog_fd)
> > attr.raw_tracepoint.prog_fd = prog_fd;
> >
> > fd = sys_bpf(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
> > }
> >
> > int bpf_load_btf(const void *btf, __u32 btf_size, char *log_buf, __u32 log_buf_size,
> > @@ -1015,7 +1015,7 @@ int bpf_load_btf(const void *btf, __u32 btf_size, char *log_buf, __u32 log_buf_s
> > goto retry;
> > }
> >
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
> > }
> >
> > int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf, __u32 *buf_len,
> > @@ -1051,7 +1051,7 @@ int bpf_enable_stats(enum bpf_stats_type type)
> > attr.enable_stats.type = type;
> >
> > fd = sys_bpf(BPF_ENABLE_STATS, &attr, sizeof(attr));
> > - return libbpf_err_errno(fd);
> > + return libbpf_err_errno(ensure_good_fd(fd));
>
> this FD is never passed as prog_fd/bpf_fd/etc, so no need here
>
See below.
> > }
> >
> > int bpf_prog_bind_map(int prog_fd, int map_fd,
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 02cd7a6738da..d4d2842e31ea 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -1223,7 +1223,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
> > obj->efile.elf = elf_memory((char *)obj->efile.obj_buf,
> > obj->efile.obj_buf_sz);
> > } else {
> > - obj->efile.fd = open(obj->path, O_RDONLY);
> > + obj->efile.fd = ensure_good_fd(open(obj->path, O_RDONLY));
>
> no need here, it's just internal ELF file FD
>
See below.
> > if (obj->efile.fd < 0) {
> > char errmsg[STRERR_BUFSIZE], *cp;
> >
> > @@ -9310,10 +9310,10 @@ static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> > attr.config2 = offset; /* kprobe_addr or probe_offset */
> >
> > /* pid filter is meaningful only for uprobes */
> > - pfd = syscall(__NR_perf_event_open, &attr,
> > - pid < 0 ? -1 : pid /* pid */,
> > - pid == -1 ? 0 : -1 /* cpu */,
> > - -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > + pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr,
> > + pid < 0 ? -1 : pid /* pid */,
> > + pid == -1 ? 0 : -1 /* cpu */,
> > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
>
> see above, pfd = ensure_good_fd(pfd) on a separate line after
> syscall() is much cleaner
>
> but thinking about this some more... do we ever pass perf_event_fd
> into BPF UAPI? I don't think so, so there is no need to enforce FD >
> 0. Let's drop that everywhere for perf_event_open().
>
I thought it was not just about BPF UAPI anymore, but also the first three fds
that can be closed by the runtime. So any fds held across calls, inside objects,
or returned from libbpf syscall functions needed to go through this. For fds
that are temporarily open and then closed later, it doesn't matter (because even
when duplicating the fd, we temporarily open an fd in the invalid slot, so if
someone was closing it in parallel, it would already be a problem).
>
> > if (pfd < 0) {
> > err = -errno;
> > pr_warn("%s perf_event_open() failed: %s\n",
> > @@ -9404,10 +9404,10 @@ static int perf_event_kprobe_open_legacy(const char *probe_name, bool retprobe,
> > attr.config = type;
> > attr.type = PERF_TYPE_TRACEPOINT;
> >
> > - pfd = syscall(__NR_perf_event_open, &attr,
> > - pid < 0 ? -1 : pid, /* pid */
> > - pid == -1 ? 0 : -1, /* cpu */
> > - -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > + pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr,
> > + pid < 0 ? -1 : pid, /* pid */
> > + pid == -1 ? 0 : -1, /* cpu */
> > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
> > if (pfd < 0) {
> > err = -errno;
> > pr_warn("legacy kprobe perf_event_open() failed: %s\n",
> > @@ -9599,10 +9599,10 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
> > attr.config = type;
> > attr.type = PERF_TYPE_TRACEPOINT;
> >
> > - pfd = syscall(__NR_perf_event_open, &attr,
> > - pid < 0 ? -1 : pid, /* pid */
> > - pid == -1 ? 0 : -1, /* cpu */
> > - -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > + pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr,
> > + pid < 0 ? -1 : pid, /* pid */
> > + pid == -1 ? 0 : -1, /* cpu */
> > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
> > if (pfd < 0) {
> > err = -errno;
> > pr_warn("legacy uprobe perf_event_open() failed: %d\n", err);
> > @@ -9731,8 +9731,8 @@ static int perf_event_open_tracepoint(const char *tp_category,
> > attr.size = sizeof(attr);
> > attr.config = tp_id;
> >
> > - pfd = syscall(__NR_perf_event_open, &attr, -1 /* pid */, 0 /* cpu */,
> > - -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > + pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr, -1 /* pid */, 0 /* cpu */,
> > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
> > if (pfd < 0) {
> > err = -errno;
> > pr_warn("tracepoint '%s/%s' perf_event_open() failed: %s\n",
> > @@ -10251,8 +10251,8 @@ perf_buffer__open_cpu_buf(struct perf_buffer *pb, struct perf_event_attr *attr,
> > cpu_buf->cpu = cpu;
> > cpu_buf->map_key = map_key;
> >
> > - cpu_buf->fd = syscall(__NR_perf_event_open, attr, -1 /* pid */, cpu,
> > - -1, PERF_FLAG_FD_CLOEXEC);
> > + cpu_buf->fd = ensure_good_fd(syscall(__NR_perf_event_open, attr, -1 /* pid */, cpu,
> > + -1, PERF_FLAG_FD_CLOEXEC));
> > if (cpu_buf->fd < 0) {
> > err = -errno;
> > pr_warn("failed to open perf buffer event on cpu #%d: %s\n",
> > @@ -10378,7 +10378,7 @@ static struct perf_buffer *__perf_buffer__new(int map_fd, size_t page_cnt,
> > pb->mmap_size = pb->page_size * page_cnt;
> > pb->map_fd = map_fd;
> >
> > - pb->epoll_fd = epoll_create1(EPOLL_CLOEXEC);
> > + pb->epoll_fd = ensure_good_fd(epoll_create1(EPOLL_CLOEXEC));
>
> same, no need
>
See above.
> > if (pb->epoll_fd < 0) {
> > err = -errno;
> > pr_warn("failed to create epoll instance: %s\n",
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index f6a5748dd318..a4e776cafaaa 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -13,6 +13,8 @@
> > #include <limits.h>
> > #include <errno.h>
> > #include <linux/err.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > #include "libbpf_legacy.h"
> > #include "relo_core.h"
> >
> > @@ -472,4 +474,25 @@ static inline bool is_ldimm64_insn(struct bpf_insn *insn)
> > return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
> > }
> >
> > +/* if fd is stdin, stdout, or stderr, dup to a fd greater than 2
> > + * Takes ownership of the fd passed in, and closes it if calling
> > + * fcntl(fd, F_DUPFD_CLOEXEC, 3).
> > + */
> > +static inline int ensure_good_fd(int fd)
> > +{
> > + int old_fd = fd, save_errno;
> > +
> > + if (unlikely(fd >= 0 && fd < 3)) {
> > + fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
> > + if (fd < 0) {
> > + save_errno = errno;
> > + pr_warn("failed to dup FD %d to FD > 2: %d\n", old_fd, -errno);
> > + }
> > + close(old_fd);
> > + if (fd < 0)
> > + errno = save_errno;
> > + }
> > + return fd;
>
> with all the if nestedness it looks more complicated than it needs to.
> How about something like this:
>
> if (fd < 0)
> return fd;
>
> if (fd < 3) {
> fd = fcnt(...);
> save_errno = errno;
> close(old_fd);
> if (fd < 0) {
> pr_warn(...);
> errno = saved_errno;
> }
> }
>
> return fd;
>
>
> WDYT?
>
Looks better, I'll do it like this.
> > +}
> > +
> > #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> > index 2df880cefdae..6106a0b5572a 100644
> > --- a/tools/lib/bpf/linker.c
> > +++ b/tools/lib/bpf/linker.c
> > @@ -302,7 +302,7 @@ static int init_output_elf(struct bpf_linker *linker, const char *file)
> > if (!linker->filename)
> > return -ENOMEM;
> >
> > - linker->fd = open(file, O_WRONLY | O_CREAT | O_TRUNC, 0644);
> > + linker->fd = ensure_good_fd(open(file, O_WRONLY | O_CREAT | O_TRUNC, 0644));
>
> again, nothing to do with BPF UAPI
>
> > if (linker->fd < 0) {
> > err = -errno;
> > pr_warn("failed to create '%s': %d\n", file, err);
> > diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c
> > index 8bc117bcc7bc..40bb33ae548b 100644
> > --- a/tools/lib/bpf/ringbuf.c
> > +++ b/tools/lib/bpf/ringbuf.c
> > @@ -173,7 +173,7 @@ ring_buffer__new(int map_fd, ring_buffer_sample_fn sample_cb, void *ctx,
> >
> > rb->page_size = getpagesize();
> >
> > - rb->epoll_fd = epoll_create1(EPOLL_CLOEXEC);
> > + rb->epoll_fd = ensure_good_fd(epoll_create1(EPOLL_CLOEXEC));
>
> here as well
>
See above.
> > if (rb->epoll_fd < 0) {
> > err = -errno;
> > pr_warn("ringbuf: failed to create epoll instance: %d\n", err);
> > diff --git a/tools/lib/bpf/skel_internal.h b/tools/lib/bpf/skel_internal.h
> > index 9cf66702fa8d..1322c4de15e2 100644
> > --- a/tools/lib/bpf/skel_internal.h
> > +++ b/tools/lib/bpf/skel_internal.h
> > @@ -6,6 +6,7 @@
> > #include <unistd.h>
> > #include <sys/syscall.h>
> > #include <sys/mman.h>
> > +#include <fcntl.h>
> >
> > /* This file is a base header for auto-generated *.lskel.h files.
> > * Its contents will change and may become part of auto-generation in the future.
> > @@ -60,11 +61,39 @@ static inline int skel_closenz(int fd)
> > return -EINVAL;
> > }
> >
> > +static inline int skel_reserve_bad_fds(struct bpf_load_and_run_opts *opts, int *fds)
> > +{
> > + int fd, err, i;
> > +
> > + for (i = 0; i < 3; i++) {
> > + fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
> > + if (fd < 0) {
> > + opts->errstr = "failed to reserve fd 0, 1, and 2";
> > + err = -errno;
> > + return err;
> > + }
> > + if (__builtin_expect(fd >= 3, 1)) {
>
> please drop the whole likely/unlikely business, we are not dealing
> with millions of FDs being open and closed, this micro optimization is
> completely unnecessary
>
Ok
>
> But also, why do still need to "reserve" bad FDs at all with all the
> ensure_good_fd() business?
>
This file is included in light skeleton, so it never goes through libbpf syscall
wrappers, also, not only will it invoke bpf syscall before calling
BPF_PROG_TEST_RUN, but also inside the syscall loader program. In there, doing
the fcntl stuff is much ugly (add a helper, more BPF assembly in gen_loader), so
I just reserve 3 fds in the beginning of the code sequence where fds are
acquired, and release them after we're done.
> > + close(fd);
> > + break;
> > + }
> > + fds[i] = fd;
> > + }
> > + return 0;
> > +}
> > +
> > static inline int bpf_load_and_run(struct bpf_load_and_run_opts *opts)
> > {
> > - int map_fd = -1, prog_fd = -1, key = 0, err;
> > + int map_fd = -1, prog_fd = -1, key = 0, err, i;
> > + int res_fds[3] = { -1, -1, -1 };
> > union bpf_attr attr;
> >
> > + /* ensures that we don't open fd 0, 1, or 2 from here on out */
> > + err = skel_reserve_bad_fds(opts, res_fds);
> > + if (err < 0) {
> > + errno = -err;
> > + goto out;
> > + }
> > +
> > map_fd = bpf_create_map_name(BPF_MAP_TYPE_ARRAY, "__loader.map", 4,
> > opts->data_sz, 1, 0);
> > if (map_fd < 0) {
> > @@ -115,6 +144,10 @@ static inline int bpf_load_and_run(struct bpf_load_and_run_opts *opts)
> > }
> > err = 0;
> > out:
> > + for (i = 0; i < 3; i++) {
> > + if (res_fds[i] >= 0)
> > + close(res_fds[i]);
> > + }
> > if (map_fd >= 0)
> > close(map_fd);
> > if (prog_fd >= 0)
> > --
> > 2.33.1
> >
--
Kartikeya
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next v4 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2
2021-10-20 22:33 ` Kumar Kartikeya Dwivedi
@ 2021-10-20 23:02 ` Andrii Nakryiko
0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 23:02 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Song Liu, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Yonghong Song,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen,
Networking
On Wed, Oct 20, 2021 at 3:33 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Thu, Oct 21, 2021 at 03:39:08AM IST, Andrii Nakryiko wrote:
> > On Wed, Oct 20, 2021 at 12:15 PM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > Add a simple wrapper for passing an fd and getting a new one >= 3 if it
> > > is one of 0, 1, or 2. There are two primary reasons to make this change:
> > > First, libbpf relies on the assumption a certain BPF fd is never 0 (e.g.
> > > most recently noticed in [0]). Second, Alexei pointed out in [1] that
> > > some environments reset stdin, stdout, and stderr if they notice an
> > > invalid fd at these numbers. To protect against both these cases, switch
> > > all internal BPF syscall wrappers in libbpf to always return an fd >= 3.
> > > We only need to modify the syscall wrappers and not other code that
> > > assumes a valid fd by doing >= 0, to avoid pointless churn, and because
> > > it is still a valid assumption. The cost paid is two additional syscalls
> > > if fd is in range [0, 2].
> > >
> > > This is only done for fds that are held by objects created using libbpf,
> > > or returned from libbpf functions, not for intermediate fds closed soon
> > > after their use is over. This implies that one of the assumptions is
> > > that the environment resetting fds in the starting range [0, 2] only do
> > > so before libbpf function call or after, but never in parallel, since
> > > that would close fds while we dup them.
> > >
> > > [0]: e31eec77e4ab ("bpf: selftests: Fix fd cleanup in get_branch_snapshot")
> > > [1]: https://lore.kernel.org/bpf/CAADnVQKVKY8o_3aU8Gzke443+uHa-eGoM0h7W4srChMXU1S4Bg@mail.gmail.com
see my recent reply to Dave, so far such references were always
indented by two spaces (AFAIK)
> > >
> > > Acked-by: Song Liu <songliubraving@fb.com>
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > > tools/lib/bpf/bpf.c | 28 ++++++++++++-------------
> > > tools/lib/bpf/libbpf.c | 36 ++++++++++++++++-----------------
> > > tools/lib/bpf/libbpf_internal.h | 23 +++++++++++++++++++++
> > > tools/lib/bpf/linker.c | 2 +-
> > > tools/lib/bpf/ringbuf.c | 2 +-
> > > tools/lib/bpf/skel_internal.h | 35 +++++++++++++++++++++++++++++++-
> > > 6 files changed, 91 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > > index 7d1741ceaa32..0e1dedd94ebf 100644
> > > --- a/tools/lib/bpf/bpf.c
> > > +++ b/tools/lib/bpf/bpf.c
> > > @@ -74,7 +74,7 @@ static inline int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size)
> > > fd = sys_bpf(BPF_PROG_LOAD, attr, size);
> > > } while (fd < 0 && errno == EAGAIN && retries-- > 0);
> > >
> > > - return fd;
> > > + return ensure_good_fd(fd);
> > > }
> > >
> > > int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr)
> > > @@ -104,7 +104,7 @@ int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr)
> > > attr.inner_map_fd = create_attr->inner_map_fd;
> > >
> > > fd = sys_bpf(BPF_MAP_CREATE, &attr, sizeof(attr));
> > > - return libbpf_err_errno(fd);
> > > + return libbpf_err_errno(ensure_good_fd(fd));
> >
> > I think it will be cleaner in all the places to do
> >
> > fd = ensure_good_fd(fd);
> >
> > That's especially true for more complicated multi-line invocations,
> > like the ones with perf_event_open syscall.
> >
>
> Ok, will fix.
>
> > But also, I feel like adding a sys_bpf_fd() wrapper to use for all bpf
> > syscalls that can return FDs would be cleaner still and will make it
> > more obvious which APIs return "objects" (that is, FDs). Then you'll
> > have ensure_good_fd() business in one or maybe just a few places,
> > instead of spread around a dozen functions.
> >
>
> Can you give a short example?
Sure.
static inline int sys_bpf_fd(enum bpf_cmd cmd, union bpf_attr *attr)
{
int fd;
fd = syscall(__NR_BPF, cmd, attr, sizeof(*attr));
fd = ensure_good_fd(fd);
return fd;
}
>
> > > }
> > >
[...]
> > > /* pid filter is meaningful only for uprobes */
> > > - pfd = syscall(__NR_perf_event_open, &attr,
> > > - pid < 0 ? -1 : pid /* pid */,
> > > - pid == -1 ? 0 : -1 /* cpu */,
> > > - -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> > > + pfd = ensure_good_fd(syscall(__NR_perf_event_open, &attr,
> > > + pid < 0 ? -1 : pid /* pid */,
> > > + pid == -1 ? 0 : -1 /* cpu */,
> > > + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC));
> >
> > see above, pfd = ensure_good_fd(pfd) on a separate line after
> > syscall() is much cleaner
> >
> > but thinking about this some more... do we ever pass perf_event_fd
> > into BPF UAPI? I don't think so, so there is no need to enforce FD >
> > 0. Let's drop that everywhere for perf_event_open().
> >
>
> I thought it was not just about BPF UAPI anymore, but also the first three fds
> that can be closed by the runtime. So any fds held across calls, inside objects,
> or returned from libbpf syscall functions needed to go through this. For fds
> that are temporarily open and then closed later, it doesn't matter (because even
> when duplicating the fd, we temporarily open an fd in the invalid slot, so if
> someone was closing it in parallel, it would already be a problem).
I think you are a bit overzealous about "preventing" this FD > 2
problem here. If it was such an easy problem to run into, not just
libbpf, but any library that opens files would be broken. So clearly
this doesn't happen all the time. I assume that apps where such
stdin/stdout/stderr forcing happens get those FD 0, 1, 2 "fixed" way
before libbpf gets involved. If someone is trying to be clever and
manipulates FD 0, 1, 2 manually, sure, then they will run into
troubles. But libbpf doesn't do anything clever like that. So it's
fine if we enforce FD > 2, not just FD > 0 (it's one fcntl() syscall
anyway), but let's not overdo it. We should be intentional about doing
this only for BPF objects (progs, btfs, maps, links; that's it).
>
> >
> > > if (pfd < 0) {
> > > err = -errno;
> > > pr_warn("%s perf_event_open() failed: %s\n",
[...]
> > >
> > > @@ -472,4 +474,25 @@ static inline bool is_ldimm64_insn(struct bpf_insn *insn)
> > > return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
> > > }
> > >
> > > +/* if fd is stdin, stdout, or stderr, dup to a fd greater than 2
> > > + * Takes ownership of the fd passed in, and closes it if calling
> > > + * fcntl(fd, F_DUPFD_CLOEXEC, 3).
> > > + */
> > > +static inline int ensure_good_fd(int fd)
> > > +{
> > > + int old_fd = fd, save_errno;
> > > +
> > > + if (unlikely(fd >= 0 && fd < 3)) {
> > > + fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
> > > + if (fd < 0) {
> > > + save_errno = errno;
> > > + pr_warn("failed to dup FD %d to FD > 2: %d\n", old_fd, -errno);
> > > + }
> > > + close(old_fd);
> > > + if (fd < 0)
> > > + errno = save_errno;
> > > + }
> > > + return fd;
> >
> > with all the if nestedness it looks more complicated than it needs to.
> > How about something like this:
> >
> > if (fd < 0)
> > return fd;
> >
> > if (fd < 3) {
> > fd = fcnt(...);
> > save_errno = errno;
> > close(old_fd);
> > if (fd < 0) {
> > pr_warn(...);
> > errno = saved_errno;
> > }
> > }
> >
> > return fd;
> >
> >
> > WDYT?
> >
>
> Looks better, I'll do it like this.
>
Thanks.
> > > +}
> > > +
[...]
>
> >
> > But also, why do still need to "reserve" bad FDs at all with all the
> > ensure_good_fd() business?
> >
>
> This file is included in light skeleton, so it never goes through libbpf syscall
oh yeah? ;) how about bpf_create_map_name() just a few lines below?..
But in general, I don't think you should worry about this FD problem
for skeleton at all. Even if FD 0 is unused,
skel_sys_bpf(BPF_PROG_LOAD) will take it, so whatever loader prog is
doing in the kernel will get FD > 0 automatically. I think you can
drop this fragile "FD reservation" logic.
> wrappers, also, not only will it invoke bpf syscall before calling
> BPF_PROG_TEST_RUN, but also inside the syscall loader program. In there, doing
> the fcntl stuff is much ugly (add a helper, more BPF assembly in gen_loader), so
> I just reserve 3 fds in the beginning of the code sequence where fds are
> acquired, and release them after we're done.
>
> > > + close(fd);
> > > + break;
> > > + }
> > > + fds[i] = fd;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > static inline int bpf_load_and_run(struct bpf_load_and_run_opts *opts)
> > > {
> > > - int map_fd = -1, prog_fd = -1, key = 0, err;
> > > + int map_fd = -1, prog_fd = -1, key = 0, err, i;
> > > + int res_fds[3] = { -1, -1, -1 };
> > > union bpf_attr attr;
> > >
> > > + /* ensures that we don't open fd 0, 1, or 2 from here on out */
> > > + err = skel_reserve_bad_fds(opts, res_fds);
> > > + if (err < 0) {
> > > + errno = -err;
> > > + goto out;
> > > + }
> > > +
> > > map_fd = bpf_create_map_name(BPF_MAP_TYPE_ARRAY, "__loader.map", 4,
> > > opts->data_sz, 1, 0);
> > > if (map_fd < 0) {
> > > @@ -115,6 +144,10 @@ static inline int bpf_load_and_run(struct bpf_load_and_run_opts *opts)
> > > }
> > > err = 0;
> > > out:
> > > + for (i = 0; i < 3; i++) {
> > > + if (res_fds[i] >= 0)
> > > + close(res_fds[i]);
> > > + }
> > > if (map_fd >= 0)
> > > close(map_fd);
> > > if (prog_fd >= 0)
> > > --
> > > 2.33.1
> > >
>
> --
> Kartikeya
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH bpf-next v4 5/8] libbpf: Use O_CLOEXEC uniformly when opening fds
2021-10-20 19:15 [PATCH bpf-next v4 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
` (3 preceding siblings ...)
2021-10-20 19:15 ` [PATCH bpf-next v4 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2 Kumar Kartikeya Dwivedi
@ 2021-10-20 19:15 ` Kumar Kartikeya Dwivedi
2021-10-20 19:15 ` [PATCH bpf-next v4 6/8] selftests/bpf: Add weak/typeless ksym test for light skeleton Kumar Kartikeya Dwivedi
` (2 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-20 19:15 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen, netdev
There are some instances where we don't use O_CLOEXEC when opening an
fd, fix these up. Otherwise, it is possible that a parallel fork causes
these fds to leak into a child process on execve.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/lib/bpf/btf.c | 2 +-
tools/lib/bpf/libbpf.c | 6 +++---
tools/lib/bpf/libbpf_probes.c | 2 +-
tools/lib/bpf/linker.c | 4 ++--
tools/lib/bpf/xsk.c | 6 +++---
5 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 1f6dea11f600..6e02f11e71ef 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -886,7 +886,7 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
return ERR_PTR(-LIBBPF_ERRNO__LIBELF);
}
- fd = open(path, O_RDONLY);
+ fd = open(path, O_RDONLY | O_CLOEXEC);
if (fd < 0) {
err = -errno;
pr_warn("failed to open %s: %s\n", path, strerror(errno));
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index d4d2842e31ea..003368435170 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1223,7 +1223,7 @@ static int bpf_object__elf_init(struct bpf_object *obj)
obj->efile.elf = elf_memory((char *)obj->efile.obj_buf,
obj->efile.obj_buf_sz);
} else {
- obj->efile.fd = ensure_good_fd(open(obj->path, O_RDONLY));
+ obj->efile.fd = ensure_good_fd(open(obj->path, O_RDONLY | O_CLOEXEC));
if (obj->efile.fd < 0) {
char errmsg[STRERR_BUFSIZE], *cp;
@@ -9329,7 +9329,7 @@ static int append_to_file(const char *file, const char *fmt, ...)
int fd, n, err = 0;
va_list ap;
- fd = open(file, O_WRONLY | O_APPEND, 0);
+ fd = open(file, O_WRONLY | O_APPEND | O_CLOEXEC, 0);
if (fd < 0)
return -errno;
@@ -10974,7 +10974,7 @@ int parse_cpu_mask_file(const char *fcpu, bool **mask, int *mask_sz)
int fd, err = 0, len;
char buf[128];
- fd = open(fcpu, O_RDONLY);
+ fd = open(fcpu, O_RDONLY | O_CLOEXEC);
if (fd < 0) {
err = -errno;
pr_warn("Failed to open cpu mask file %s: %d\n", fcpu, err);
diff --git a/tools/lib/bpf/libbpf_probes.c b/tools/lib/bpf/libbpf_probes.c
index cd8c703dde71..68f2dbf364aa 100644
--- a/tools/lib/bpf/libbpf_probes.c
+++ b/tools/lib/bpf/libbpf_probes.c
@@ -33,7 +33,7 @@ static int get_vendor_id(int ifindex)
snprintf(path, sizeof(path), "/sys/class/net/%s/device/vendor", ifname);
- fd = open(path, O_RDONLY);
+ fd = open(path, O_RDONLY | O_CLOEXEC);
if (fd < 0)
return -1;
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index 6106a0b5572a..f993706eff77 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -302,7 +302,7 @@ static int init_output_elf(struct bpf_linker *linker, const char *file)
if (!linker->filename)
return -ENOMEM;
- linker->fd = ensure_good_fd(open(file, O_WRONLY | O_CREAT | O_TRUNC, 0644));
+ linker->fd = ensure_good_fd(open(file, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0644));
if (linker->fd < 0) {
err = -errno;
pr_warn("failed to create '%s': %d\n", file, err);
@@ -557,7 +557,7 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
obj->filename = filename;
- obj->fd = open(filename, O_RDONLY);
+ obj->fd = open(filename, O_RDONLY | O_CLOEXEC);
if (obj->fd < 0) {
err = -errno;
pr_warn("failed to open file '%s': %d\n", filename, err);
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index a2111696ba91..81f8fbc85e70 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -300,7 +300,7 @@ int xsk_umem__create_v0_0_4(struct xsk_umem **umem_ptr, void *umem_area,
if (!umem)
return -ENOMEM;
- umem->fd = socket(AF_XDP, SOCK_RAW, 0);
+ umem->fd = socket(AF_XDP, SOCK_RAW | SOCK_CLOEXEC, 0);
if (umem->fd < 0) {
err = -errno;
goto out_umem_alloc;
@@ -549,7 +549,7 @@ static int xsk_get_max_queues(struct xsk_socket *xsk)
struct ifreq ifr = {};
int fd, err, ret;
- fd = socket(AF_LOCAL, SOCK_DGRAM, 0);
+ fd = socket(AF_LOCAL, SOCK_DGRAM | SOCK_CLOEXEC, 0);
if (fd < 0)
return -errno;
@@ -1046,7 +1046,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr,
}
if (umem->refcount++ > 0) {
- xsk->fd = socket(AF_XDP, SOCK_RAW, 0);
+ xsk->fd = socket(AF_XDP, SOCK_RAW | SOCK_CLOEXEC, 0);
if (xsk->fd < 0) {
err = -errno;
goto out_xsk_alloc;
--
2.33.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH bpf-next v4 6/8] selftests/bpf: Add weak/typeless ksym test for light skeleton
2021-10-20 19:15 [PATCH bpf-next v4 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
` (4 preceding siblings ...)
2021-10-20 19:15 ` [PATCH bpf-next v4 5/8] libbpf: Use O_CLOEXEC uniformly when opening fds Kumar Kartikeya Dwivedi
@ 2021-10-20 19:15 ` Kumar Kartikeya Dwivedi
2021-10-20 22:17 ` Andrii Nakryiko
2021-10-20 19:15 ` [PATCH bpf-next v4 7/8] selftests/bpf: Fix fd cleanup in sk_lookup test Kumar Kartikeya Dwivedi
2021-10-20 19:15 ` [PATCH bpf-next v4 8/8] selftests/bpf: Fix memory leak in test_ima Kumar Kartikeya Dwivedi
7 siblings, 1 reply; 16+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-20 19:15 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen, netdev
Also, avoid using CO-RE features, as lskel doesn't support CO-RE, yet.
Include both light and libbpf skeleton in same file to test both of them
together.
In c48e51c8b07a ("bpf: selftests: Add selftests for module kfunc support"),
I added support for generating both lskel and libbpf skel for a BPF
object, however the name parameter for bpftool caused collisions when
included in same file together. This meant that every test needed a
separate file for a libbpf/light skeleton separation instead of
subtests.
Change that by appending a "_light" suffix to the name for files listed
in LSKELS_EXTRA, such that both light and libbpf skeleton can be used in
the same file for subtests, leading to better code sharing.
While at it, improve the build output by saying GEN-LSKEL instead of
GEN-SKEL for light skeleton generation recipe.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/testing/selftests/bpf/Makefile | 7 ++--
.../selftests/bpf/prog_tests/ksyms_btf.c | 35 +++++++++++++++-
.../selftests/bpf/prog_tests/ksyms_module.c | 40 +++++++++++++++++--
.../bpf/prog_tests/ksyms_module_libbpf.c | 28 -------------
.../selftests/bpf/progs/test_ksyms_weak.c | 3 +-
5 files changed, 74 insertions(+), 39 deletions(-)
delete mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_module_libbpf.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 498222543c37..1c3c8befc249 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -325,7 +325,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
@@ -399,12 +399,13 @@ $(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
$(Q)$$(BPFTOOL) gen skeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
$(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
- $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
+ $$(call msg,GEN-LSKEL,$(TRUNNER_BINARY),$$@)
$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked2.o) $$(<:.o=.linked1.o)
$(Q)$$(BPFTOOL) gen object $$(<:.o=.linked3.o) $$(<:.o=.linked2.o)
$(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
- $(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
+ $$(eval LSKEL_NAME := $$(notdir $$(<:.o=$$(if $$(filter $$(notdir $$(<:.o=.c)),$(LSKELS_EXTRA)),_light,))))
+ $(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.linked3.o) name $$(LSKEL_NAME) > $$@
$(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
$$(call msg,LINK-BPF,$(TRUNNER_BINARY),$$(@:.skel.h=.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..9f4853781702 100644
--- a/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c
+++ b/tools/testing/selftests/bpf/prog_tests/ksyms_btf.c
@@ -7,6 +7,7 @@
#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 +90,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 */
@@ -109,6 +110,33 @@ static void test_weak_syms(void)
test_ksyms_weak__destroy(skel);
}
+static void test_weak_syms_light(void)
+{
+ struct test_ksyms_weak_light *skel;
+ struct test_ksyms_weak_light__data *data;
+ int err;
+
+ skel = test_ksyms_weak_light__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "test_ksyms_weak_light__open_and_load"))
+ return;
+
+ err = test_ksyms_weak_light__attach(skel);
+ if (!ASSERT_OK(err, "test_ksyms_weak_light__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_light__destroy(skel);
+}
+
void test_ksyms_btf(void)
{
int percpu_datasec;
@@ -136,4 +164,7 @@ void test_ksyms_btf(void)
if (test__start_subtest("weak_ksyms"))
test_weak_syms();
+
+ if (test__start_subtest("weak_ksyms_light"))
+ test_weak_syms_light();
}
diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_module.c b/tools/testing/selftests/bpf/prog_tests/ksyms_module.c
index 831447878d7b..653a1352ccb8 100644
--- a/tools/testing/selftests/bpf/prog_tests/ksyms_module.c
+++ b/tools/testing/selftests/bpf/prog_tests/ksyms_module.c
@@ -4,10 +4,11 @@
#include <test_progs.h>
#include <network_helpers.h>
#include "test_ksyms_module.lskel.h"
+#include "test_ksyms_module.skel.h"
-void test_ksyms_module(void)
+void test_ksyms_module_light(void)
{
- struct test_ksyms_module *skel;
+ struct test_ksyms_module_light *skel;
int retval;
int err;
@@ -16,8 +17,8 @@ void test_ksyms_module(void)
return;
}
- skel = test_ksyms_module__open_and_load();
- if (!ASSERT_OK_PTR(skel, "test_ksyms_module__open_and_load"))
+ skel = test_ksyms_module_light__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "test_ksyms_module_light__open_and_load"))
return;
err = bpf_prog_test_run(skel->progs.load.prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
NULL, NULL, (__u32 *)&retval, NULL);
@@ -25,6 +26,37 @@ void test_ksyms_module(void)
goto cleanup;
ASSERT_EQ(retval, 0, "retval");
ASSERT_EQ(skel->bss->out_bpf_testmod_ksym, 42, "bpf_testmod_ksym");
+cleanup:
+ test_ksyms_module_light__destroy(skel);
+}
+
+void test_ksyms_module_libbpf(void)
+{
+ struct test_ksyms_module *skel;
+ int retval, err;
+
+ if (!env.has_testmod) {
+ test__skip();
+ return;
+ }
+
+ skel = test_ksyms_module__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "test_ksyms_module__open"))
+ return;
+ err = bpf_prog_test_run(bpf_program__fd(skel->progs.load), 1, &pkt_v4,
+ sizeof(pkt_v4), NULL, NULL, (__u32 *)&retval, NULL);
+ if (!ASSERT_OK(err, "bpf_prog_test_run"))
+ goto cleanup;
+ ASSERT_EQ(retval, 0, "retval");
+ ASSERT_EQ(skel->bss->out_bpf_testmod_ksym, 42, "bpf_testmod_ksym");
cleanup:
test_ksyms_module__destroy(skel);
}
+
+void test_ksyms_module(void)
+{
+ if (test__start_subtest("light"))
+ test_ksyms_module_light();
+ if (test__start_subtest("libbpf"))
+ test_ksyms_module_libbpf();
+}
diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms_module_libbpf.c b/tools/testing/selftests/bpf/prog_tests/ksyms_module_libbpf.c
deleted file mode 100644
index e6343ef63af9..000000000000
--- a/tools/testing/selftests/bpf/prog_tests/ksyms_module_libbpf.c
+++ /dev/null
@@ -1,28 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-#include <test_progs.h>
-#include <network_helpers.h>
-#include "test_ksyms_module.skel.h"
-
-void test_ksyms_module_libbpf(void)
-{
- struct test_ksyms_module *skel;
- int retval, err;
-
- if (!env.has_testmod) {
- test__skip();
- return;
- }
-
- skel = test_ksyms_module__open_and_load();
- if (!ASSERT_OK_PTR(skel, "test_ksyms_module__open"))
- return;
- err = bpf_prog_test_run(bpf_program__fd(skel->progs.load), 1, &pkt_v4,
- sizeof(pkt_v4), NULL, NULL, (__u32 *)&retval, NULL);
- if (!ASSERT_OK(err, "bpf_prog_test_run"))
- goto cleanup;
- ASSERT_EQ(retval, 0, "retval");
- ASSERT_EQ(skel->bss->out_bpf_testmod_ksym, 42, "bpf_testmod_ksym");
-cleanup:
- test_ksyms_module__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.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next v4 6/8] selftests/bpf: Add weak/typeless ksym test for light skeleton
2021-10-20 19:15 ` [PATCH bpf-next v4 6/8] selftests/bpf: Add weak/typeless ksym test for light skeleton Kumar Kartikeya Dwivedi
@ 2021-10-20 22:17 ` Andrii Nakryiko
2021-10-20 22:37 ` Kumar Kartikeya Dwivedi
0 siblings, 1 reply; 16+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 22:17 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen,
Networking
On Wed, Oct 20, 2021 at 12:15 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Also, avoid using CO-RE features, as lskel doesn't support CO-RE, yet.
> Include both light and libbpf skeleton in same file to test both of them
> together.
>
> In c48e51c8b07a ("bpf: selftests: Add selftests for module kfunc support"),
> I added support for generating both lskel and libbpf skel for a BPF
> object, however the name parameter for bpftool caused collisions when
> included in same file together. This meant that every test needed a
> separate file for a libbpf/light skeleton separation instead of
> subtests.
>
> Change that by appending a "_light" suffix to the name for files listed
> in LSKELS_EXTRA, such that both light and libbpf skeleton can be used in
> the same file for subtests, leading to better code sharing.
>
> While at it, improve the build output by saying GEN-LSKEL instead of
> GEN-SKEL for light skeleton generation recipe.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
> tools/testing/selftests/bpf/Makefile | 7 ++--
> .../selftests/bpf/prog_tests/ksyms_btf.c | 35 +++++++++++++++-
> .../selftests/bpf/prog_tests/ksyms_module.c | 40 +++++++++++++++++--
> .../bpf/prog_tests/ksyms_module_libbpf.c | 28 -------------
> .../selftests/bpf/progs/test_ksyms_weak.c | 3 +-
> 5 files changed, 74 insertions(+), 39 deletions(-)
> delete mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_module_libbpf.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 498222543c37..1c3c8befc249 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -325,7 +325,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
> @@ -399,12 +399,13 @@ $(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> $(Q)$$(BPFTOOL) gen skeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
>
> $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> - $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> + $$(call msg,GEN-LSKEL,$(TRUNNER_BINARY),$$@)
This breaks nice output alignment:
GEN-SKEL [test_progs-no_alu32] bpf_iter_tcp4.skel.h
GEN-LSKEL [test_progs-no_alu32] trace_vprintk.lskel.h
Isn't ".lskel.h" suffix enough to distinguish them?
> $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
> $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked2.o) $$(<:.o=.linked1.o)
> $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked3.o) $$(<:.o=.linked2.o)
> $(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
> - $(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
> + $$(eval LSKEL_NAME := $$(notdir $$(<:.o=$$(if $$(filter $$(notdir $$(<:.o=.c)),$(LSKELS_EXTRA)),_light,))))
eval inside eval?.. Wow, do we really need that? If you just want to
add _light (I suggest _lskel though, it will make for a more
meaningful and recognizable names in user-space code) suffix, do it
for all light skeletons unconditionally and keep it simple?
> + $(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.linked3.o) name $$(LSKEL_NAME) > $$@
>
> $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
> $$(call msg,LINK-BPF,$(TRUNNER_BINARY),$$(@:.skel.h=.o))
[...]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next v4 6/8] selftests/bpf: Add weak/typeless ksym test for light skeleton
2021-10-20 22:17 ` Andrii Nakryiko
@ 2021-10-20 22:37 ` Kumar Kartikeya Dwivedi
2021-10-20 22:43 ` Andrii Nakryiko
0 siblings, 1 reply; 16+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-20 22:37 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen,
Networking
On Thu, Oct 21, 2021 at 03:47:19AM IST, Andrii Nakryiko wrote:
> On Wed, Oct 20, 2021 at 12:15 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > Also, avoid using CO-RE features, as lskel doesn't support CO-RE, yet.
> > Include both light and libbpf skeleton in same file to test both of them
> > together.
> >
> > In c48e51c8b07a ("bpf: selftests: Add selftests for module kfunc support"),
> > I added support for generating both lskel and libbpf skel for a BPF
> > object, however the name parameter for bpftool caused collisions when
> > included in same file together. This meant that every test needed a
> > separate file for a libbpf/light skeleton separation instead of
> > subtests.
> >
> > Change that by appending a "_light" suffix to the name for files listed
> > in LSKELS_EXTRA, such that both light and libbpf skeleton can be used in
> > the same file for subtests, leading to better code sharing.
> >
> > While at it, improve the build output by saying GEN-LSKEL instead of
> > GEN-SKEL for light skeleton generation recipe.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> > tools/testing/selftests/bpf/Makefile | 7 ++--
> > .../selftests/bpf/prog_tests/ksyms_btf.c | 35 +++++++++++++++-
> > .../selftests/bpf/prog_tests/ksyms_module.c | 40 +++++++++++++++++--
> > .../bpf/prog_tests/ksyms_module_libbpf.c | 28 -------------
> > .../selftests/bpf/progs/test_ksyms_weak.c | 3 +-
> > 5 files changed, 74 insertions(+), 39 deletions(-)
> > delete mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_module_libbpf.c
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 498222543c37..1c3c8befc249 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -325,7 +325,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
> > @@ -399,12 +399,13 @@ $(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> > $(Q)$$(BPFTOOL) gen skeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
> >
> > $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> > - $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> > + $$(call msg,GEN-LSKEL,$(TRUNNER_BINARY),$$@)
>
> This breaks nice output alignment:
>
> GEN-SKEL [test_progs-no_alu32] bpf_iter_tcp4.skel.h
> GEN-LSKEL [test_progs-no_alu32] trace_vprintk.lskel.h
>
Ok, I'll drop it.
> Isn't ".lskel.h" suffix enough to distinguish them?
>
> > $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
> > $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked2.o) $$(<:.o=.linked1.o)
> > $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked3.o) $$(<:.o=.linked2.o)
> > $(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
> > - $(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
> > + $$(eval LSKEL_NAME := $$(notdir $$(<:.o=$$(if $$(filter $$(notdir $$(<:.o=.c)),$(LSKELS_EXTRA)),_light,))))
>
> eval inside eval?.. Wow, do we really need that? If you just want to
I knew you'd like it ;-)
> add _light (I suggest _lskel though, it will make for a more
> meaningful and recognizable names in user-space code) suffix, do it
> for all light skeletons unconditionally and keep it simple?
>
I avoided this because it would be a lot of unecessary changes, while we only
need this for two files. Every struct/function name will get the suffix, but I
can do it if the Makefile stuff looks too horrible.
> > + $(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.linked3.o) name $$(LSKEL_NAME) > $$@
> >
> > $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
> > $$(call msg,LINK-BPF,$(TRUNNER_BINARY),$$(@:.skel.h=.o))
>
> [...]
--
Kartikeya
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next v4 6/8] selftests/bpf: Add weak/typeless ksym test for light skeleton
2021-10-20 22:37 ` Kumar Kartikeya Dwivedi
@ 2021-10-20 22:43 ` Andrii Nakryiko
0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 22:43 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen,
Networking
On Wed, Oct 20, 2021 at 3:37 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> On Thu, Oct 21, 2021 at 03:47:19AM IST, Andrii Nakryiko wrote:
> > On Wed, Oct 20, 2021 at 12:15 PM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> > >
> > > Also, avoid using CO-RE features, as lskel doesn't support CO-RE, yet.
> > > Include both light and libbpf skeleton in same file to test both of them
> > > together.
> > >
> > > In c48e51c8b07a ("bpf: selftests: Add selftests for module kfunc support"),
> > > I added support for generating both lskel and libbpf skel for a BPF
> > > object, however the name parameter for bpftool caused collisions when
> > > included in same file together. This meant that every test needed a
> > > separate file for a libbpf/light skeleton separation instead of
> > > subtests.
> > >
> > > Change that by appending a "_light" suffix to the name for files listed
> > > in LSKELS_EXTRA, such that both light and libbpf skeleton can be used in
> > > the same file for subtests, leading to better code sharing.
> > >
> > > While at it, improve the build output by saying GEN-LSKEL instead of
> > > GEN-SKEL for light skeleton generation recipe.
> > >
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > > tools/testing/selftests/bpf/Makefile | 7 ++--
> > > .../selftests/bpf/prog_tests/ksyms_btf.c | 35 +++++++++++++++-
> > > .../selftests/bpf/prog_tests/ksyms_module.c | 40 +++++++++++++++++--
> > > .../bpf/prog_tests/ksyms_module_libbpf.c | 28 -------------
> > > .../selftests/bpf/progs/test_ksyms_weak.c | 3 +-
> > > 5 files changed, 74 insertions(+), 39 deletions(-)
> > > delete mode 100644 tools/testing/selftests/bpf/prog_tests/ksyms_module_libbpf.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > index 498222543c37..1c3c8befc249 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -325,7 +325,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
> > > @@ -399,12 +399,13 @@ $(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> > > $(Q)$$(BPFTOOL) gen skeleton $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
> > >
> > > $(TRUNNER_BPF_LSKELS): %.lskel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> > > - $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> > > + $$(call msg,GEN-LSKEL,$(TRUNNER_BINARY),$$@)
> >
> > This breaks nice output alignment:
> >
> > GEN-SKEL [test_progs-no_alu32] bpf_iter_tcp4.skel.h
> > GEN-LSKEL [test_progs-no_alu32] trace_vprintk.lskel.h
> >
>
> Ok, I'll drop it.
>
> > Isn't ".lskel.h" suffix enough to distinguish them?
> >
> > > $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked1.o) $$<
> > > $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked2.o) $$(<:.o=.linked1.o)
> > > $(Q)$$(BPFTOOL) gen object $$(<:.o=.linked3.o) $$(<:.o=.linked2.o)
> > > $(Q)diff $$(<:.o=.linked2.o) $$(<:.o=.linked3.o)
> > > - $(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.linked3.o) name $$(notdir $$(<:.o=)) > $$@
> > > + $$(eval LSKEL_NAME := $$(notdir $$(<:.o=$$(if $$(filter $$(notdir $$(<:.o=.c)),$(LSKELS_EXTRA)),_light,))))
> >
> > eval inside eval?.. Wow, do we really need that? If you just want to
>
> I knew you'd like it ;-)
yeah, big fan, obviously :)
>
> > add _light (I suggest _lskel though, it will make for a more
> > meaningful and recognizable names in user-space code) suffix, do it
> > for all light skeletons unconditionally and keep it simple?
> >
>
> I avoided this because it would be a lot of unecessary changes, while we only
> need this for two files. Every struct/function name will get the suffix, but I
> can do it if the Makefile stuff looks too horrible.
There are 10 light skeleton-using selftests, it's not a problem to do
one-time renaming. Currently there are only two files that do both
skel and lskel, but that number will inevitably grow, so better to set
everything up now.
>
> > > + $(Q)$$(BPFTOOL) gen skeleton -L $$(<:.o=.linked3.o) name $$(LSKEL_NAME) > $$@
> > >
> > > $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_BPF_OBJS) $(BPFTOOL) | $(TRUNNER_OUTPUT)
> > > $$(call msg,LINK-BPF,$(TRUNNER_BINARY),$$(@:.skel.h=.o))
> >
> > [...]
>
> --
> Kartikeya
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH bpf-next v4 7/8] selftests/bpf: Fix fd cleanup in sk_lookup test
2021-10-20 19:15 [PATCH bpf-next v4 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
` (5 preceding siblings ...)
2021-10-20 19:15 ` [PATCH bpf-next v4 6/8] selftests/bpf: Add weak/typeless ksym test for light skeleton Kumar Kartikeya Dwivedi
@ 2021-10-20 19:15 ` Kumar Kartikeya Dwivedi
2021-10-20 22:19 ` Andrii Nakryiko
2021-10-20 19:15 ` [PATCH bpf-next v4 8/8] selftests/bpf: Fix memory leak in test_ima Kumar Kartikeya Dwivedi
7 siblings, 1 reply; 16+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-20 19:15 UTC (permalink / raw)
To: bpf
Cc: Jakub Sitnicki, Song Liu, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Yonghong Song,
Jesper Dangaard Brouer, Toke Høiland-Jørgensen, netdev
Similar to the fix in commit:
e31eec77e4ab ("bpf: selftests: Fix fd cleanup in get_branch_snapshot")
We use designated initializer to set fds to -1 without breaking on
future changes to MAX_SERVER constant denoting the array size.
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.
Fixes: 0ab5539f8584 ("selftests/bpf: Tests for BPF_SK_LOOKUP attach point")
Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/testing/selftests/bpf/prog_tests/sk_lookup.c | 14 ++++++++------
1 file changed, 8 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..cbee46d2d525 100644
--- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
+++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
@@ -598,7 +598,7 @@ 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 server_fds[] = { [0 ... MAX_SERVERS - 1] = -1 };
int client_fd, reuse_conn_fd = -1;
struct bpf_link *lookup_link;
int i, err;
@@ -663,8 +663,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,7 +1054,7 @@ 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[] = { [0 ... MAX_SERVERS - 1] = -1 };
struct bpf_sk_lookup ctx;
__u64 server_cookie;
int i, err;
@@ -1097,8 +1098,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.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH bpf-next v4 7/8] selftests/bpf: Fix fd cleanup in sk_lookup test
2021-10-20 19:15 ` [PATCH bpf-next v4 7/8] selftests/bpf: Fix fd cleanup in sk_lookup test Kumar Kartikeya Dwivedi
@ 2021-10-20 22:19 ` Andrii Nakryiko
0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2021-10-20 22:19 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Jakub Sitnicki, Song Liu, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau,
Yonghong Song, Jesper Dangaard Brouer,
Toke Høiland-Jørgensen, Networking
On Wed, Oct 20, 2021 at 12:15 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> Similar to the fix in commit:
> e31eec77e4ab ("bpf: selftests: Fix fd cleanup in get_branch_snapshot")
>
> We use designated initializer to set fds to -1 without breaking on
> future changes to MAX_SERVER constant denoting the array size.
>
> 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.
>
> Fixes: 0ab5539f8584 ("selftests/bpf: Tests for BPF_SK_LOOKUP attach point")
> Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
> tools/testing/selftests/bpf/prog_tests/sk_lookup.c | 14 ++++++++------
> 1 file changed, 8 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..cbee46d2d525 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
> @@ -598,7 +598,7 @@ 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 server_fds[] = { [0 ... MAX_SERVERS - 1] = -1 };
if you have this, why do you need early break logic below?
> int client_fd, reuse_conn_fd = -1;
> struct bpf_link *lookup_link;
> int i, err;
> @@ -663,8 +663,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,7 +1054,7 @@ 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[] = { [0 ... MAX_SERVERS - 1] = -1 };
> struct bpf_sk_lookup ctx;
> __u64 server_cookie;
> int i, err;
> @@ -1097,8 +1098,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.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH bpf-next v4 8/8] selftests/bpf: Fix memory leak in test_ima
2021-10-20 19:15 [PATCH bpf-next v4 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
` (6 preceding siblings ...)
2021-10-20 19:15 ` [PATCH bpf-next v4 7/8] selftests/bpf: Fix fd cleanup in sk_lookup test Kumar Kartikeya Dwivedi
@ 2021-10-20 19:15 ` Kumar Kartikeya Dwivedi
7 siblings, 0 replies; 16+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-20 19:15 UTC (permalink / raw)
To: bpf
Cc: Andrii Nakryiko, Song Liu, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Yonghong Song, Jesper Dangaard Brouer,
Toke Høiland-Jørgensen, netdev
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")
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Acked-by: Song Liu <songliubraving@fb.com>
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.1
^ permalink raw reply related [flat|nested] 16+ messages in thread