* [PATCH bpf-next v2 1/8] bpf: Add bpf_kallsyms_lookup_name helper
2021-10-13 7:33 [PATCH bpf-next v2 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
@ 2021-10-13 7:33 ` Kumar Kartikeya Dwivedi
2021-10-13 7:33 ` [PATCH bpf-next v2 2/8] libbpf: Add typeless ksym support to gen_loader Kumar Kartikeya Dwivedi
` (6 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-13 7:33 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 | 14 ++++++++++++++
kernel/bpf/syscall.c | 24 ++++++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 14 ++++++++++++++
4 files changed, 53 insertions(+)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d604c8251d88..17206aae329d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2107,6 +2107,7 @@ extern const struct bpf_func_proto bpf_for_each_map_elem_proto;
extern const struct bpf_func_proto bpf_btf_find_by_name_kind_proto;
extern const struct bpf_func_proto bpf_sk_setsockopt_proto;
extern const struct bpf_func_proto bpf_sk_getsockopt_proto;
+extern const struct bpf_func_proto bpf_kallsyms_lookup_name_proto;
const struct bpf_func_proto *tracing_prog_func_proto(
enum bpf_func_id func_id, const struct bpf_prog *prog);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6fc59d61937a..bbd0a3f4e5f6 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4909,6 +4909,19 @@ union bpf_attr {
* Return
* The number of bytes written to the buffer, or a negative error
* in case of failure.
+ *
+ * long bpf_kallsyms_lookup_name(const char *name, int name_sz, int flags, u64 *res)
+ * Description
+ * Get the address of a kernel symbol, returned in *res*. *res* is
+ * set to 0 if the symbol is not found.
+ * Return
+ * On success, zero. On error, a negative value.
+ *
+ * **-EINVAL** if *flags* is not zero.
+ *
+ * **-EINVAL** if string *name* is not the same size as *name_sz*.
+ *
+ * **-ENOENT** if symbol is not found.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -5089,6 +5102,7 @@ union bpf_attr {
FN(task_pt_regs), \
FN(get_branch_snapshot), \
FN(trace_vprintk), \
+ FN(kallsyms_lookup_name), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4e50c0bfdb7d..073ca9ebe58b 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4753,6 +4753,28 @@ static const struct bpf_func_proto bpf_sys_close_proto = {
.arg1_type = ARG_ANYTHING,
};
+BPF_CALL_4(bpf_kallsyms_lookup_name, const char *, name, int, name_sz, int, flags, u64 *, res)
+{
+ if (flags)
+ return -EINVAL;
+
+ if (name_sz <= 1 || name[name_sz - 1])
+ return -EINVAL;
+
+ *res = kallsyms_lookup_name(name);
+ return *res ? 0 : -ENOENT;
+}
+
+const struct bpf_func_proto bpf_kallsyms_lookup_name_proto = {
+ .func = bpf_kallsyms_lookup_name,
+ .gpl_only = true,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_MEM,
+ .arg2_type = ARG_CONST_SIZE,
+ .arg3_type = ARG_ANYTHING,
+ .arg4_type = ARG_PTR_TO_LONG,
+};
+
static const struct bpf_func_proto *
syscall_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
{
@@ -4763,6 +4785,8 @@ syscall_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_btf_find_by_name_kind_proto;
case BPF_FUNC_sys_close:
return &bpf_sys_close_proto;
+ case BPF_FUNC_kallsyms_lookup_name:
+ return &bpf_kallsyms_lookup_name_proto;
default:
return tracing_prog_func_proto(func_id, prog);
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 6fc59d61937a..bbd0a3f4e5f6 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4909,6 +4909,19 @@ union bpf_attr {
* Return
* The number of bytes written to the buffer, or a negative error
* in case of failure.
+ *
+ * long bpf_kallsyms_lookup_name(const char *name, int name_sz, int flags, u64 *res)
+ * Description
+ * Get the address of a kernel symbol, returned in *res*. *res* is
+ * set to 0 if the symbol is not found.
+ * Return
+ * On success, zero. On error, a negative value.
+ *
+ * **-EINVAL** if *flags* is not zero.
+ *
+ * **-EINVAL** if string *name* is not the same size as *name_sz*.
+ *
+ * **-ENOENT** if symbol is not found.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -5089,6 +5102,7 @@ union bpf_attr {
FN(task_pt_regs), \
FN(get_branch_snapshot), \
FN(trace_vprintk), \
+ FN(kallsyms_lookup_name), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
--
2.33.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH bpf-next v2 2/8] libbpf: Add typeless ksym support to gen_loader
2021-10-13 7:33 [PATCH bpf-next v2 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
2021-10-13 7:33 ` [PATCH bpf-next v2 1/8] bpf: Add bpf_kallsyms_lookup_name helper Kumar Kartikeya Dwivedi
@ 2021-10-13 7:33 ` Kumar Kartikeya Dwivedi
2021-10-14 16:39 ` Song Liu
2021-10-13 7:33 ` [PATCH bpf-next v2 3/8] libbpf: Add weak " Kumar Kartikeya Dwivedi
` (5 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-13 7:33 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.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
tools/lib/bpf/bpf_gen_internal.h | 12 +++--
tools/lib/bpf/gen_loader.c | 82 ++++++++++++++++++++++++++++++--
tools/lib/bpf/libbpf.c | 13 ++---
3 files changed, 92 insertions(+), 15 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..6be17dd48f02 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,52 @@ static void emit_relo_kfunc_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo
relo->name, kdesc->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:
+ if (!gen->log_level)
+ return;
+ emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_8,
+ offsetof(struct bpf_insn, imm)));
+ emit(gen, BPF_LDX_MEM(BPF_H, BPF_REG_9, BPF_REG_8, sizeof(struct bpf_insn) +
+ offsetof(struct bpf_insn, imm)));
+ debug_regs(gen, BPF_REG_7, BPF_REG_9, " var t=0 w=%d (%s:count=%d): imm[0]: %%d, imm[1]: %%d",
+ relo->is_weak, relo->name, kdesc->ref);
+ emit(gen, BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_8, offsetofend(struct bpf_insn, code)));
+ debug_regs(gen, BPF_REG_9, -1, " var t=0 w=%d (%s:count=%d): insn.reg",
+ relo->is_weak, relo->name, kdesc->ref);
+}
+
/* Expects:
* BPF_REG_8 - pointer to instruction
*/
@@ -748,7 +818,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 +846,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 ae0889bebe32..2ff7c9487634 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6355,17 +6355,14 @@ static int bpf_program__record_externs(struct bpf_program *prog)
case RELO_EXTERN_VAR:
if (ext->type != EXT_KSYM)
continue;
- if (!ext->ksym.type_id) {
- pr_warn("typeless ksym %s is not supported yet\n",
- ext->name);
- return -ENOTSUP;
- }
- bpf_gen__record_extern(obj->gen_loader, ext->name, ext->is_weak,
+ bpf_gen__record_extern(obj->gen_loader, ext->name,
+ ext->is_weak, !ext->ksym.type_id,
BTF_KIND_VAR, relo->insn_idx);
break;
case RELO_EXTERN_FUNC:
- bpf_gen__record_extern(obj->gen_loader, ext->name, ext->is_weak,
- BTF_KIND_FUNC, relo->insn_idx);
+ bpf_gen__record_extern(obj->gen_loader, ext->name,
+ ext->is_weak, 0, BTF_KIND_FUNC,
+ relo->insn_idx);
break;
default:
continue;
--
2.33.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH bpf-next v2 2/8] libbpf: Add typeless ksym support to gen_loader
2021-10-13 7:33 ` [PATCH bpf-next v2 2/8] libbpf: Add typeless ksym support to gen_loader Kumar Kartikeya Dwivedi
@ 2021-10-14 16:39 ` Song Liu
2021-10-14 17:53 ` Kumar Kartikeya Dwivedi
0 siblings, 1 reply; 14+ messages in thread
From: Song Liu @ 2021-10-14 16:39 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin Lau, Yonghong Song, Jesper Dangaard Brouer,
Toke Høiland-Jørgensen, netdev
> On Oct 13, 2021, at 12:33 AM, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> 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.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
[...]
> }
>
> +/* Expects:
> + * BPF_REG_8 - pointer to instruction
> + */
> +static void emit_relo_ksym_typeless(struct bpf_gen *gen,
> + struct ksym_relo_desc *relo, int insn)
This function has quite some duplicated logic as emit_relo_ksym_btf().
I guess we can somehow reuse the code here. Say, we pull changes from
3/8 first to handle weak type. Then we extend the function to handle
typeless. Would this work?
> +{
> + 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:
> + if (!gen->log_level)
> + return;
> + emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_8,
> + offsetof(struct bpf_insn, imm)));
> + emit(gen, BPF_LDX_MEM(BPF_H, BPF_REG_9, BPF_REG_8, sizeof(struct bpf_insn) +
> + offsetof(struct bpf_insn, imm)));
> + debug_regs(gen, BPF_REG_7, BPF_REG_9, " var t=0 w=%d (%s:count=%d): imm[0]: %%d, imm[1]: %%d",
> + relo->is_weak, relo->name, kdesc->ref);
> + emit(gen, BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_8, offsetofend(struct bpf_insn, code)));
> + debug_regs(gen, BPF_REG_9, -1, " var t=0 w=%d (%s:count=%d): insn.reg",
> + relo->is_weak, relo->name, kdesc->ref);
>
[...]
> +++ b/tools/lib/bpf/libbpf.c
> @@ -6355,17 +6355,14 @@ static int bpf_program__record_externs(struct bpf_program *prog)
> case RELO_EXTERN_VAR:
> if (ext->type != EXT_KSYM)
> continue;
> - if (!ext->ksym.type_id) {
> - pr_warn("typeless ksym %s is not supported yet\n",
> - ext->name);
> - return -ENOTSUP;
> - }
> - bpf_gen__record_extern(obj->gen_loader, ext->name, ext->is_weak,
> + bpf_gen__record_extern(obj->gen_loader, ext->name,
> + ext->is_weak, !ext->ksym.type_id,
> BTF_KIND_VAR, relo->insn_idx);
> break;
> case RELO_EXTERN_FUNC:
> - bpf_gen__record_extern(obj->gen_loader, ext->name, ext->is_weak,
> - BTF_KIND_FUNC, relo->insn_idx);
> + bpf_gen__record_extern(obj->gen_loader, ext->name,
> + ext->is_weak, 0, BTF_KIND_FUNC,
nit: Prefer use "false" for bool arguments.
> + relo->insn_idx);
> break;
> default:
> continue;
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH bpf-next v2 2/8] libbpf: Add typeless ksym support to gen_loader
2021-10-14 16:39 ` Song Liu
@ 2021-10-14 17:53 ` Kumar Kartikeya Dwivedi
2021-10-14 19:12 ` Song Liu
0 siblings, 1 reply; 14+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-14 17:53 UTC (permalink / raw)
To: Song Liu
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin Lau, Yonghong Song, Jesper Dangaard Brouer,
Toke Høiland-Jørgensen, netdev
On Thu, Oct 14, 2021 at 10:09:43PM IST, Song Liu wrote:
>
>
> > On Oct 13, 2021, at 12:33 AM, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > 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.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> [...]
> > }
> >
> > +/* Expects:
> > + * BPF_REG_8 - pointer to instruction
> > + */
> > +static void emit_relo_ksym_typeless(struct bpf_gen *gen,
> > + struct ksym_relo_desc *relo, int insn)
>
> This function has quite some duplicated logic as emit_relo_ksym_btf().
> I guess we can somehow reuse the code here. Say, we pull changes from
> 3/8 first to handle weak type. Then we extend the function to handle
> typeless. Would this work?
>
Ok, will put both into the same function in the next version. Though the part
between:
> > +{
> > + 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));
this and ...
> > + 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)));
... this won't overlap (so it will have to jump into the else branch to clear_src_reg).
e.g. it looks something like this:
if (kdesc->ref > 1) {
move...
if (!relo->is_typeless)
...
goto clear_src_reg;
}
kdesc->insn = insn;
...
if (relo->is_typeless) {
...
} else {
...
clear_src_reg:
...
}
so it looked better to split into separate functions (maybe we can just move the
logging part to common helper? the rest is just duplicating the inital get and
move_blob2blob).
> > +log:
> > + if (!gen->log_level)
> > + return;
> > + emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_8,
> > + offsetof(struct bpf_insn, imm)));
> > + emit(gen, BPF_LDX_MEM(BPF_H, BPF_REG_9, BPF_REG_8, sizeof(struct bpf_insn) +
> > + offsetof(struct bpf_insn, imm)));
> > + debug_regs(gen, BPF_REG_7, BPF_REG_9, " var t=0 w=%d (%s:count=%d): imm[0]: %%d, imm[1]: %%d",
> > + relo->is_weak, relo->name, kdesc->ref);
> > + emit(gen, BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_8, offsetofend(struct bpf_insn, code)));
> > + debug_regs(gen, BPF_REG_9, -1, " var t=0 w=%d (%s:count=%d): insn.reg",
> > + relo->is_weak, relo->name, kdesc->ref);
> >
> [...]
>
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -6355,17 +6355,14 @@ static int bpf_program__record_externs(struct bpf_program *prog)
> > case RELO_EXTERN_VAR:
> > if (ext->type != EXT_KSYM)
> > continue;
> > - if (!ext->ksym.type_id) {
> > - pr_warn("typeless ksym %s is not supported yet\n",
> > - ext->name);
> > - return -ENOTSUP;
> > - }
> > - bpf_gen__record_extern(obj->gen_loader, ext->name, ext->is_weak,
> > + bpf_gen__record_extern(obj->gen_loader, ext->name,
> > + ext->is_weak, !ext->ksym.type_id,
> > BTF_KIND_VAR, relo->insn_idx);
> > break;
> > case RELO_EXTERN_FUNC:
> > - bpf_gen__record_extern(obj->gen_loader, ext->name, ext->is_weak,
> > - BTF_KIND_FUNC, relo->insn_idx);
> > + bpf_gen__record_extern(obj->gen_loader, ext->name,
> > + ext->is_weak, 0, BTF_KIND_FUNC,
>
> nit: Prefer use "false" for bool arguments.
>
> > + relo->insn_idx);
> > break;
> > default:
> > continue;
> > --
> > 2.33.0
> >
>
--
Kartikeya
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH bpf-next v2 2/8] libbpf: Add typeless ksym support to gen_loader
2021-10-14 17:53 ` Kumar Kartikeya Dwivedi
@ 2021-10-14 19:12 ` Song Liu
0 siblings, 0 replies; 14+ messages in thread
From: Song Liu @ 2021-10-14 19:12 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin Lau, Yonghong Song, Jesper Dangaard Brouer,
Toke Høiland-Jørgensen, netdev
> On Oct 14, 2021, at 10:53 AM, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Thu, Oct 14, 2021 at 10:09:43PM IST, Song Liu wrote:
>>
>>
>>> On Oct 13, 2021, at 12:33 AM, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>>>
>>> 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.
>>>
>>> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>> [...]
>>> }
>>>
>>> +/* Expects:
>>> + * BPF_REG_8 - pointer to instruction
>>> + */
>>> +static void emit_relo_ksym_typeless(struct bpf_gen *gen,
>>> + struct ksym_relo_desc *relo, int insn)
>>
>> This function has quite some duplicated logic as emit_relo_ksym_btf().
>> I guess we can somehow reuse the code here. Say, we pull changes from
>> 3/8 first to handle weak type. Then we extend the function to handle
>> typeless. Would this work?
>>
>
> Ok, will put both into the same function in the next version. Though the part
> between:
>
>>> +{
>>> + 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));
>
> this and ...
>
>>> + 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)));
>
> ... this won't overlap (so it will have to jump into the else branch to clear_src_reg).
>
> e.g. it looks something like this:
>
> if (kdesc->ref > 1) {
> move...
> if (!relo->is_typeless)
> ...
> goto clear_src_reg;
> }
> kdesc->insn = insn;
> ...
> if (relo->is_typeless) {
> ...
> } else {
> ...
> clear_src_reg:
> ...
> }
>
> so it looked better to split into separate functions (maybe we can just move the
> logging part to common helper? the rest is just duplicating the inital get and
> move_blob2blob).
Yeah, I can tell it not very clean either way. A common helper might be the best
option here.
Thanks,
Song
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH bpf-next v2 3/8] libbpf: Add weak ksym support to gen_loader
2021-10-13 7:33 [PATCH bpf-next v2 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
2021-10-13 7:33 ` [PATCH bpf-next v2 1/8] bpf: Add bpf_kallsyms_lookup_name helper Kumar Kartikeya Dwivedi
2021-10-13 7:33 ` [PATCH bpf-next v2 2/8] libbpf: Add typeless ksym support to gen_loader Kumar Kartikeya Dwivedi
@ 2021-10-13 7:33 ` Kumar Kartikeya Dwivedi
2021-10-14 16:44 ` Song Liu
2021-10-13 7:33 ` [PATCH bpf-next v2 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2 Kumar Kartikeya Dwivedi
` (4 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-13 7:33 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 | 37 ++++++++++++++++++++++++++++++++-----
1 file changed, 32 insertions(+), 5 deletions(-)
diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
index 6be17dd48f02..0e84d644791e 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,6 +777,7 @@ static void emit_relo_ksym_typeless(struct bpf_gen *gen,
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)
@@ -786,27 +788,52 @@ 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 */
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+ reg_mask = 0x0f; /* src_reg,dst_reg,... */
+#elif defined(__BIG_ENDIAN_BITFIELD)
+ reg_mask = 0xf0; /* dst_reg,src_reg,... */
+#else
+#error "Unsupported bit endianness, cannot proceed"
+#endif
+ emit(gen, BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_8, offsetofend(struct bpf_insn, code)));
+ emit(gen, BPF_ALU32_IMM(BPF_AND, BPF_REG_9, reg_mask));
+ emit(gen, BPF_STX_MEM(BPF_B, BPF_REG_8, BPF_REG_9, offsetofend(struct bpf_insn, code)));
+
if (!gen->log_level)
return;
emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_8,
offsetof(struct bpf_insn, imm)));
emit(gen, BPF_LDX_MEM(BPF_H, BPF_REG_9, BPF_REG_8, sizeof(struct bpf_insn) +
offsetof(struct bpf_insn, imm)));
- debug_regs(gen, BPF_REG_7, BPF_REG_9, " var (%s:count=%d): imm: %%d, fd: %%d",
- relo->name, kdesc->ref);
+ debug_regs(gen, BPF_REG_7, BPF_REG_9, " var t=1 w=%d (%s:count=%d): imm[0]: %%d, imm[1]: %%d",
+ relo->is_weak, relo->name, kdesc->ref);
+ emit(gen, BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_8, offsetofend(struct bpf_insn, code)));
+ debug_regs(gen, BPF_REG_9, -1, " var t=1 w=%d (%s:count=%d): insn.reg",
+ relo->is_weak, relo->name, kdesc->ref);
}
static void emit_relo(struct bpf_gen *gen, struct ksym_relo_desc *relo, int insns)
--
2.33.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH bpf-next v2 3/8] libbpf: Add weak ksym support to gen_loader
2021-10-13 7:33 ` [PATCH bpf-next v2 3/8] libbpf: Add weak " Kumar Kartikeya Dwivedi
@ 2021-10-14 16:44 ` Song Liu
0 siblings, 0 replies; 14+ messages in thread
From: Song Liu @ 2021-10-14 16:44 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin Lau, Yonghong Song, Jesper Dangaard Brouer,
Toke Høiland-Jørgensen, netdev
> On Oct 13, 2021, at 12:33 AM, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> 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).
[...]
> emit(gen, BPF_ALU64_IMM(BPF_RSH, BPF_REG_7, 32));
> emit(gen, BPF_STX_MEM(BPF_W, BPF_REG_8, BPF_REG_7,
> sizeof(struct bpf_insn) + offsetof(struct bpf_insn, imm)));
> -log:
> + emit(gen, BPF_JMP_IMM(BPF_JSGE, BPF_REG_7, 0, 3));
> +clear_src_reg:
> + /* clear bpf_object__relocate_data's src_reg assignment, otherwise we get a verifier failure */
> +#if defined(__LITTLE_ENDIAN_BITFIELD)
> + reg_mask = 0x0f; /* src_reg,dst_reg,... */
> +#elif defined(__BIG_ENDIAN_BITFIELD)
> + reg_mask = 0xf0; /* dst_reg,src_reg,... */
> +#else
> +#error "Unsupported bit endianness, cannot proceed"
> +#endif
nit: Please put the "if defined" block in an inline helper.
> + emit(gen, BPF_LDX_MEM(BPF_B, BPF_REG_9, BPF_REG_8, offsetofend(struct bpf_insn, code)));
> + emit(gen, BPF_ALU32_IMM(BPF_AND, BPF_REG_9, reg_mask));
> + emit(gen, BPF_STX_MEM(BPF_B, BPF_REG_8, BPF_REG_9, offsetofend(struct bpf_insn, code)));
> +
> if (!gen->log_level)
> return;
> emit(gen, BPF_LDX_MEM(BPF_W, BPF_REG_7, BPF_REG_8,
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH bpf-next v2 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2
2021-10-13 7:33 [PATCH bpf-next v2 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
` (2 preceding siblings ...)
2021-10-13 7:33 ` [PATCH bpf-next v2 3/8] libbpf: Add weak " Kumar Kartikeya Dwivedi
@ 2021-10-13 7:33 ` Kumar Kartikeya Dwivedi
2021-10-14 16:55 ` Song Liu
2021-10-13 7:33 ` [PATCH bpf-next v2 5/8] libbpf: Use O_CLOEXEC uniformly when opening fds Kumar Kartikeya Dwivedi
` (3 subsequent siblings)
7 siblings, 1 reply; 14+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-13 7:33 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
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
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 | 34 ++++++++++++++++++++++++++++++-
6 files changed, 90 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 2ff7c9487634..00ddc430e4b9 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;
@@ -9312,10 +9312,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",
@@ -9406,10 +9406,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",
@@ -9601,10 +9601,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);
@@ -9733,8 +9733,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",
@@ -10253,8 +10253,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",
@@ -10380,7 +10380,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 f7fd3944d46d..9ae046d3b1c3 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..02b645d4e678 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,38 @@ 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 (fd >= 3)
+ close(fd);
+ else
+ 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 +143,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.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH bpf-next v2 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2
2021-10-13 7:33 ` [PATCH bpf-next v2 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2 Kumar Kartikeya Dwivedi
@ 2021-10-14 16:55 ` Song Liu
0 siblings, 0 replies; 14+ messages in thread
From: Song Liu @ 2021-10-14 16:55 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin Lau, Yonghong Song, Jesper Dangaard Brouer,
Toke Høiland-Jørgensen, netdev
> On Oct 13, 2021, at 12:33 AM, 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].
Acked-by: Song Liu <songliubraving@fb.com>
With on nitpick below.
[...]
> +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 (fd >= 3)
> + close(fd);
nit: Maybe likely(fd >= 3) and break here?
> + else
> + 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 +143,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.0
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH bpf-next v2 5/8] libbpf: Use O_CLOEXEC uniformly when opening fds
2021-10-13 7:33 [PATCH bpf-next v2 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
` (3 preceding siblings ...)
2021-10-13 7:33 ` [PATCH bpf-next v2 4/8] libbpf: Ensure that BPF syscall fds are never 0, 1, or 2 Kumar Kartikeya Dwivedi
@ 2021-10-13 7:33 ` Kumar Kartikeya Dwivedi
2021-10-13 7:33 ` [PATCH bpf-next v2 6/8] selftests/bpf: Add weak/typeless ksym test for light skeleton Kumar Kartikeya Dwivedi
` (2 subsequent siblings)
7 siblings, 0 replies; 14+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-13 7:33 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 60fbd1c6d466..06a7a4e52134 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 00ddc430e4b9..82f807ccbb2a 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;
@@ -9331,7 +9331,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;
@@ -10976,7 +10976,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.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH bpf-next v2 6/8] selftests/bpf: Add weak/typeless ksym test for light skeleton
2021-10-13 7:33 [PATCH bpf-next v2 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
` (4 preceding siblings ...)
2021-10-13 7:33 ` [PATCH bpf-next v2 5/8] libbpf: Use O_CLOEXEC uniformly when opening fds Kumar Kartikeya Dwivedi
@ 2021-10-13 7:33 ` Kumar Kartikeya Dwivedi
2021-10-13 7:33 ` [PATCH bpf-next v2 7/8] selftests/bpf: Fix fd cleanup in sk_lookup test Kumar Kartikeya Dwivedi
2021-10-13 7:33 ` [PATCH bpf-next v2 8/8] selftests/bpf: Fix memory leak in test_ima Kumar Kartikeya Dwivedi
7 siblings, 0 replies; 14+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-13 7:33 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.
Create a file for testing libbpf skeleton as well, so that both
gen_loader and libbpf get tested.
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.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH bpf-next v2 7/8] selftests/bpf: Fix fd cleanup in sk_lookup test
2021-10-13 7:33 [PATCH bpf-next v2 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
` (5 preceding siblings ...)
2021-10-13 7:33 ` [PATCH bpf-next v2 6/8] selftests/bpf: Add weak/typeless ksym test for light skeleton Kumar Kartikeya Dwivedi
@ 2021-10-13 7:33 ` Kumar Kartikeya Dwivedi
2021-10-13 7:33 ` [PATCH bpf-next v2 8/8] selftests/bpf: Fix memory leak in test_ima Kumar Kartikeya Dwivedi
7 siblings, 0 replies; 14+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-13 7:33 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.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH bpf-next v2 8/8] selftests/bpf: Fix memory leak in test_ima
2021-10-13 7:33 [PATCH bpf-next v2 0/8] Typeless/weak ksym for gen_loader + misc fixups Kumar Kartikeya Dwivedi
` (6 preceding siblings ...)
2021-10-13 7:33 ` [PATCH bpf-next v2 7/8] selftests/bpf: Fix fd cleanup in sk_lookup test Kumar Kartikeya Dwivedi
@ 2021-10-13 7:33 ` Kumar Kartikeya Dwivedi
7 siblings, 0 replies; 14+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2021-10-13 7:33 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.0
^ permalink raw reply related [flat|nested] 14+ messages in thread