bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel
@ 2021-11-12  5:02 Alexei Starovoitov
  2021-11-12  5:02 ` [PATCH v2 bpf-next 01/12] libbpf: s/btf__type_by_id/btf_type_by_id/ Alexei Starovoitov
                   ` (12 more replies)
  0 siblings, 13 replies; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

v1->v2:
. Refactor uapi to pass 'struct bpf_core_relo' from LLVM into libbpf and further
into the kernel instead of bpf_core_apply_relo() bpf helper. Because of this
change the CO-RE algorithm has an ability to log error and debug events through
the standard bpf verifer log mechanism which was not possible with helper
approach.
. #define RELO_CORE macro was removed and replaced with btf_member_bit_offset() patch.

This set introduces CO-RE support in the kernel.
There are several reasons to add such support:
1. It's a step toward signed BPF programs.
2. It allows golang like languages that struggle to adopt libbpf
   to take advantage of CO-RE powers.
3. Currently the field accessed by 'ldx [R1 + 10]' insn is recognized
   by the verifier purely based on +10 offset. If R1 points to a union
   the verifier picks one of the fields at this offset.
   With CO-RE the kernel can disambiguate the field access.

Alexei Starovoitov (12):
  libbpf: s/btf__type_by_id/btf_type_by_id/.
  bpf: Rename btf_member accessors.
  bpf: Prepare relo_core.c for kernel duty.
  bpf: Define enum bpf_core_relo_kind as uapi.
  bpf: Pass a set of bpf_core_relo-s to prog_load command.
  bpf: Add bpf_core_add_cands() and wire it into
    bpf_core_apply_relo_insn().
  libbpf: Use CO-RE in the kernel in light skeleton.
  libbpf: Support init of inner maps in light skeleton.
  selftests/bpf: Convert kfunc test with CO-RE to lskel.
  selftests/bpf: Improve inner_map test coverage.
  selftests/bpf: Convert map_ptr_kern test to use light skeleton.
  selftests/bpf: Additional test for CO-RE in the kernel.

 include/linux/bpf.h                           |   3 +
 include/linux/btf.h                           |  90 ++++++++-
 include/uapi/linux/bpf.h                      |  78 +++++++-
 kernel/bpf/Makefile                           |   4 +
 kernel/bpf/bpf_struct_ops.c                   |   6 +-
 kernel/bpf/btf.c                              | 187 +++++++++++++++++-
 kernel/bpf/syscall.c                          |   2 +-
 kernel/bpf/verifier.c                         |  71 +++++++
 net/ipv4/bpf_tcp_ca.c                         |   6 +-
 tools/include/uapi/linux/bpf.h                |  78 +++++++-
 tools/lib/bpf/bpf_gen_internal.h              |   4 +
 tools/lib/bpf/btf.c                           |   2 +-
 tools/lib/bpf/gen_loader.c                    |  68 ++++++-
 tools/lib/bpf/libbpf.c                        | 112 ++++++++---
 tools/lib/bpf/libbpf_internal.h               |   2 +-
 tools/lib/bpf/relo_core.c                     | 171 ++++++++++------
 tools/lib/bpf/relo_core.h                     |  71 -------
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../selftests/bpf/prog_tests/core_kern.c      |  21 ++
 .../selftests/bpf/prog_tests/kfunc_call.c     |  10 +-
 .../selftests/bpf/prog_tests/map_ptr.c        |  16 +-
 tools/testing/selftests/bpf/progs/core_kern.c |  60 ++++++
 .../selftests/bpf/progs/map_ptr_kern.c        |  16 +-
 23 files changed, 877 insertions(+), 204 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/core_kern.c
 create mode 100644 tools/testing/selftests/bpf/progs/core_kern.c

-- 
2.30.2


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 01/12] libbpf: s/btf__type_by_id/btf_type_by_id/.
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  0:32   ` Andrii Nakryiko
  2021-11-12  5:02 ` [PATCH v2 bpf-next 02/12] bpf: Rename btf_member accessors Alexei Starovoitov
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

To prepare relo_core.c to be compiled in the kernel and the user space
replace btf__type_by_id with btf_type_by_id.

In libbpf btf__type_by_id and btf_type_by_id have different behavior.

bpf_core_apply_relo_insn() needs behavior of uapi btf__type_by_id
vs internal btf_type_by_id, but type_id range check is already done
in bpf_core_apply_relo(), so it's safe to replace it everywhere.
The kernel btf_type_by_id() does the check anyway. It doesn't hurt.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/lib/bpf/btf.c             |  2 +-
 tools/lib/bpf/libbpf_internal.h |  2 +-
 tools/lib/bpf/relo_core.c       | 16 ++++++++--------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index fadf089ae8fe..aa0e0bbde697 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -454,7 +454,7 @@ const struct btf *btf__base_btf(const struct btf *btf)
 }
 
 /* internal helper returning non-const pointer to a type */
-struct btf_type *btf_type_by_id(struct btf *btf, __u32 type_id)
+struct btf_type *btf_type_by_id(const struct btf *btf, __u32 type_id)
 {
 	if (type_id == 0)
 		return &btf_void;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index f7ac349650a1..1e1250e1dfa3 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -172,7 +172,7 @@ static inline void *libbpf_reallocarray(void *ptr, size_t nmemb, size_t size)
 struct btf;
 struct btf_type;
 
-struct btf_type *btf_type_by_id(struct btf *btf, __u32 type_id);
+struct btf_type *btf_type_by_id(const struct btf *btf, __u32 type_id);
 const char *btf_kind_str(const struct btf_type *t);
 const struct btf_type *skip_mods_and_typedefs(const struct btf *btf, __u32 id, __u32 *res_id);
 
diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
index b5b8956a1be8..dc9f775cf10b 100644
--- a/tools/lib/bpf/relo_core.c
+++ b/tools/lib/bpf/relo_core.c
@@ -51,7 +51,7 @@ static bool is_flex_arr(const struct btf *btf,
 		return false;
 
 	/* has to be the last member of enclosing struct */
-	t = btf__type_by_id(btf, acc->type_id);
+	t = btf_type_by_id(btf, acc->type_id);
 	return acc->idx == btf_vlen(t) - 1;
 }
 
@@ -388,7 +388,7 @@ static int bpf_core_match_member(const struct btf *local_btf,
 		return 0;
 
 	local_id = local_acc->type_id;
-	local_type = btf__type_by_id(local_btf, local_id);
+	local_type = btf_type_by_id(local_btf, local_id);
 	local_member = btf_members(local_type) + local_acc->idx;
 	local_name = btf__name_by_offset(local_btf, local_member->name_off);
 
@@ -580,7 +580,7 @@ static int bpf_core_calc_field_relo(const char *prog_name,
 		return -EUCLEAN; /* request instruction poisoning */
 
 	acc = &spec->spec[spec->len - 1];
-	t = btf__type_by_id(spec->btf, acc->type_id);
+	t = btf_type_by_id(spec->btf, acc->type_id);
 
 	/* a[n] accessor needs special handling */
 	if (!acc->name) {
@@ -729,7 +729,7 @@ static int bpf_core_calc_enumval_relo(const struct bpf_core_relo *relo,
 	case BPF_ENUMVAL_VALUE:
 		if (!spec)
 			return -EUCLEAN; /* request instruction poisoning */
-		t = btf__type_by_id(spec->btf, spec->spec[0].type_id);
+		t = btf_type_by_id(spec->btf, spec->spec[0].type_id);
 		e = btf_enum(t) + spec->spec[0].idx;
 		*val = e->val;
 		break;
@@ -805,8 +805,8 @@ static int bpf_core_calc_relo(const char *prog_name,
 		if (res->orig_sz != res->new_sz) {
 			const struct btf_type *orig_t, *new_t;
 
-			orig_t = btf__type_by_id(local_spec->btf, res->orig_type_id);
-			new_t = btf__type_by_id(targ_spec->btf, res->new_type_id);
+			orig_t = btf_type_by_id(local_spec->btf, res->orig_type_id);
+			new_t = btf_type_by_id(targ_spec->btf, res->new_type_id);
 
 			/* There are two use cases in which it's safe to
 			 * adjust load/store's mem size:
@@ -1054,7 +1054,7 @@ static void bpf_core_dump_spec(int level, const struct bpf_core_spec *spec)
 	int i;
 
 	type_id = spec->root_type_id;
-	t = btf__type_by_id(spec->btf, type_id);
+	t = btf_type_by_id(spec->btf, type_id);
 	s = btf__name_by_offset(spec->btf, t->name_off);
 
 	libbpf_print(level, "[%u] %s %s", type_id, btf_kind_str(t), str_is_empty(s) ? "<anon>" : s);
@@ -1158,7 +1158,7 @@ int bpf_core_apply_relo_insn(const char *prog_name, struct bpf_insn *insn,
 	int i, j, err;
 
 	local_id = relo->type_id;
-	local_type = btf__type_by_id(local_btf, local_id);
+	local_type = btf_type_by_id(local_btf, local_id);
 	if (!local_type)
 		return -EINVAL;
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 02/12] bpf: Rename btf_member accessors.
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
  2021-11-12  5:02 ` [PATCH v2 bpf-next 01/12] libbpf: s/btf__type_by_id/btf_type_by_id/ Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  0:38   ` Andrii Nakryiko
  2021-11-12  5:02 ` [PATCH v2 bpf-next 03/12] bpf: Prepare relo_core.c for kernel duty Alexei Starovoitov
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

Rename btf_member_bit_offset() and btf_member_bitfield_size() to
avoid conflicts with similarly named helpers in libbpf's btf.h.
Rename the kernel helpers, since libbpf helpers are part of uapi.

Suggested-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/btf.h         |  8 ++++----
 kernel/bpf/bpf_struct_ops.c |  6 +++---
 kernel/bpf/btf.c            | 18 +++++++++---------
 net/ipv4/bpf_tcp_ca.c       |  6 +++---
 4 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 203eef993d76..956f70388f69 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -194,15 +194,15 @@ static inline bool btf_type_kflag(const struct btf_type *t)
 	return BTF_INFO_KFLAG(t->info);
 }
 
-static inline u32 btf_member_bit_offset(const struct btf_type *struct_type,
-					const struct btf_member *member)
+static inline u32 __btf_member_bit_offset(const struct btf_type *struct_type,
+					  const struct btf_member *member)
 {
 	return btf_type_kflag(struct_type) ? BTF_MEMBER_BIT_OFFSET(member->offset)
 					   : member->offset;
 }
 
-static inline u32 btf_member_bitfield_size(const struct btf_type *struct_type,
-					   const struct btf_member *member)
+static inline u32 __btf_member_bitfield_size(const struct btf_type *struct_type,
+					     const struct btf_member *member)
 {
 	return btf_type_kflag(struct_type) ? BTF_MEMBER_BITFIELD_SIZE(member->offset)
 					   : 0;
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 8ecfe4752769..21069dbe9138 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -165,7 +165,7 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
 				break;
 			}
 
-			if (btf_member_bitfield_size(t, member)) {
+			if (__btf_member_bitfield_size(t, member)) {
 				pr_warn("bit field member %s in struct %s is not supported\n",
 					mname, st_ops->name);
 				break;
@@ -296,7 +296,7 @@ static int check_zero_holes(const struct btf_type *t, void *data)
 	const struct btf_type *mtype;
 
 	for_each_member(i, t, member) {
-		moff = btf_member_bit_offset(t, member) / 8;
+		moff = __btf_member_bit_offset(t, member) / 8;
 		if (moff > prev_mend &&
 		    memchr_inv(data + prev_mend, 0, moff - prev_mend))
 			return -EINVAL;
@@ -387,7 +387,7 @@ static int bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 		struct bpf_prog *prog;
 		u32 moff;
 
-		moff = btf_member_bit_offset(t, member) / 8;
+		moff = __btf_member_bit_offset(t, member) / 8;
 		ptype = btf_type_resolve_ptr(btf_vmlinux, member->type, NULL);
 		if (ptype == module_type) {
 			if (*(void **)(udata + moff))
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 1dd9ba82da1e..4373f9eb3106 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -2969,7 +2969,7 @@ static s32 btf_struct_check_meta(struct btf_verifier_env *env,
 			return -EINVAL;
 		}
 
-		offset = btf_member_bit_offset(t, member);
+		offset = __btf_member_bit_offset(t, member);
 		if (is_union && offset) {
 			btf_verifier_log_member(env, t, member,
 						"Invalid member bits_offset");
@@ -3094,7 +3094,7 @@ static int btf_find_struct_field(const struct btf *btf, const struct btf_type *t
 		if (off != -ENOENT)
 			/* only one such field is allowed */
 			return -E2BIG;
-		off = btf_member_bit_offset(t, member);
+		off = __btf_member_bit_offset(t, member);
 		if (off % 8)
 			/* valid C code cannot generate such BTF */
 			return -EINVAL;
@@ -3184,8 +3184,8 @@ static void __btf_struct_show(const struct btf *btf, const struct btf_type *t,
 
 		btf_show_start_member(show, member);
 
-		member_offset = btf_member_bit_offset(t, member);
-		bitfield_size = btf_member_bitfield_size(t, member);
+		member_offset = __btf_member_bit_offset(t, member);
+		bitfield_size = __btf_member_bitfield_size(t, member);
 		bytes_offset = BITS_ROUNDDOWN_BYTES(member_offset);
 		bits8_offset = BITS_PER_BYTE_MASKED(member_offset);
 		if (bitfield_size) {
@@ -5060,7 +5060,7 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 		if (array_elem->nelems != 0)
 			goto error;
 
-		moff = btf_member_bit_offset(t, member) / 8;
+		moff = __btf_member_bit_offset(t, member) / 8;
 		if (off < moff)
 			goto error;
 
@@ -5083,14 +5083,14 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 
 	for_each_member(i, t, member) {
 		/* offset of the field in bytes */
-		moff = btf_member_bit_offset(t, member) / 8;
+		moff = __btf_member_bit_offset(t, member) / 8;
 		if (off + size <= moff)
 			/* won't find anything, field is already too far */
 			break;
 
-		if (btf_member_bitfield_size(t, member)) {
-			u32 end_bit = btf_member_bit_offset(t, member) +
-				btf_member_bitfield_size(t, member);
+		if (__btf_member_bitfield_size(t, member)) {
+			u32 end_bit = __btf_member_bit_offset(t, member) +
+				__btf_member_bitfield_size(t, member);
 
 			/* off <= moff instead of off == moff because clang
 			 * does not generate a BTF member for anonymous
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index 2cf02b4d77fb..67466dbff152 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -169,7 +169,7 @@ static u32 prog_ops_moff(const struct bpf_prog *prog)
 	t = bpf_tcp_congestion_ops.type;
 	m = &btf_type_member(t)[midx];
 
-	return btf_member_bit_offset(t, m) / 8;
+	return __btf_member_bit_offset(t, m) / 8;
 }
 
 static const struct bpf_func_proto *
@@ -244,7 +244,7 @@ static int bpf_tcp_ca_init_member(const struct btf_type *t,
 	utcp_ca = (const struct tcp_congestion_ops *)udata;
 	tcp_ca = (struct tcp_congestion_ops *)kdata;
 
-	moff = btf_member_bit_offset(t, member) / 8;
+	moff = __btf_member_bit_offset(t, member) / 8;
 	switch (moff) {
 	case offsetof(struct tcp_congestion_ops, flags):
 		if (utcp_ca->flags & ~TCP_CONG_MASK)
@@ -274,7 +274,7 @@ static int bpf_tcp_ca_init_member(const struct btf_type *t,
 static int bpf_tcp_ca_check_member(const struct btf_type *t,
 				   const struct btf_member *member)
 {
-	if (is_unsupported(btf_member_bit_offset(t, member) / 8))
+	if (is_unsupported(__btf_member_bit_offset(t, member) / 8))
 		return -ENOTSUPP;
 	return 0;
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 03/12] bpf: Prepare relo_core.c for kernel duty.
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
  2021-11-12  5:02 ` [PATCH v2 bpf-next 01/12] libbpf: s/btf__type_by_id/btf_type_by_id/ Alexei Starovoitov
  2021-11-12  5:02 ` [PATCH v2 bpf-next 02/12] bpf: Rename btf_member accessors Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  0:58   ` Andrii Nakryiko
  2021-11-12  5:02 ` [PATCH v2 bpf-next 04/12] bpf: Define enum bpf_core_relo_kind as uapi Alexei Starovoitov
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

Make relo_core.c to be compiled with kernel and with libbpf.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/btf.h       | 82 +++++++++++++++++++++++++++++++++++++++
 kernel/bpf/Makefile       |  4 ++
 kernel/bpf/btf.c          | 26 +++++++++++++
 tools/lib/bpf/relo_core.c | 71 ++++++++++++++++++++++++++++-----
 4 files changed, 174 insertions(+), 9 deletions(-)

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 956f70388f69..9587c6dfa3f7 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -144,6 +144,53 @@ static inline bool btf_type_is_enum(const struct btf_type *t)
 	return BTF_INFO_KIND(t->info) == BTF_KIND_ENUM;
 }
 
+static inline bool str_is_empty(const char *s)
+{
+	return !s || !s[0];
+}
+
+static inline u16 btf_kind(const struct btf_type *t)
+{
+	return BTF_INFO_KIND(t->info);
+}
+
+static inline bool btf_is_enum(const struct btf_type *t)
+{
+	return btf_kind(t) == BTF_KIND_ENUM;
+}
+
+static inline bool btf_is_composite(const struct btf_type *t)
+{
+	u16 kind = btf_kind(t);
+
+	return kind == BTF_KIND_STRUCT || kind == BTF_KIND_UNION;
+}
+
+static inline bool btf_is_array(const struct btf_type *t)
+{
+	return btf_kind(t) == BTF_KIND_ARRAY;
+}
+
+static inline bool btf_is_int(const struct btf_type *t)
+{
+	return btf_kind(t) == BTF_KIND_INT;
+}
+
+static inline bool btf_is_ptr(const struct btf_type *t)
+{
+	return btf_kind(t) == BTF_KIND_PTR;
+}
+
+static inline u8 btf_int_offset(const struct btf_type *t)
+{
+	return BTF_INT_OFFSET(*(u32 *)(t + 1));
+}
+
+static inline u8 btf_int_encoding(const struct btf_type *t)
+{
+	return BTF_INT_ENCODING(*(u32 *)(t + 1));
+}
+
 static inline bool btf_type_is_scalar(const struct btf_type *t)
 {
 	return btf_type_is_int(t) || btf_type_is_enum(t);
@@ -184,6 +231,11 @@ static inline u16 btf_type_vlen(const struct btf_type *t)
 	return BTF_INFO_VLEN(t->info);
 }
 
+static inline u16 btf_vlen(const struct btf_type *t)
+{
+	return btf_type_vlen(t);
+}
+
 static inline u16 btf_func_linkage(const struct btf_type *t)
 {
 	return BTF_INFO_VLEN(t->info);
@@ -208,11 +260,41 @@ static inline u32 __btf_member_bitfield_size(const struct btf_type *struct_type,
 					   : 0;
 }
 
+static inline struct btf_member *btf_members(const struct btf_type *t)
+{
+	return (struct btf_member *)(t + 1);
+}
+
+static inline u32 btf_member_bit_offset(const struct btf_type *t, u32 member_idx)
+{
+	const struct btf_member *m = btf_members(t) + member_idx;
+
+	return __btf_member_bit_offset(t, m);
+}
+
+static inline u32 btf_member_bitfield_size(const struct btf_type *t, u32 member_idx)
+{
+	const struct btf_member *m = btf_members(t) + member_idx;
+
+	return __btf_member_bitfield_size(t, m);
+}
+
 static inline const struct btf_member *btf_type_member(const struct btf_type *t)
 {
 	return (const struct btf_member *)(t + 1);
 }
 
+
+static inline struct btf_array *btf_array(const struct btf_type *t)
+{
+	return (struct btf_array *)(t + 1);
+}
+
+static inline struct btf_enum *btf_enum(const struct btf_type *t)
+{
+	return (struct btf_enum *)(t + 1);
+}
+
 static inline const struct btf_var_secinfo *btf_type_var_secinfo(
 		const struct btf_type *t)
 {
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index cf6ca339f3cd..c1a9be6a4b9f 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -36,3 +36,7 @@ obj-$(CONFIG_BPF_SYSCALL) += bpf_struct_ops.o
 obj-${CONFIG_BPF_LSM} += bpf_lsm.o
 endif
 obj-$(CONFIG_BPF_PRELOAD) += preload/
+
+obj-$(CONFIG_BPF_SYSCALL) += relo_core.o
+$(obj)/relo_core.o: $(srctree)/tools/lib/bpf/relo_core.c FORCE
+	$(call if_changed_rule,cc_o_c)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 4373f9eb3106..e3cce46fa02f 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6413,3 +6413,29 @@ bool bpf_check_mod_kfunc_call(struct kfunc_btf_id_list *klist, u32 kfunc_id,
 
 DEFINE_KFUNC_BTF_ID_LIST(bpf_tcp_ca_kfunc_list);
 DEFINE_KFUNC_BTF_ID_LIST(prog_test_kfunc_list);
+
+int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
+			      const struct btf *targ_btf, __u32 targ_id)
+{
+	return -EOPNOTSUPP;
+}
+
+static bool bpf_core_is_flavor_sep(const char *s)
+{
+	/* check X___Y name pattern, where X and Y are not underscores */
+	return s[0] != '_' &&				      /* X */
+	       s[1] == '_' && s[2] == '_' && s[3] == '_' &&   /* ___ */
+	       s[4] != '_';				      /* Y */
+}
+
+size_t bpf_core_essential_name_len(const char *name)
+{
+	size_t n = strlen(name);
+	int i;
+
+	for (i = n - 5; i >= 0; i--) {
+		if (bpf_core_is_flavor_sep(name + i))
+			return i + 1;
+	}
+	return n;
+}
diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
index dc9f775cf10b..b80180357b31 100644
--- a/tools/lib/bpf/relo_core.c
+++ b/tools/lib/bpf/relo_core.c
@@ -1,6 +1,58 @@
 // SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
 /* Copyright (c) 2019 Facebook */
 
+#ifdef __KERNEL__
+#include <linux/bpf.h>
+#include <linux/btf.h>
+#include <linux/string.h>
+#include <linux/bpf_verifier.h>
+#include "relo_core.h"
+
+static const char *btf_kind_str(const struct btf_type *t)
+{
+	return btf_type_str(t);
+}
+
+static bool is_ldimm64_insn(struct bpf_insn *insn)
+{
+	return insn->code == (BPF_LD | BPF_IMM | BPF_DW);
+}
+
+static const struct btf_type *
+skip_mods_and_typedefs(const struct btf *btf, u32 id, u32 *res_id)
+{
+	return btf_type_skip_modifiers(btf, id, res_id);
+}
+
+static const char *btf__name_by_offset(const struct btf *btf, u32 offset)
+{
+	return btf_name_by_offset(btf, offset);
+}
+
+static s64 btf__resolve_size(const struct btf *btf, u32 type_id)
+{
+	const struct btf_type *t;
+	int size;
+
+	t = btf_type_by_id(btf, type_id);
+	t = btf_resolve_size(btf, t, &size);
+	if (IS_ERR(t))
+		return PTR_ERR(t);
+	return size;
+}
+enum libbpf_print_level {
+	LIBBPF_WARN,
+	LIBBPF_INFO,
+	LIBBPF_DEBUG,
+};
+#undef pr_warn
+#undef pr_info
+#undef pr_debug
+#define pr_warn(fmt, log, ...)	bpf_log((void *)log, fmt, "", ##__VA_ARGS__)
+#define pr_info(fmt, log, ...)	bpf_log((void *)log, fmt, "", ##__VA_ARGS__)
+#define pr_debug(fmt, log, ...)	bpf_log((void *)log, fmt, "", ##__VA_ARGS__)
+#define libbpf_print(level, fmt, ...)	bpf_log((void *)prog_name, fmt, ##__VA_ARGS__)
+#else
 #include <stdio.h>
 #include <string.h>
 #include <errno.h>
@@ -12,8 +64,9 @@
 #include "btf.h"
 #include "str_error.h"
 #include "libbpf_internal.h"
+#endif
 
-#define BPF_CORE_SPEC_MAX_LEN 64
+#define BPF_CORE_SPEC_MAX_LEN 32
 
 /* represents BPF CO-RE field or array element accessor */
 struct bpf_core_accessor {
@@ -272,8 +325,8 @@ static int bpf_core_parse_spec(const struct btf *btf,
 				return sz;
 			spec->bit_offset += access_idx * sz * 8;
 		} else {
-			pr_warn("relo for [%u] %s (at idx %d) captures type [%d] of unexpected kind %s\n",
-				type_id, spec_str, i, id, btf_kind_str(t));
+/*			pr_warn("relo for [%u] %s (at idx %d) captures type [%d] of unexpected kind %s\n",
+				type_id, spec_str, i, id, btf_kind_str(t));*/
 			return -EINVAL;
 		}
 	}
@@ -346,8 +399,8 @@ static int bpf_core_fields_are_compat(const struct btf *local_btf,
 		targ_id = btf_array(targ_type)->type;
 		goto recur;
 	default:
-		pr_warn("unexpected kind %d relocated, local [%d], target [%d]\n",
-			btf_kind(local_type), local_id, targ_id);
+/*		pr_warn("unexpected kind %d relocated, local [%d], target [%d]\n",
+			btf_kind(local_type), local_id, targ_id);*/
 		return 0;
 	}
 }
@@ -1045,7 +1098,7 @@ static int bpf_core_patch_insn(const char *prog_name, struct bpf_insn *insn,
  * [<type-id>] (<type-name>) + <raw-spec> => <offset>@<spec>,
  * where <spec> is a C-syntax view of recorded field access, e.g.: x.a[3].b
  */
-static void bpf_core_dump_spec(int level, const struct bpf_core_spec *spec)
+static void bpf_core_dump_spec(const char *prog_name, int level, const struct bpf_core_spec *spec)
 {
 	const struct btf_type *t;
 	const struct btf_enum *e;
@@ -1181,7 +1234,7 @@ int bpf_core_apply_relo_insn(const char *prog_name, struct bpf_insn *insn,
 
 	pr_debug("prog '%s': relo #%d: kind <%s> (%d), spec is ", prog_name,
 		 relo_idx, core_relo_kind_str(relo->kind), relo->kind);
-	bpf_core_dump_spec(LIBBPF_DEBUG, &local_spec);
+	bpf_core_dump_spec(prog_name, LIBBPF_DEBUG, &local_spec);
 	libbpf_print(LIBBPF_DEBUG, "\n");
 
 	/* TYPE_ID_LOCAL relo is special and doesn't need candidate search */
@@ -1207,14 +1260,14 @@ int bpf_core_apply_relo_insn(const char *prog_name, struct bpf_insn *insn,
 		if (err < 0) {
 			pr_warn("prog '%s': relo #%d: error matching candidate #%d ",
 				prog_name, relo_idx, i);
-			bpf_core_dump_spec(LIBBPF_WARN, &cand_spec);
+			bpf_core_dump_spec(prog_name, LIBBPF_WARN, &cand_spec);
 			libbpf_print(LIBBPF_WARN, ": %d\n", err);
 			return err;
 		}
 
 		pr_debug("prog '%s': relo #%d: %s candidate #%d ", prog_name,
 			 relo_idx, err == 0 ? "non-matching" : "matching", i);
-		bpf_core_dump_spec(LIBBPF_DEBUG, &cand_spec);
+		bpf_core_dump_spec(prog_name, LIBBPF_DEBUG, &cand_spec);
 		libbpf_print(LIBBPF_DEBUG, "\n");
 
 		if (err == 0)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 04/12] bpf: Define enum bpf_core_relo_kind as uapi.
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
                   ` (2 preceding siblings ...)
  2021-11-12  5:02 ` [PATCH v2 bpf-next 03/12] bpf: Prepare relo_core.c for kernel duty Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  1:04   ` Andrii Nakryiko
  2021-11-12  5:02 ` [PATCH v2 bpf-next 05/12] bpf: Pass a set of bpf_core_relo-s to prog_load command Alexei Starovoitov
                   ` (8 subsequent siblings)
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

enum bpf_core_relo_kind is generated by llvm and processed by libbpf.
It's a de-facto uapi.
With CO-RE in the kernel the bpf_core_relo_kind values become uapi de-jure.
Also rename them with BPF_CORE_ prefix to distinguish from conflicting names in
bpf_core_read.h. The enums bpf_field_info_kind, bpf_type_id_kind,
bpf_type_info_kind, bpf_enum_value_kind are passing different values from bpf
program into llvm.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
 include/uapi/linux/bpf.h       | 19 ++++++++
 tools/include/uapi/linux/bpf.h | 19 ++++++++
 tools/lib/bpf/libbpf.c         |  2 +-
 tools/lib/bpf/relo_core.c      | 84 +++++++++++++++++-----------------
 tools/lib/bpf/relo_core.h      | 18 --------
 5 files changed, 81 insertions(+), 61 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6297eafdc40f..3c37bfe92359 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -6349,4 +6349,23 @@ enum {
 	BTF_F_ZERO	=	(1ULL << 3),
 };
 
+/* bpf_core_relo_kind encodes which aspect of captured field/type/enum value
+ * has to be adjusted by relocations. It is emitted by llvm and passed to
+ * libbpf and later to the kernel.
+ */
+enum bpf_core_relo_kind {
+	BPF_CORE_FIELD_BYTE_OFFSET = 0,      /* field byte offset */
+	BPF_CORE_FIELD_BYTE_SIZE = 1,        /* field size in bytes */
+	BPF_CORE_FIELD_EXISTS = 2,           /* field existence in target kernel */
+	BPF_CORE_FIELD_SIGNED = 3,           /* field signedness (0 - unsigned, 1 - signed) */
+	BPF_CORE_FIELD_LSHIFT_U64 = 4,       /* bitfield-specific left bitshift */
+	BPF_CORE_FIELD_RSHIFT_U64 = 5,       /* bitfield-specific right bitshift */
+	BPF_CORE_TYPE_ID_LOCAL = 6,          /* type ID in local BPF object */
+	BPF_CORE_TYPE_ID_TARGET = 7,         /* type ID in target kernel */
+	BPF_CORE_TYPE_EXISTS = 8,            /* type existence in target kernel */
+	BPF_CORE_TYPE_SIZE = 9,              /* type size in bytes */
+	BPF_CORE_ENUMVAL_EXISTS = 10,        /* enum value existence in target kernel */
+	BPF_CORE_ENUMVAL_VALUE = 11,         /* enum value integer value */
+};
+
 #endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 6297eafdc40f..3c37bfe92359 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -6349,4 +6349,23 @@ enum {
 	BTF_F_ZERO	=	(1ULL << 3),
 };
 
+/* bpf_core_relo_kind encodes which aspect of captured field/type/enum value
+ * has to be adjusted by relocations. It is emitted by llvm and passed to
+ * libbpf and later to the kernel.
+ */
+enum bpf_core_relo_kind {
+	BPF_CORE_FIELD_BYTE_OFFSET = 0,      /* field byte offset */
+	BPF_CORE_FIELD_BYTE_SIZE = 1,        /* field size in bytes */
+	BPF_CORE_FIELD_EXISTS = 2,           /* field existence in target kernel */
+	BPF_CORE_FIELD_SIGNED = 3,           /* field signedness (0 - unsigned, 1 - signed) */
+	BPF_CORE_FIELD_LSHIFT_U64 = 4,       /* bitfield-specific left bitshift */
+	BPF_CORE_FIELD_RSHIFT_U64 = 5,       /* bitfield-specific right bitshift */
+	BPF_CORE_TYPE_ID_LOCAL = 6,          /* type ID in local BPF object */
+	BPF_CORE_TYPE_ID_TARGET = 7,         /* type ID in target kernel */
+	BPF_CORE_TYPE_EXISTS = 8,            /* type existence in target kernel */
+	BPF_CORE_TYPE_SIZE = 9,              /* type size in bytes */
+	BPF_CORE_ENUMVAL_EXISTS = 10,        /* enum value existence in target kernel */
+	BPF_CORE_ENUMVAL_VALUE = 11,         /* enum value integer value */
+};
+
 #endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index de7e09a6b5ec..e41867550ef2 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -5440,7 +5440,7 @@ static int bpf_core_apply_relo(struct bpf_program *prog,
 		return -ENOTSUP;
 	}
 
-	if (relo->kind != BPF_TYPE_ID_LOCAL &&
+	if (relo->kind != BPF_CORE_TYPE_ID_LOCAL &&
 	    !hashmap__find(cand_cache, type_key, (void **)&cands)) {
 		cands = bpf_core_find_cands(prog->obj, local_btf, local_id);
 		if (IS_ERR(cands)) {
diff --git a/tools/lib/bpf/relo_core.c b/tools/lib/bpf/relo_core.c
index b80180357b31..b5eb0a54a608 100644
--- a/tools/lib/bpf/relo_core.c
+++ b/tools/lib/bpf/relo_core.c
@@ -111,18 +111,18 @@ static bool is_flex_arr(const struct btf *btf,
 static const char *core_relo_kind_str(enum bpf_core_relo_kind kind)
 {
 	switch (kind) {
-	case BPF_FIELD_BYTE_OFFSET: return "byte_off";
-	case BPF_FIELD_BYTE_SIZE: return "byte_sz";
-	case BPF_FIELD_EXISTS: return "field_exists";
-	case BPF_FIELD_SIGNED: return "signed";
-	case BPF_FIELD_LSHIFT_U64: return "lshift_u64";
-	case BPF_FIELD_RSHIFT_U64: return "rshift_u64";
-	case BPF_TYPE_ID_LOCAL: return "local_type_id";
-	case BPF_TYPE_ID_TARGET: return "target_type_id";
-	case BPF_TYPE_EXISTS: return "type_exists";
-	case BPF_TYPE_SIZE: return "type_size";
-	case BPF_ENUMVAL_EXISTS: return "enumval_exists";
-	case BPF_ENUMVAL_VALUE: return "enumval_value";
+	case BPF_CORE_FIELD_BYTE_OFFSET: return "byte_off";
+	case BPF_CORE_FIELD_BYTE_SIZE: return "byte_sz";
+	case BPF_CORE_FIELD_EXISTS: return "field_exists";
+	case BPF_CORE_FIELD_SIGNED: return "signed";
+	case BPF_CORE_FIELD_LSHIFT_U64: return "lshift_u64";
+	case BPF_CORE_FIELD_RSHIFT_U64: return "rshift_u64";
+	case BPF_CORE_TYPE_ID_LOCAL: return "local_type_id";
+	case BPF_CORE_TYPE_ID_TARGET: return "target_type_id";
+	case BPF_CORE_TYPE_EXISTS: return "type_exists";
+	case BPF_CORE_TYPE_SIZE: return "type_size";
+	case BPF_CORE_ENUMVAL_EXISTS: return "enumval_exists";
+	case BPF_CORE_ENUMVAL_VALUE: return "enumval_value";
 	default: return "unknown";
 	}
 }
@@ -130,12 +130,12 @@ static const char *core_relo_kind_str(enum bpf_core_relo_kind kind)
 static bool core_relo_is_field_based(enum bpf_core_relo_kind kind)
 {
 	switch (kind) {
-	case BPF_FIELD_BYTE_OFFSET:
-	case BPF_FIELD_BYTE_SIZE:
-	case BPF_FIELD_EXISTS:
-	case BPF_FIELD_SIGNED:
-	case BPF_FIELD_LSHIFT_U64:
-	case BPF_FIELD_RSHIFT_U64:
+	case BPF_CORE_FIELD_BYTE_OFFSET:
+	case BPF_CORE_FIELD_BYTE_SIZE:
+	case BPF_CORE_FIELD_EXISTS:
+	case BPF_CORE_FIELD_SIGNED:
+	case BPF_CORE_FIELD_LSHIFT_U64:
+	case BPF_CORE_FIELD_RSHIFT_U64:
 		return true;
 	default:
 		return false;
@@ -145,10 +145,10 @@ static bool core_relo_is_field_based(enum bpf_core_relo_kind kind)
 static bool core_relo_is_type_based(enum bpf_core_relo_kind kind)
 {
 	switch (kind) {
-	case BPF_TYPE_ID_LOCAL:
-	case BPF_TYPE_ID_TARGET:
-	case BPF_TYPE_EXISTS:
-	case BPF_TYPE_SIZE:
+	case BPF_CORE_TYPE_ID_LOCAL:
+	case BPF_CORE_TYPE_ID_TARGET:
+	case BPF_CORE_TYPE_EXISTS:
+	case BPF_CORE_TYPE_SIZE:
 		return true;
 	default:
 		return false;
@@ -158,8 +158,8 @@ static bool core_relo_is_type_based(enum bpf_core_relo_kind kind)
 static bool core_relo_is_enumval_based(enum bpf_core_relo_kind kind)
 {
 	switch (kind) {
-	case BPF_ENUMVAL_EXISTS:
-	case BPF_ENUMVAL_VALUE:
+	case BPF_CORE_ENUMVAL_EXISTS:
+	case BPF_CORE_ENUMVAL_VALUE:
 		return true;
 	default:
 		return false;
@@ -624,7 +624,7 @@ static int bpf_core_calc_field_relo(const char *prog_name,
 
 	*field_sz = 0;
 
-	if (relo->kind == BPF_FIELD_EXISTS) {
+	if (relo->kind == BPF_CORE_FIELD_EXISTS) {
 		*val = spec ? 1 : 0;
 		return 0;
 	}
@@ -637,7 +637,7 @@ static int bpf_core_calc_field_relo(const char *prog_name,
 
 	/* a[n] accessor needs special handling */
 	if (!acc->name) {
-		if (relo->kind == BPF_FIELD_BYTE_OFFSET) {
+		if (relo->kind == BPF_CORE_FIELD_BYTE_OFFSET) {
 			*val = spec->bit_offset / 8;
 			/* remember field size for load/store mem size */
 			sz = btf__resolve_size(spec->btf, acc->type_id);
@@ -645,7 +645,7 @@ static int bpf_core_calc_field_relo(const char *prog_name,
 				return -EINVAL;
 			*field_sz = sz;
 			*type_id = acc->type_id;
-		} else if (relo->kind == BPF_FIELD_BYTE_SIZE) {
+		} else if (relo->kind == BPF_CORE_FIELD_BYTE_SIZE) {
 			sz = btf__resolve_size(spec->btf, acc->type_id);
 			if (sz < 0)
 				return -EINVAL;
@@ -697,36 +697,36 @@ static int bpf_core_calc_field_relo(const char *prog_name,
 		*validate = !bitfield;
 
 	switch (relo->kind) {
-	case BPF_FIELD_BYTE_OFFSET:
+	case BPF_CORE_FIELD_BYTE_OFFSET:
 		*val = byte_off;
 		if (!bitfield) {
 			*field_sz = byte_sz;
 			*type_id = field_type_id;
 		}
 		break;
-	case BPF_FIELD_BYTE_SIZE:
+	case BPF_CORE_FIELD_BYTE_SIZE:
 		*val = byte_sz;
 		break;
-	case BPF_FIELD_SIGNED:
+	case BPF_CORE_FIELD_SIGNED:
 		/* enums will be assumed unsigned */
 		*val = btf_is_enum(mt) ||
 		       (btf_int_encoding(mt) & BTF_INT_SIGNED);
 		if (validate)
 			*validate = true; /* signedness is never ambiguous */
 		break;
-	case BPF_FIELD_LSHIFT_U64:
+	case BPF_CORE_FIELD_LSHIFT_U64:
 #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
 		*val = 64 - (bit_off + bit_sz - byte_off  * 8);
 #else
 		*val = (8 - byte_sz) * 8 + (bit_off - byte_off * 8);
 #endif
 		break;
-	case BPF_FIELD_RSHIFT_U64:
+	case BPF_CORE_FIELD_RSHIFT_U64:
 		*val = 64 - bit_sz;
 		if (validate)
 			*validate = true; /* right shift is never ambiguous */
 		break;
-	case BPF_FIELD_EXISTS:
+	case BPF_CORE_FIELD_EXISTS:
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -747,20 +747,20 @@ static int bpf_core_calc_type_relo(const struct bpf_core_relo *relo,
 	}
 
 	switch (relo->kind) {
-	case BPF_TYPE_ID_TARGET:
+	case BPF_CORE_TYPE_ID_TARGET:
 		*val = spec->root_type_id;
 		break;
-	case BPF_TYPE_EXISTS:
+	case BPF_CORE_TYPE_EXISTS:
 		*val = 1;
 		break;
-	case BPF_TYPE_SIZE:
+	case BPF_CORE_TYPE_SIZE:
 		sz = btf__resolve_size(spec->btf, spec->root_type_id);
 		if (sz < 0)
 			return -EINVAL;
 		*val = sz;
 		break;
-	case BPF_TYPE_ID_LOCAL:
-	/* BPF_TYPE_ID_LOCAL is handled specially and shouldn't get here */
+	case BPF_CORE_TYPE_ID_LOCAL:
+	/* BPF_CORE_TYPE_ID_LOCAL is handled specially and shouldn't get here */
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -776,10 +776,10 @@ static int bpf_core_calc_enumval_relo(const struct bpf_core_relo *relo,
 	const struct btf_enum *e;
 
 	switch (relo->kind) {
-	case BPF_ENUMVAL_EXISTS:
+	case BPF_CORE_ENUMVAL_EXISTS:
 		*val = spec ? 1 : 0;
 		break;
-	case BPF_ENUMVAL_VALUE:
+	case BPF_CORE_ENUMVAL_VALUE:
 		if (!spec)
 			return -EUCLEAN; /* request instruction poisoning */
 		t = btf_type_by_id(spec->btf, spec->spec[0].type_id);
@@ -1238,7 +1238,7 @@ int bpf_core_apply_relo_insn(const char *prog_name, struct bpf_insn *insn,
 	libbpf_print(LIBBPF_DEBUG, "\n");
 
 	/* TYPE_ID_LOCAL relo is special and doesn't need candidate search */
-	if (relo->kind == BPF_TYPE_ID_LOCAL) {
+	if (relo->kind == BPF_CORE_TYPE_ID_LOCAL) {
 		targ_res.validate = true;
 		targ_res.poison = false;
 		targ_res.orig_val = local_spec.root_type_id;
@@ -1304,7 +1304,7 @@ int bpf_core_apply_relo_insn(const char *prog_name, struct bpf_insn *insn,
 	}
 
 	/*
-	 * For BPF_FIELD_EXISTS relo or when used BPF program has field
+	 * For BPF_CORE_FIELD_EXISTS relo or when used BPF program has field
 	 * existence checks or kernel version/config checks, it's expected
 	 * that we might not find any candidates. In this case, if field
 	 * wasn't found in any candidate, the list of candidates shouldn't
diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h
index 3b9f8f18346c..7720af11f96f 100644
--- a/tools/lib/bpf/relo_core.h
+++ b/tools/lib/bpf/relo_core.h
@@ -4,24 +4,6 @@
 #ifndef __RELO_CORE_H
 #define __RELO_CORE_H
 
-/* bpf_core_relo_kind encodes which aspect of captured field/type/enum value
- * has to be adjusted by relocations.
- */
-enum bpf_core_relo_kind {
-	BPF_FIELD_BYTE_OFFSET = 0,	/* field byte offset */
-	BPF_FIELD_BYTE_SIZE = 1,	/* field size in bytes */
-	BPF_FIELD_EXISTS = 2,		/* field existence in target kernel */
-	BPF_FIELD_SIGNED = 3,		/* field signedness (0 - unsigned, 1 - signed) */
-	BPF_FIELD_LSHIFT_U64 = 4,	/* bitfield-specific left bitshift */
-	BPF_FIELD_RSHIFT_U64 = 5,	/* bitfield-specific right bitshift */
-	BPF_TYPE_ID_LOCAL = 6,		/* type ID in local BPF object */
-	BPF_TYPE_ID_TARGET = 7,		/* type ID in target kernel */
-	BPF_TYPE_EXISTS = 8,		/* type existence in target kernel */
-	BPF_TYPE_SIZE = 9,		/* type size in bytes */
-	BPF_ENUMVAL_EXISTS = 10,	/* enum value existence in target kernel */
-	BPF_ENUMVAL_VALUE = 11,		/* enum value integer value */
-};
-
 /* The minimum bpf_core_relo checked by the loader
  *
  * CO-RE relocation captures the following data:
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 05/12] bpf: Pass a set of bpf_core_relo-s to prog_load command.
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
                   ` (3 preceding siblings ...)
  2021-11-12  5:02 ` [PATCH v2 bpf-next 04/12] bpf: Define enum bpf_core_relo_kind as uapi Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  1:17   ` Andrii Nakryiko
  2021-11-12  5:02 ` [PATCH v2 bpf-next 06/12] bpf: Add bpf_core_add_cands() and wire it into bpf_core_apply_relo_insn() Alexei Starovoitov
                   ` (7 subsequent siblings)
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

struct bpf_core_relo is generated by llvm and processed by libbpf.
It's a de-facto uapi.
With CO-RE in the kernel the struct bpf_core_relo becomes uapi de-jure.
Add an ability to pass a set of 'struct bpf_core_relo' to prog_load command
and let the kernel perform CO-RE relocations.

Note the struct bpf_line_info and struct bpf_func_info have the same
layout when passed from LLVM to libbpf and from libbpf to the kernel
except "insn_off" fields means "byte offset" when LLVM generates it.
Then libbpf converts it to "insn index" to pass to the kernel.
The struct bpf_core_relo's "insn_off" field is always "byte offset".

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/bpf.h            |  3 ++
 include/uapi/linux/bpf.h       | 59 +++++++++++++++++++++++++++-
 kernel/bpf/btf.c               |  7 ++++
 kernel/bpf/syscall.c           |  2 +-
 kernel/bpf/verifier.c          | 71 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 59 +++++++++++++++++++++++++++-
 tools/lib/bpf/relo_core.h      | 53 -------------------------
 7 files changed, 198 insertions(+), 56 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index df3410bff4b0..649501ab12b1 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1726,6 +1726,9 @@ bool bpf_prog_has_kfunc_call(const struct bpf_prog *prog);
 const struct btf_func_model *
 bpf_jit_find_kfunc_model(const struct bpf_prog *prog,
 			 const struct bpf_insn *insn);
+int bpf_core_relo_apply(struct bpf_verifier_log *log, const struct btf *btf,
+			const struct bpf_core_relo *relo, int relo_idx,
+			void *insn);
 #else /* !CONFIG_BPF_SYSCALL */
 static inline struct bpf_prog *bpf_prog_get(u32 ufd)
 {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 3c37bfe92359..941c7f22f787 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1342,8 +1342,10 @@ union bpf_attr {
 			/* or valid module BTF object fd or 0 to attach to vmlinux */
 			__u32		attach_btf_obj_fd;
 		};
-		__u32		:32;		/* pad */
+		__u32		core_relo_cnt;	/* number of bpf_core_relo */
 		__aligned_u64	fd_array;	/* array of FDs */
+		__aligned_u64	core_relo;
+		__u32		core_relo_rec_size; /* sizeof(struct bpf_core_relo) */
 	};
 
 	struct { /* anonymous struct used by BPF_OBJ_* commands */
@@ -6368,4 +6370,59 @@ enum bpf_core_relo_kind {
 	BPF_CORE_ENUMVAL_VALUE = 11,         /* enum value integer value */
 };
 
+/*
+ * "struct bpf_core_relo" is used to pass relocation data form LLVM to libbpf
+ * and from libbpf to the kernel.
+ *
+ * CO-RE relocation captures the following data:
+ * - insn_off - instruction offset (in bytes) within a BPF program that needs
+ *   its insn->imm field to be relocated with actual field info;
+ * - type_id - BTF type ID of the "root" (containing) entity of a relocatable
+ *   type or field;
+ * - access_str_off - offset into corresponding .BTF string section. String
+ *   interpretation depends on specific relocation kind:
+ *     - for field-based relocations, string encodes an accessed field using
+ *       a sequence of field and array indices, separated by colon (:). It's
+ *       conceptually very close to LLVM's getelementptr ([0]) instruction's
+ *       arguments for identifying offset to a field.
+ *     - for type-based relocations, strings is expected to be just "0";
+ *     - for enum value-based relocations, string contains an index of enum
+ *       value within its enum type;
+ * - kind - one of enum bpf_core_relo_kind;
+ *
+ * Example:
+ *   struct sample {
+ *       int a;
+ *       struct {
+ *           int b[10];
+ *       };
+ *   };
+ *
+ *   struct sample *s = ...;
+ *   int x = &s->a;     // encoded as "0:0" (a is field #0)
+ *   int y = &s->b[5];  // encoded as "0:1:0:5" (anon struct is field #1,
+ *                      // b is field #0 inside anon struct, accessing elem #5)
+ *   int z = &s[10]->b; // encoded as "10:1" (ptr is used as an array)
+ *
+ * type_id for all relocs in this example will capture BTF type id of
+ * `struct sample`.
+ *
+ * Such relocation is emitted when using __builtin_preserve_access_index()
+ * Clang built-in, passing expression that captures field address, e.g.:
+ *
+ * bpf_probe_read(&dst, sizeof(dst),
+ *		  __builtin_preserve_access_index(&src->a.b.c));
+ *
+ * In this case Clang will emit field relocation recording necessary data to
+ * be able to find offset of embedded `a.b.c` field within `src` struct.
+ *
+ * [0] https://llvm.org/docs/LangRef.html#getelementptr-instruction
+ */
+struct bpf_core_relo {
+	__u32 insn_off;
+	__u32 type_id;
+	__u32 access_str_off;
+	enum bpf_core_relo_kind kind;
+};
+
 #endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index e3cce46fa02f..efb7fa2f81a2 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6439,3 +6439,10 @@ size_t bpf_core_essential_name_len(const char *name)
 	}
 	return n;
 }
+
+int bpf_core_relo_apply(struct bpf_verifier_log *log, const struct btf *btf,
+			const struct bpf_core_relo *relo, int relo_idx,
+			void *insn)
+{
+	return -EOPNOTSUPP;
+}
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 50f96ea4452a..45c9bb932132 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2184,7 +2184,7 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type)
 }
 
 /* last field in 'union bpf_attr' used by this command */
-#define	BPF_PROG_LOAD_LAST_FIELD fd_array
+#define	BPF_PROG_LOAD_LAST_FIELD core_relo_rec_size
 
 static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
 {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1aafb43f61d1..c2246414e182 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10268,6 +10268,73 @@ static int check_btf_line(struct bpf_verifier_env *env,
 	return err;
 }
 
+static int check_core_relo(struct bpf_verifier_env *env,
+			   const union bpf_attr *attr,
+			   bpfptr_t uattr)
+{
+	u32 i, nr_core_relo, ncopy, expected_size, rec_size;
+	struct bpf_core_relo core_relo = {};
+	struct bpf_prog *prog = env->prog;
+	const struct btf *btf = prog->aux->btf;
+	bpfptr_t u_core_relo;
+	int err;
+
+	nr_core_relo = attr->core_relo_cnt;
+	if (!nr_core_relo)
+		return 0;
+	if (nr_core_relo > INT_MAX / sizeof(struct bpf_core_relo))
+		return -EINVAL;
+
+	rec_size = attr->core_relo_rec_size;
+	if (rec_size != sizeof(struct bpf_core_relo))
+		return -EINVAL;
+
+	u_core_relo = make_bpfptr(attr->core_relo, uattr.is_kernel);
+	expected_size = sizeof(struct bpf_core_relo);
+	ncopy = min_t(u32, expected_size, rec_size);
+
+	/* Unlike func_info and line_info, copy and apply each CO-RE
+	 * relocation record one at a time
+	 */
+	for (i = 0; i < nr_core_relo; i++) {
+		/* future proofing when sizeof(bpf_core_relo) changes */
+		err = bpf_check_uarg_tail_zero(u_core_relo, expected_size, rec_size);
+		if (err) {
+			if (err == -E2BIG) {
+				verbose(env, "nonzero tailing record in core_relo");
+				if (copy_to_bpfptr_offset(uattr,
+							  offsetof(union bpf_attr, core_relo_rec_size),
+							  &expected_size, sizeof(expected_size)))
+					err = -EFAULT;
+			}
+			goto err;
+		}
+
+		if (copy_from_bpfptr(&core_relo, u_core_relo, ncopy)) {
+			err = -EFAULT;
+			goto err;
+		}
+
+		if (core_relo.insn_off % 8 || core_relo.insn_off / 8 >= prog->len) {
+			verbose(env, "Invalid core_relo[%u].insn_off:%u prog->len:%u\n",
+				i, core_relo.insn_off, prog->len);
+			err = -EINVAL;
+			goto err;
+		}
+
+		err = bpf_core_relo_apply(&env->log, btf, &core_relo, i,
+					  &prog->insnsi[core_relo.insn_off / 8]);
+		if (err)
+			goto err;
+		bpfptr_add(&u_core_relo, rec_size);
+	}
+
+	return 0;
+
+err:
+	return err;
+}
+
 static int check_btf_info(struct bpf_verifier_env *env,
 			  const union bpf_attr *attr,
 			  bpfptr_t uattr)
@@ -10298,6 +10365,10 @@ static int check_btf_info(struct bpf_verifier_env *env,
 	if (err)
 		return err;
 
+	err = check_core_relo(env, attr, uattr);
+	if (err)
+		return err;
+
 	return 0;
 }
 
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 3c37bfe92359..941c7f22f787 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1342,8 +1342,10 @@ union bpf_attr {
 			/* or valid module BTF object fd or 0 to attach to vmlinux */
 			__u32		attach_btf_obj_fd;
 		};
-		__u32		:32;		/* pad */
+		__u32		core_relo_cnt;	/* number of bpf_core_relo */
 		__aligned_u64	fd_array;	/* array of FDs */
+		__aligned_u64	core_relo;
+		__u32		core_relo_rec_size; /* sizeof(struct bpf_core_relo) */
 	};
 
 	struct { /* anonymous struct used by BPF_OBJ_* commands */
@@ -6368,4 +6370,59 @@ enum bpf_core_relo_kind {
 	BPF_CORE_ENUMVAL_VALUE = 11,         /* enum value integer value */
 };
 
+/*
+ * "struct bpf_core_relo" is used to pass relocation data form LLVM to libbpf
+ * and from libbpf to the kernel.
+ *
+ * CO-RE relocation captures the following data:
+ * - insn_off - instruction offset (in bytes) within a BPF program that needs
+ *   its insn->imm field to be relocated with actual field info;
+ * - type_id - BTF type ID of the "root" (containing) entity of a relocatable
+ *   type or field;
+ * - access_str_off - offset into corresponding .BTF string section. String
+ *   interpretation depends on specific relocation kind:
+ *     - for field-based relocations, string encodes an accessed field using
+ *       a sequence of field and array indices, separated by colon (:). It's
+ *       conceptually very close to LLVM's getelementptr ([0]) instruction's
+ *       arguments for identifying offset to a field.
+ *     - for type-based relocations, strings is expected to be just "0";
+ *     - for enum value-based relocations, string contains an index of enum
+ *       value within its enum type;
+ * - kind - one of enum bpf_core_relo_kind;
+ *
+ * Example:
+ *   struct sample {
+ *       int a;
+ *       struct {
+ *           int b[10];
+ *       };
+ *   };
+ *
+ *   struct sample *s = ...;
+ *   int x = &s->a;     // encoded as "0:0" (a is field #0)
+ *   int y = &s->b[5];  // encoded as "0:1:0:5" (anon struct is field #1,
+ *                      // b is field #0 inside anon struct, accessing elem #5)
+ *   int z = &s[10]->b; // encoded as "10:1" (ptr is used as an array)
+ *
+ * type_id for all relocs in this example will capture BTF type id of
+ * `struct sample`.
+ *
+ * Such relocation is emitted when using __builtin_preserve_access_index()
+ * Clang built-in, passing expression that captures field address, e.g.:
+ *
+ * bpf_probe_read(&dst, sizeof(dst),
+ *		  __builtin_preserve_access_index(&src->a.b.c));
+ *
+ * In this case Clang will emit field relocation recording necessary data to
+ * be able to find offset of embedded `a.b.c` field within `src` struct.
+ *
+ * [0] https://llvm.org/docs/LangRef.html#getelementptr-instruction
+ */
+struct bpf_core_relo {
+	__u32 insn_off;
+	__u32 type_id;
+	__u32 access_str_off;
+	enum bpf_core_relo_kind kind;
+};
+
 #endif /* _UAPI__LINUX_BPF_H__ */
diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h
index 7720af11f96f..b0a555f148bf 100644
--- a/tools/lib/bpf/relo_core.h
+++ b/tools/lib/bpf/relo_core.h
@@ -4,59 +4,6 @@
 #ifndef __RELO_CORE_H
 #define __RELO_CORE_H
 
-/* The minimum bpf_core_relo checked by the loader
- *
- * CO-RE relocation captures the following data:
- * - insn_off - instruction offset (in bytes) within a BPF program that needs
- *   its insn->imm field to be relocated with actual field info;
- * - type_id - BTF type ID of the "root" (containing) entity of a relocatable
- *   type or field;
- * - access_str_off - offset into corresponding .BTF string section. String
- *   interpretation depends on specific relocation kind:
- *     - for field-based relocations, string encodes an accessed field using
- *     a sequence of field and array indices, separated by colon (:). It's
- *     conceptually very close to LLVM's getelementptr ([0]) instruction's
- *     arguments for identifying offset to a field.
- *     - for type-based relocations, strings is expected to be just "0";
- *     - for enum value-based relocations, string contains an index of enum
- *     value within its enum type;
- *
- * Example to provide a better feel.
- *
- *   struct sample {
- *       int a;
- *       struct {
- *           int b[10];
- *       };
- *   };
- *
- *   struct sample *s = ...;
- *   int x = &s->a;     // encoded as "0:0" (a is field #0)
- *   int y = &s->b[5];  // encoded as "0:1:0:5" (anon struct is field #1,
- *                      // b is field #0 inside anon struct, accessing elem #5)
- *   int z = &s[10]->b; // encoded as "10:1" (ptr is used as an array)
- *
- * type_id for all relocs in this example  will capture BTF type id of
- * `struct sample`.
- *
- * Such relocation is emitted when using __builtin_preserve_access_index()
- * Clang built-in, passing expression that captures field address, e.g.:
- *
- * bpf_probe_read(&dst, sizeof(dst),
- *		  __builtin_preserve_access_index(&src->a.b.c));
- *
- * In this case Clang will emit field relocation recording necessary data to
- * be able to find offset of embedded `a.b.c` field within `src` struct.
- *
- *   [0] https://llvm.org/docs/LangRef.html#getelementptr-instruction
- */
-struct bpf_core_relo {
-	__u32   insn_off;
-	__u32   type_id;
-	__u32   access_str_off;
-	enum bpf_core_relo_kind kind;
-};
-
 struct bpf_core_cand {
 	const struct btf *btf;
 	const struct btf_type *t;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 06/12] bpf: Add bpf_core_add_cands() and wire it into bpf_core_apply_relo_insn().
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
                   ` (4 preceding siblings ...)
  2021-11-12  5:02 ` [PATCH v2 bpf-next 05/12] bpf: Pass a set of bpf_core_relo-s to prog_load command Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  1:31   ` Andrii Nakryiko
  2021-11-12  5:02 ` [PATCH v2 bpf-next 07/12] libbpf: Use CO-RE in the kernel in light skeleton Alexei Starovoitov
                   ` (6 subsequent siblings)
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

Given BPF program's BTF perform a linear search through kernel BTFs for
a possible candidate.
Then wire the result into bpf_core_apply_relo_insn().

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/btf.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 137 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index efb7fa2f81a2..aeb591579282 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -25,6 +25,7 @@
 #include <linux/kobject.h>
 #include <linux/sysfs.h>
 #include <net/sock.h>
+#include "../tools/lib/bpf/relo_core.h"
 
 /* BTF (BPF Type Format) is the meta data format which describes
  * the data types of BPF program/map.  Hence, it basically focus
@@ -6440,9 +6441,144 @@ size_t bpf_core_essential_name_len(const char *name)
 	return n;
 }
 
+static void bpf_core_free_cands(struct bpf_core_cand_list *cands)
+{
+	if (!cands)
+		return;
+        kfree(cands->cands);
+        kfree(cands);
+}
+
+static int bpf_core_add_cands(struct bpf_verifier_log *log,
+			      struct bpf_core_cand *local_cand,
+                              size_t local_essent_len,
+                              const struct btf *targ_btf,
+                              int targ_start_id,
+                              struct bpf_core_cand_list *cands)
+{
+	struct bpf_core_cand *new_cands, *cand;
+	const struct btf_type *t;
+	const char *targ_name;
+	size_t targ_essent_len;
+	int n, i;
+
+	n = btf_nr_types(targ_btf);
+	for (i = targ_start_id; i < n; i++) {
+		t = btf_type_by_id(targ_btf, i);
+		if (btf_kind(t) != btf_kind(local_cand->t))
+			continue;
+
+		targ_name = btf_name_by_offset(targ_btf, t->name_off);
+		if (str_is_empty(targ_name))
+			continue;
+
+		targ_essent_len = bpf_core_essential_name_len(targ_name);
+		if (targ_essent_len != local_essent_len)
+			continue;
+
+		if (strncmp(local_cand->name, targ_name, local_essent_len) != 0)
+			continue;
+
+		bpf_log(log,
+			"CO-RE relocating [%d] %s %s: found target candidate [%d] %s %s\n",
+			local_cand->id, btf_type_str(local_cand->t),
+			local_cand->name, i, btf_type_str(t), targ_name);
+		new_cands = krealloc(cands->cands,
+				     (cands->len + 1) * sizeof(*cands->cands), GFP_KERNEL);
+		if (!new_cands)
+			return -ENOMEM;
+
+		cand = &new_cands[cands->len];
+		cand->btf = targ_btf;
+		cand->t = t;
+		cand->name = targ_name;
+		cand->id = i;
+
+		cands->cands = new_cands;
+		cands->len++;
+	}
+	return 0;
+}
+
+static struct bpf_core_cand_list *
+bpf_core_find_cands(struct bpf_verifier_log *log, const struct btf *local_btf, u32 local_type_id)
+{
+	struct bpf_core_cand local_cand = {};
+	struct bpf_core_cand_list *cands;
+	const struct btf *main_btf;
+	size_t local_essent_len;
+	struct btf *mod_btf;
+	int err;
+	int id;
+
+	local_cand.btf = local_btf;
+	local_cand.t = btf_type_by_id(local_btf, local_type_id);
+	if (!local_cand.t)
+		return ERR_PTR(-EINVAL);
+
+	local_cand.name = btf_name_by_offset(local_btf, local_cand.t->name_off);
+	if (str_is_empty(local_cand.name))
+		return ERR_PTR(-EINVAL);
+	local_essent_len = bpf_core_essential_name_len(local_cand.name);
+
+	cands = kcalloc(1, sizeof(*cands), GFP_KERNEL);
+	if (!cands)
+		return ERR_PTR(-ENOMEM);
+
+	/* Attempt to find target candidates in vmlinux BTF first */
+	main_btf = bpf_get_btf_vmlinux();
+	err = bpf_core_add_cands(log, &local_cand, local_essent_len, main_btf, 1, cands);
+	if (err)
+		goto err_out;
+
+	/* if vmlinux BTF has any candidate, don't go for module BTFs */
+	if (cands->len)
+		return cands;
+
+	/* If candidate is not found in vmlinux's BTF then search in module's BTFs */
+	spin_lock_bh(&btf_idr_lock);
+	idr_for_each_entry(&btf_idr, mod_btf, id) {
+		if (!btf_is_module(mod_btf))
+			continue;
+		/* linear search could be slow hence unlock/lock
+		 * the IDR to avoiding holding it for too long
+		 */
+		btf_get(mod_btf);
+		spin_unlock_bh(&btf_idr_lock);
+		err = bpf_core_add_cands(log, &local_cand, local_essent_len,
+					 mod_btf, btf_nr_types(main_btf), cands);
+		if (err) {
+			btf_put(mod_btf);
+			goto err_out;
+		}
+		spin_lock_bh(&btf_idr_lock);
+		btf_put(mod_btf);
+	}
+	spin_unlock_bh(&btf_idr_lock);
+
+	return cands;
+err_out:
+	bpf_core_free_cands(cands);
+	return ERR_PTR(err);
+}
+
 int bpf_core_relo_apply(struct bpf_verifier_log *log, const struct btf *btf,
 			const struct bpf_core_relo *relo, int relo_idx,
 			void *insn)
 {
-	return -EOPNOTSUPP;
+	struct bpf_core_cand_list *cands = NULL;
+	int err;
+
+	if (relo->kind != BPF_CORE_TYPE_ID_LOCAL) {
+		cands = bpf_core_find_cands(log, btf, relo->type_id);
+		if (IS_ERR(cands)) {
+			bpf_log(log, "target candidate search failed for %d\n",
+			       relo->type_id);
+                        return PTR_ERR(cands);
+                }
+	}
+	err = bpf_core_apply_relo_insn((void *)log, insn, relo->insn_off / 8,
+				       relo, relo_idx, btf, cands);
+	bpf_core_free_cands(cands);
+	return err;
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 07/12] libbpf: Use CO-RE in the kernel in light skeleton.
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
                   ` (5 preceding siblings ...)
  2021-11-12  5:02 ` [PATCH v2 bpf-next 06/12] bpf: Add bpf_core_add_cands() and wire it into bpf_core_apply_relo_insn() Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  3:45   ` Andrii Nakryiko
  2021-11-12  5:02 ` [PATCH v2 bpf-next 08/12] libbpf: Support init of inner maps " Alexei Starovoitov
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

Without lskel the CO-RE relocations are processed by libbpf before any other
work is done. Instead, when lksel is needed, remember relocation as RELO_CORE
kind. Then when loader prog is generated for a given bpf program pass CO-RE
relos of that program to gen loader via bpf_gen__record_relo_core(). The gen
loader will remember them as-is and pass it later as-is into the kernel.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/lib/bpf/bpf_gen_internal.h |   3 +
 tools/lib/bpf/gen_loader.c       |  41 +++++++++++-
 tools/lib/bpf/libbpf.c           | 104 +++++++++++++++++++++++--------
 3 files changed, 119 insertions(+), 29 deletions(-)

diff --git a/tools/lib/bpf/bpf_gen_internal.h b/tools/lib/bpf/bpf_gen_internal.h
index 75ca9fb857b2..ed162fdeecf6 100644
--- a/tools/lib/bpf/bpf_gen_internal.h
+++ b/tools/lib/bpf/bpf_gen_internal.h
@@ -39,6 +39,8 @@ struct bpf_gen {
 	int error;
 	struct ksym_relo_desc *relos;
 	int relo_cnt;
+	struct bpf_core_relo *core_relo;
+	int core_relo_cnt;
 	char attach_target[128];
 	int attach_kind;
 	struct ksym_desc *ksyms;
@@ -61,5 +63,6 @@ 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,
 			    bool is_typeless, int kind, int insn_idx);
+void bpf_gen__record_relo_core(struct bpf_gen *gen, const struct bpf_core_relo *core_relo);
 
 #endif
diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
index 7b73f97b1fa1..442c4477e38e 100644
--- a/tools/lib/bpf/gen_loader.c
+++ b/tools/lib/bpf/gen_loader.c
@@ -839,6 +839,22 @@ static void emit_relo_ksym_btf(struct bpf_gen *gen, struct ksym_relo_desc *relo,
 	emit_ksym_relo_log(gen, relo, kdesc->ref);
 }
 
+void bpf_gen__record_relo_core(struct bpf_gen *gen,
+			       const struct bpf_core_relo *core_relo)
+{
+	struct bpf_core_relo *relo;
+
+	relo = libbpf_reallocarray(gen->core_relo, gen->core_relo_cnt + 1, sizeof(*relo));
+	if (!relo) {
+		gen->error = -ENOMEM;
+		return;
+	}
+	gen->core_relo = relo;
+	relo += gen->core_relo_cnt;
+	memcpy(relo, core_relo, sizeof(*relo));
+	gen->core_relo_cnt++;
+}
+
 static void emit_relo(struct bpf_gen *gen, struct ksym_relo_desc *relo, int insns)
 {
 	int insn;
@@ -871,6 +887,15 @@ static void emit_relos(struct bpf_gen *gen, int insns)
 		emit_relo(gen, gen->relos + i, insns);
 }
 
+static void cleanup_core_relo(struct bpf_gen *gen)
+{
+	if (!gen->core_relo_cnt)
+		return;
+	free(gen->core_relo);
+	gen->core_relo_cnt = 0;
+	gen->core_relo = NULL;
+}
+
 static void cleanup_relos(struct bpf_gen *gen, int insns)
 {
 	int i, insn;
@@ -898,6 +923,7 @@ static void cleanup_relos(struct bpf_gen *gen, int insns)
 		gen->relo_cnt = 0;
 		gen->relos = NULL;
 	}
+	cleanup_core_relo(gen);
 }
 
 void bpf_gen__prog_load(struct bpf_gen *gen,
@@ -905,12 +931,13 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
 			const char *license, struct bpf_insn *insns, size_t insn_cnt,
 			struct bpf_prog_load_opts *load_attr, int prog_idx)
 {
-	int attr_size = offsetofend(union bpf_attr, fd_array);
-	int prog_load_attr, license_off, insns_off, func_info, line_info;
+	int prog_load_attr, license_off, insns_off, func_info, line_info, core_relo;
+	int attr_size = offsetofend(union bpf_attr, core_relo_rec_size);
 	union bpf_attr attr;
 
 	memset(&attr, 0, attr_size);
-	pr_debug("gen: prog_load: type %d insns_cnt %zd\n", prog_type, insn_cnt);
+	pr_debug("gen: prog_load: type %d insns_cnt %zd progi_idx %d\n",
+		 prog_type, insn_cnt, prog_idx);
 	/* add license string to blob of bytes */
 	license_off = add_data(gen, license, strlen(license) + 1);
 	/* add insns to blob of bytes */
@@ -934,6 +961,11 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
 	line_info = add_data(gen, load_attr->line_info,
 			     attr.line_info_cnt * attr.line_info_rec_size);
 
+	attr.core_relo_rec_size = sizeof(struct bpf_core_relo);
+	attr.core_relo_cnt = gen->core_relo_cnt;
+	core_relo = add_data(gen, gen->core_relo,
+			     attr.core_relo_cnt * attr.core_relo_rec_size);
+
 	memcpy(attr.prog_name, prog_name,
 	       min((unsigned)strlen(prog_name), BPF_OBJ_NAME_LEN - 1));
 	prog_load_attr = add_data(gen, &attr, attr_size);
@@ -950,6 +982,9 @@ void bpf_gen__prog_load(struct bpf_gen *gen,
 	/* populate union bpf_attr with a pointer to line_info */
 	emit_rel_store(gen, attr_field(prog_load_attr, line_info), line_info);
 
+	/* populate union bpf_attr with a pointer to core_relo */
+	emit_rel_store(gen, attr_field(prog_load_attr, core_relo), core_relo);
+
 	/* populate union bpf_attr fd_array with a pointer to data where map_fds are saved */
 	emit_rel_store(gen, attr_field(prog_load_attr, fd_array), gen->fd_array);
 
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index e41867550ef2..2311f484511a 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -211,6 +211,7 @@ enum reloc_type {
 	RELO_EXTERN_VAR,
 	RELO_EXTERN_FUNC,
 	RELO_SUBPROG_ADDR,
+	RELO_CORE,
 };
 
 struct reloc_desc {
@@ -218,6 +219,9 @@ struct reloc_desc {
 	int insn_idx;
 	int map_idx;
 	int sym_off;
+	int access_str_off;
+	int type_id;
+	enum bpf_core_relo_kind kind;
 };
 
 struct bpf_sec_def;
@@ -5398,6 +5402,26 @@ static void *u32_as_hash_key(__u32 x)
 	return (void *)(uintptr_t)x;
 }
 
+static int record_relo_core(struct bpf_program *prog,
+			    const struct bpf_core_relo *core_relo, int insn_idx)
+{
+	struct reloc_desc *relos, *relo;
+
+	relos = libbpf_reallocarray(prog->reloc_desc,
+				    prog->nr_reloc + 1, sizeof(*relos));
+	if (!relos)
+		return -ENOMEM;
+	relo = &relos[prog->nr_reloc];
+	relo->type = RELO_CORE;
+	relo->insn_idx = insn_idx;
+	relo->access_str_off = core_relo->access_str_off;
+	relo->type_id = core_relo->type_id;
+	relo->kind = core_relo->kind;
+	prog->reloc_desc = relos;
+	prog->nr_reloc++;
+	return 0;
+}
+
 static int bpf_core_apply_relo(struct bpf_program *prog,
 			       const struct bpf_core_relo *relo,
 			       int relo_idx,
@@ -5434,10 +5458,16 @@ static int bpf_core_apply_relo(struct bpf_program *prog,
 		return -EINVAL;
 
 	if (prog->obj->gen_loader) {
-		pr_warn("// TODO core_relo: prog %td insn[%d] %s kind %d\n",
+		const char *spec_str = btf__name_by_offset(local_btf, relo->access_str_off);
+
+		/* Adjust relo_core's insn_idx since final subprog offset in
+		 * the main prog is known.
+		 */
+		insn_idx += prog->sub_insn_off;
+		pr_debug("record_relo_core: prog %td insn[%d] %s %s %s final insn_idx %d\n",
 			prog - prog->obj->programs, relo->insn_off / 8,
-			local_name, relo->kind);
-		return -ENOTSUP;
+			btf_kind_str(local_type), local_name, spec_str, insn_idx);
+		return record_relo_core(prog, relo, insn_idx);
 	}
 
 	if (relo->kind != BPF_CORE_TYPE_ID_LOCAL &&
@@ -5634,6 +5664,9 @@ bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
 		case RELO_CALL:
 			/* handled already */
 			break;
+		case RELO_CORE:
+			/* handled already */
+			break;
 		default:
 			pr_warn("prog '%s': relo #%d: bad relo type %d\n",
 				prog->name, i, relo->type);
@@ -6071,6 +6104,35 @@ bpf_object__free_relocs(struct bpf_object *obj)
 	}
 }
 
+static int cmp_relocs(const void *_a, const void *_b)
+{
+	const struct reloc_desc *a = _a;
+	const struct reloc_desc *b = _b;
+
+	if (a->insn_idx != b->insn_idx)
+		return a->insn_idx < b->insn_idx ? -1 : 1;
+
+	/* no two relocations should have the same insn_idx, but ... */
+	if (a->type != b->type)
+		return a->type < b->type ? -1 : 1;
+
+	return 0;
+}
+
+static void bpf_object__sort_relos(struct bpf_object *obj)
+{
+	int i;
+
+	for (i = 0; i < obj->nr_programs; i++) {
+		struct bpf_program *p = &obj->programs[i];
+
+		if (!p->nr_reloc)
+			continue;
+
+		qsort(p->reloc_desc, p->nr_reloc, sizeof(*p->reloc_desc), cmp_relocs);
+	}
+}
+
 static int
 bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path)
 {
@@ -6085,6 +6147,8 @@ bpf_object__relocate(struct bpf_object *obj, const char *targ_btf_path)
 				err);
 			return err;
 		}
+		if (obj->gen_loader)
+			bpf_object__sort_relos(obj);
 	}
 
 	/* Before relocating calls pre-process relocations and mark
@@ -6262,21 +6326,6 @@ static int bpf_object__collect_map_relos(struct bpf_object *obj,
 	return 0;
 }
 
-static int cmp_relocs(const void *_a, const void *_b)
-{
-	const struct reloc_desc *a = _a;
-	const struct reloc_desc *b = _b;
-
-	if (a->insn_idx != b->insn_idx)
-		return a->insn_idx < b->insn_idx ? -1 : 1;
-
-	/* no two relocations should have the same insn_idx, but ... */
-	if (a->type != b->type)
-		return a->type < b->type ? -1 : 1;
-
-	return 0;
-}
-
 static int bpf_object__collect_relos(struct bpf_object *obj)
 {
 	int i, err;
@@ -6309,14 +6358,7 @@ static int bpf_object__collect_relos(struct bpf_object *obj)
 			return err;
 	}
 
-	for (i = 0; i < obj->nr_programs; i++) {
-		struct bpf_program *p = &obj->programs[i];
-
-		if (!p->nr_reloc)
-			continue;
-
-		qsort(p->reloc_desc, p->nr_reloc, sizeof(*p->reloc_desc), cmp_relocs);
-	}
+	bpf_object__sort_relos(obj);
 	return 0;
 }
 
@@ -6581,6 +6623,16 @@ static int bpf_program__record_externs(struct bpf_program *prog)
 					       ext->is_weak, false, BTF_KIND_FUNC,
 					       relo->insn_idx);
 			break;
+		case RELO_CORE: {
+			struct bpf_core_relo cr = {
+				.insn_off = relo->insn_idx * 8,
+				.type_id = relo->type_id,
+				.access_str_off = relo->access_str_off,
+				.kind = relo->kind,
+			};
+			bpf_gen__record_relo_core(obj->gen_loader, &cr);
+			break;
+		}
 		default:
 			continue;
 		}
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 08/12] libbpf: Support init of inner maps in light skeleton.
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
                   ` (6 preceding siblings ...)
  2021-11-12  5:02 ` [PATCH v2 bpf-next 07/12] libbpf: Use CO-RE in the kernel in light skeleton Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  4:10   ` Andrii Nakryiko
  2021-11-12  5:02 ` [PATCH v2 bpf-next 09/12] selftests/bpf: Convert kfunc test with CO-RE to lskel Alexei Starovoitov
                   ` (4 subsequent siblings)
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

Add ability to initialize inner maps in light skeleton.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/lib/bpf/bpf_gen_internal.h |  1 +
 tools/lib/bpf/gen_loader.c       | 27 +++++++++++++++++++++++++++
 tools/lib/bpf/libbpf.c           |  6 +++---
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/bpf_gen_internal.h b/tools/lib/bpf/bpf_gen_internal.h
index ed162fdeecf6..34a7f529be39 100644
--- a/tools/lib/bpf/bpf_gen_internal.h
+++ b/tools/lib/bpf/bpf_gen_internal.h
@@ -64,5 +64,6 @@ void bpf_gen__record_attach_target(struct bpf_gen *gen, const char *name, enum b
 void bpf_gen__record_extern(struct bpf_gen *gen, const char *name, bool is_weak,
 			    bool is_typeless, int kind, int insn_idx);
 void bpf_gen__record_relo_core(struct bpf_gen *gen, const struct bpf_core_relo *core_relo);
+void bpf_gen__populate_outer_map(struct bpf_gen *gen, int outer_map_idx, int key, int inner_map_idx);
 
 #endif
diff --git a/tools/lib/bpf/gen_loader.c b/tools/lib/bpf/gen_loader.c
index 442c4477e38e..21aa564c9b91 100644
--- a/tools/lib/bpf/gen_loader.c
+++ b/tools/lib/bpf/gen_loader.c
@@ -1063,6 +1063,33 @@ void bpf_gen__map_update_elem(struct bpf_gen *gen, int map_idx, void *pvalue,
 	emit_check_err(gen);
 }
 
+void bpf_gen__populate_outer_map(struct bpf_gen *gen, int outer_map_idx, int slot,
+				 int inner_map_idx)
+{
+	int attr_size = offsetofend(union bpf_attr, flags);
+	int map_update_attr, key;
+	union bpf_attr attr;
+
+	memset(&attr, 0, attr_size);
+	pr_debug("gen: populate_outer_map: outer %d key %d inner %d\n",
+		 outer_map_idx, slot, inner_map_idx);
+
+	key = add_data(gen, &slot, sizeof(slot));
+
+	map_update_attr = add_data(gen, &attr, attr_size);
+	move_blob2blob(gen, attr_field(map_update_attr, map_fd), 4,
+		       blob_fd_array_off(gen, outer_map_idx));
+	emit_rel_store(gen, attr_field(map_update_attr, key), key);
+	emit_rel_store(gen, attr_field(map_update_attr, value),
+		       blob_fd_array_off(gen, inner_map_idx));
+
+	/* emit MAP_UPDATE_ELEM command */
+	emit_sys_bpf(gen, BPF_MAP_UPDATE_ELEM, map_update_attr, attr_size);
+	debug_ret(gen, "populate_outer_map outer %d key %d inner %d",
+		  outer_map_idx, slot, inner_map_idx);
+	emit_check_err(gen);
+}
+
 void bpf_gen__map_freeze(struct bpf_gen *gen, int map_idx)
 {
 	int attr_size = offsetofend(union bpf_attr, map_fd);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 2311f484511a..a8a827d778ee 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4956,9 +4956,9 @@ static int init_map_slots(struct bpf_object *obj, struct bpf_map *map)
 		targ_map = map->init_slots[i];
 		fd = bpf_map__fd(targ_map);
 		if (obj->gen_loader) {
-			pr_warn("// TODO map_update_elem: idx %td key %d value==map_idx %td\n",
-				map - obj->maps, i, targ_map - obj->maps);
-			return -ENOTSUP;
+			bpf_gen__populate_outer_map(obj->gen_loader,
+						    map - obj->maps, i,
+						    targ_map - obj->maps);
 		} else {
 			err = bpf_map_update_elem(map->fd, &i, &fd, 0);
 		}
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 09/12] selftests/bpf: Convert kfunc test with CO-RE to lskel.
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
                   ` (7 preceding siblings ...)
  2021-11-12  5:02 ` [PATCH v2 bpf-next 08/12] libbpf: Support init of inner maps " Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  4:13   ` Andrii Nakryiko
  2021-11-12  5:02 ` [PATCH v2 bpf-next 10/12] selftests/bpf: Improve inner_map test coverage Alexei Starovoitov
                   ` (3 subsequent siblings)
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

Convert kfunc_call_test_subprog to light skeleton.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/testing/selftests/bpf/Makefile                |  3 ++-
 tools/testing/selftests/bpf/prog_tests/kfunc_call.c | 10 +++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 0470802c907c..811c5e150aa9 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -325,7 +325,8 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h		\
 		linked_vars.skel.h linked_maps.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
+	test_ringbuf.c atomics.c trace_printk.c trace_vprintk.c \
+	kfunc_call_test_subprog.c
 # Generate both light skeleton and libbpf skeleton for these
 LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c
 SKEL_BLACKLIST += $$(LSKELS)
diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
index 5c9c0176991b..4ba6f3867b3c 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
@@ -3,7 +3,7 @@
 #include <test_progs.h>
 #include <network_helpers.h>
 #include "kfunc_call_test.lskel.h"
-#include "kfunc_call_test_subprog.skel.h"
+#include "kfunc_call_test_subprog.lskel.h"
 
 static void test_main(void)
 {
@@ -31,14 +31,14 @@ static void test_main(void)
 
 static void test_subprog(void)
 {
-	struct kfunc_call_test_subprog *skel;
+	struct kfunc_call_test_subprog_lskel *skel;
 	int prog_fd, retval, err;
 
-	skel = kfunc_call_test_subprog__open_and_load();
+	skel = kfunc_call_test_subprog_lskel__open_and_load();
 	if (!ASSERT_OK_PTR(skel, "skel"))
 		return;
 
-	prog_fd = bpf_program__fd(skel->progs.kfunc_call_test1);
+	prog_fd = skel->progs.kfunc_call_test1.prog_fd;
 	err = bpf_prog_test_run(prog_fd, 1, &pkt_v4, sizeof(pkt_v4),
 				NULL, NULL, (__u32 *)&retval, NULL);
 	ASSERT_OK(err, "bpf_prog_test_run(test1)");
@@ -46,7 +46,7 @@ static void test_subprog(void)
 	ASSERT_NEQ(skel->data->active_res, -1, "active_res");
 	ASSERT_EQ(skel->data->sk_state_res, BPF_TCP_CLOSE, "sk_state_res");
 
-	kfunc_call_test_subprog__destroy(skel);
+	kfunc_call_test_subprog_lskel__destroy(skel);
 }
 
 void test_kfunc_call(void)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 10/12] selftests/bpf: Improve inner_map test coverage.
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
                   ` (8 preceding siblings ...)
  2021-11-12  5:02 ` [PATCH v2 bpf-next 09/12] selftests/bpf: Convert kfunc test with CO-RE to lskel Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  4:15   ` Andrii Nakryiko
  2021-11-12  5:02 ` [PATCH v2 bpf-next 11/12] selftests/bpf: Convert map_ptr_kern test to use light skeleton Alexei Starovoitov
                   ` (2 subsequent siblings)
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

Check that hash and array inner maps are properly initialized.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/testing/selftests/bpf/progs/map_ptr_kern.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
index b1b711d9b214..b64df94ec476 100644
--- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c
+++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
@@ -334,9 +334,11 @@ static inline int check_lpm_trie(void)
 	return 1;
 }
 
+#define INNER_MAX_ENTRIES 1234
+
 struct inner_map {
 	__uint(type, BPF_MAP_TYPE_ARRAY);
-	__uint(max_entries, 1);
+	__uint(max_entries, INNER_MAX_ENTRIES);
 	__type(key, __u32);
 	__type(value, __u32);
 } inner_map SEC(".maps");
@@ -348,7 +350,7 @@ struct {
 	__type(value, __u32);
 	__array(values, struct {
 		__uint(type, BPF_MAP_TYPE_ARRAY);
-		__uint(max_entries, 1);
+		__uint(max_entries, INNER_MAX_ENTRIES);
 		__type(key, __u32);
 		__type(value, __u32);
 	});
@@ -360,8 +362,13 @@ static inline int check_array_of_maps(void)
 {
 	struct bpf_array *array_of_maps = (struct bpf_array *)&m_array_of_maps;
 	struct bpf_map *map = (struct bpf_map *)&m_array_of_maps;
+	struct bpf_array *inner_map;
+	int key = 0;
 
 	VERIFY(check_default(&array_of_maps->map, map));
+	inner_map = bpf_map_lookup_elem(array_of_maps, &key);
+	VERIFY(inner_map != 0);
+	VERIFY(inner_map->map.max_entries == INNER_MAX_ENTRIES);
 
 	return 1;
 }
@@ -382,8 +389,13 @@ static inline int check_hash_of_maps(void)
 {
 	struct bpf_htab *hash_of_maps = (struct bpf_htab *)&m_hash_of_maps;
 	struct bpf_map *map = (struct bpf_map *)&m_hash_of_maps;
+	struct bpf_htab *inner_map;
+	int key = 2;
 
 	VERIFY(check_default(&hash_of_maps->map, map));
+	inner_map = bpf_map_lookup_elem(hash_of_maps, &key);
+	VERIFY(inner_map != 0);
+	VERIFY(inner_map->map.max_entries == INNER_MAX_ENTRIES);
 
 	return 1;
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 11/12] selftests/bpf: Convert map_ptr_kern test to use light skeleton.
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
                   ` (9 preceding siblings ...)
  2021-11-12  5:02 ` [PATCH v2 bpf-next 10/12] selftests/bpf: Improve inner_map test coverage Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  4:16   ` Andrii Nakryiko
  2021-11-12  5:02 ` [PATCH v2 bpf-next 12/12] selftests/bpf: Additional test for CO-RE in the kernel Alexei Starovoitov
  2021-11-17  0:48 ` [PATCH v2 bpf-next 00/12] bpf: CO-RE support " Matteo Croce
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

To exercise CO-RE in the kernel further convert map_ptr_kern
test to light skeleton.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/testing/selftests/bpf/Makefile             |  2 +-
 tools/testing/selftests/bpf/prog_tests/map_ptr.c | 16 +++++++---------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 811c5e150aa9..539a70b3b770 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -326,7 +326,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 \
-	kfunc_call_test_subprog.c
+	kfunc_call_test_subprog.c map_ptr_kern.c
 # Generate both light skeleton and libbpf skeleton for these
 LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c
 SKEL_BLACKLIST += $$(LSKELS)
diff --git a/tools/testing/selftests/bpf/prog_tests/map_ptr.c b/tools/testing/selftests/bpf/prog_tests/map_ptr.c
index 4972f92205c7..273725504f11 100644
--- a/tools/testing/selftests/bpf/prog_tests/map_ptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/map_ptr.c
@@ -4,31 +4,29 @@
 #include <test_progs.h>
 #include <network_helpers.h>
 
-#include "map_ptr_kern.skel.h"
+#include "map_ptr_kern.lskel.h"
 
 void test_map_ptr(void)
 {
-	struct map_ptr_kern *skel;
+	struct map_ptr_kern_lskel *skel;
 	__u32 duration = 0, retval;
 	char buf[128];
 	int err;
 	int page_size = getpagesize();
 
-	skel = map_ptr_kern__open();
+	skel = map_ptr_kern_lskel__open();
 	if (!ASSERT_OK_PTR(skel, "skel_open"))
 		return;
 
-	err = bpf_map__set_max_entries(skel->maps.m_ringbuf, page_size);
-	if (!ASSERT_OK(err, "bpf_map__set_max_entries"))
-		goto cleanup;
+	skel->maps.m_ringbuf.max_entries = page_size;
 
-	err = map_ptr_kern__load(skel);
+	err = map_ptr_kern_lskel__load(skel);
 	if (!ASSERT_OK(err, "skel_load"))
 		goto cleanup;
 
 	skel->bss->page_size = page_size;
 
-	err = bpf_prog_test_run(bpf_program__fd(skel->progs.cg_skb), 1, &pkt_v4,
+	err = bpf_prog_test_run(skel->progs.cg_skb.prog_fd, 1, &pkt_v4,
 				sizeof(pkt_v4), buf, NULL, &retval, NULL);
 
 	if (CHECK(err, "test_run", "err=%d errno=%d\n", err, errno))
@@ -39,5 +37,5 @@ void test_map_ptr(void)
 		goto cleanup;
 
 cleanup:
-	map_ptr_kern__destroy(skel);
+	map_ptr_kern_lskel__destroy(skel);
 }
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 bpf-next 12/12] selftests/bpf: Additional test for CO-RE in the kernel.
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
                   ` (10 preceding siblings ...)
  2021-11-12  5:02 ` [PATCH v2 bpf-next 11/12] selftests/bpf: Convert map_ptr_kern test to use light skeleton Alexei Starovoitov
@ 2021-11-12  5:02 ` Alexei Starovoitov
  2021-11-17  4:18   ` Andrii Nakryiko
  2021-11-17  0:48 ` [PATCH v2 bpf-next 00/12] bpf: CO-RE support " Matteo Croce
  12 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-12  5:02 UTC (permalink / raw)
  To: davem; +Cc: daniel, andrii, bpf, kernel-team

From: Alexei Starovoitov <ast@kernel.org>

Additional test where randmap() function is appended to three different bpf
programs. That action checks struct bpf_core_relo replication logic and offset
adjustment.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/testing/selftests/bpf/Makefile          |  2 +-
 .../selftests/bpf/prog_tests/core_kern.c      | 21 +++++++
 tools/testing/selftests/bpf/progs/core_kern.c | 60 +++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/core_kern.c
 create mode 100644 tools/testing/selftests/bpf/progs/core_kern.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 539a70b3b770..df6a9865b3b5 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -326,7 +326,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 \
-	kfunc_call_test_subprog.c map_ptr_kern.c
+	kfunc_call_test_subprog.c map_ptr_kern.c core_kern.c
 # Generate both light skeleton and libbpf skeleton for these
 LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c
 SKEL_BLACKLIST += $$(LSKELS)
diff --git a/tools/testing/selftests/bpf/prog_tests/core_kern.c b/tools/testing/selftests/bpf/prog_tests/core_kern.c
new file mode 100644
index 000000000000..f64843c5728c
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/core_kern.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+
+#include "test_progs.h"
+#include "core_kern.lskel.h"
+
+void test_core_kern_lskel(void)
+{
+	struct core_kern_lskel *skel;
+	int err;
+
+	skel = core_kern_lskel__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		goto cleanup;
+
+	err = core_kern_lskel__attach(skel);
+	if (!ASSERT_OK(err, "attach"))
+		goto cleanup;
+cleanup:
+	core_kern_lskel__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/core_kern.c b/tools/testing/selftests/bpf/progs/core_kern.c
new file mode 100644
index 000000000000..5ec308aac9b5
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/core_kern.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+#include "vmlinux.h"
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, u32);
+	__type(value, u32);
+	__uint(max_entries, 256);
+} array1 SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, u32);
+	__type(value, u32);
+	__uint(max_entries, 256);
+} array2 SEC(".maps");
+
+int randmap(int v)
+{
+	struct bpf_map *map = (struct bpf_map *)&array1;
+	int key = bpf_get_prandom_u32() & 0xff;
+	int *val;
+
+	if (bpf_get_prandom_u32() & 1)
+		map = (struct bpf_map *)&array2;
+
+	val = bpf_map_lookup_elem(map, &key);
+	if (val)
+		*val = bpf_get_prandom_u32() + v;
+
+	return 0;
+}
+
+SEC("tp_btf/xdp_devmap_xmit")
+int BPF_PROG(tp_xdp_devmap_xmit_multi, const struct net_device
+	     *from_dev, const struct net_device *to_dev, int sent, int drops,
+	     int err)
+{
+        return randmap(from_dev->ifindex);
+}
+
+SEC("fentry/eth_type_trans")
+int BPF_PROG(fentry_eth_type_trans, struct sk_buff *skb,
+	     struct net_device *dev, unsigned short protocol)
+{
+	return randmap(dev->ifindex + skb->len);
+}
+
+SEC("fexit/eth_type_trans")
+int BPF_PROG(fexit_eth_type_trans, struct sk_buff *skb,
+	     struct net_device *dev, unsigned short protocol)
+{
+	return randmap(dev->ifindex + skb->len);
+}
+
+char LICENSE[] SEC("license") = "GPL";
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 01/12] libbpf: s/btf__type_by_id/btf_type_by_id/.
  2021-11-12  5:02 ` [PATCH v2 bpf-next 01/12] libbpf: s/btf__type_by_id/btf_type_by_id/ Alexei Starovoitov
@ 2021-11-17  0:32   ` Andrii Nakryiko
  0 siblings, 0 replies; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  0:32 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> To prepare relo_core.c to be compiled in the kernel and the user space
> replace btf__type_by_id with btf_type_by_id.
>
> In libbpf btf__type_by_id and btf_type_by_id have different behavior.
>
> bpf_core_apply_relo_insn() needs behavior of uapi btf__type_by_id
> vs internal btf_type_by_id, but type_id range check is already done
> in bpf_core_apply_relo(), so it's safe to replace it everywhere.
> The kernel btf_type_by_id() does the check anyway. It doesn't hurt.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---


A bit of obnoxiousness. When applied locally with pw-apply, the
subject of this patch looks like this:

eddb6c529562 libbpf: S/btf__type_by_id/btf_type_by_id/

Quite weird. More human-readable "Replace btf__type_by_id() with
btf_type_by_id()" reads so much better ;)

>  tools/lib/bpf/btf.c             |  2 +-
>  tools/lib/bpf/libbpf_internal.h |  2 +-
>  tools/lib/bpf/relo_core.c       | 16 ++++++++--------
>  3 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index fadf089ae8fe..aa0e0bbde697 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -454,7 +454,7 @@ const struct btf *btf__base_btf(const struct btf *btf)
>  }
>

[...]

>         type_id = spec->root_type_id;
> -       t = btf__type_by_id(spec->btf, type_id);
> +       t = btf_type_by_id(spec->btf, type_id);
>         s = btf__name_by_offset(spec->btf, t->name_off);
>
>         libbpf_print(level, "[%u] %s %s", type_id, btf_kind_str(t), str_is_empty(s) ? "<anon>" : s);
> @@ -1158,7 +1158,7 @@ int bpf_core_apply_relo_insn(const char *prog_name, struct bpf_insn *insn,
>         int i, j, err;
>
>         local_id = relo->type_id;
> -       local_type = btf__type_by_id(local_btf, local_id);
> +       local_type = btf_type_by_id(local_btf, local_id);
>         if (!local_type)
>                 return -EINVAL;

Please remove this check, it's meaningless with the use of
btf_type_by_id() and you already justified why it's not necessary.

>
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 02/12] bpf: Rename btf_member accessors.
  2021-11-12  5:02 ` [PATCH v2 bpf-next 02/12] bpf: Rename btf_member accessors Alexei Starovoitov
@ 2021-11-17  0:38   ` Andrii Nakryiko
  2021-11-19  3:17     ` Alexei Starovoitov
  0 siblings, 1 reply; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  0:38 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Rename btf_member_bit_offset() and btf_member_bitfield_size() to
> avoid conflicts with similarly named helpers in libbpf's btf.h.
> Rename the kernel helpers, since libbpf helpers are part of uapi.
>
> Suggested-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---

See below, I'd get rid of those "almost duplicates" instead, but up to
you, I'm fine either way.

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

>  include/linux/btf.h         |  8 ++++----
>  kernel/bpf/bpf_struct_ops.c |  6 +++---
>  kernel/bpf/btf.c            | 18 +++++++++---------
>  net/ipv4/bpf_tcp_ca.c       |  6 +++---
>  4 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 203eef993d76..956f70388f69 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -194,15 +194,15 @@ static inline bool btf_type_kflag(const struct btf_type *t)
>         return BTF_INFO_KFLAG(t->info);
>  }
>
> -static inline u32 btf_member_bit_offset(const struct btf_type *struct_type,
> -                                       const struct btf_member *member)
> +static inline u32 __btf_member_bit_offset(const struct btf_type *struct_type,
> +                                         const struct btf_member *member)

a bit surprised you didn't choose to just remove them, given you had
to touch all 24 places in the kernel that call this helper

>  {
>         return btf_type_kflag(struct_type) ? BTF_MEMBER_BIT_OFFSET(member->offset)
>                                            : member->offset;
>  }
>
> -static inline u32 btf_member_bitfield_size(const struct btf_type *struct_type,
> -                                          const struct btf_member *member)
> +static inline u32 __btf_member_bitfield_size(const struct btf_type *struct_type,
> +                                            const struct btf_member *member)
>  {
>         return btf_type_kflag(struct_type) ? BTF_MEMBER_BITFIELD_SIZE(member->offset)
>                                            : 0;
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 8ecfe4752769..21069dbe9138 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -165,7 +165,7 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>                                 break;
>                         }
>
> -                       if (btf_member_bitfield_size(t, member)) {
> +                       if (__btf_member_bitfield_size(t, member)) {

like in this case it would be btf_member_bitfield_size(t, j)


>                                 pr_warn("bit field member %s in struct %s is not supported\n",
>                                         mname, st_ops->name);
>                                 break;
> @@ -296,7 +296,7 @@ static int check_zero_holes(const struct btf_type *t, void *data)
>         const struct btf_type *mtype;
>
>         for_each_member(i, t, member) {
> -               moff = btf_member_bit_offset(t, member) / 8;
> +               moff = __btf_member_bit_offset(t, member) / 8;

same here, seema like in all the cases we already have member_idx (i
in this case)

>                 if (moff > prev_mend &&
>                     memchr_inv(data + prev_mend, 0, moff - prev_mend))
>                         return -EINVAL;

[...]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel
  2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
                   ` (11 preceding siblings ...)
  2021-11-12  5:02 ` [PATCH v2 bpf-next 12/12] selftests/bpf: Additional test for CO-RE in the kernel Alexei Starovoitov
@ 2021-11-17  0:48 ` Matteo Croce
  2021-11-17  4:34   ` Andrii Nakryiko
  12 siblings, 1 reply; 38+ messages in thread
From: Matteo Croce @ 2021-11-17  0:48 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Fri, Nov 12, 2021 at 6:02 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> v1->v2:
> . Refactor uapi to pass 'struct bpf_core_relo' from LLVM into libbpf and further
> into the kernel instead of bpf_core_apply_relo() bpf helper. Because of this
> change the CO-RE algorithm has an ability to log error and debug events through
> the standard bpf verifer log mechanism which was not possible with helper
> approach.
> . #define RELO_CORE macro was removed and replaced with btf_member_bit_offset() patch.
>
> This set introduces CO-RE support in the kernel.
> There are several reasons to add such support:
> 1. It's a step toward signed BPF programs.
> 2. It allows golang like languages that struggle to adopt libbpf
>    to take advantage of CO-RE powers.
> 3. Currently the field accessed by 'ldx [R1 + 10]' insn is recognized
>    by the verifier purely based on +10 offset. If R1 points to a union
>    the verifier picks one of the fields at this offset.
>    With CO-RE the kernel can disambiguate the field access.
>

Great, I tested the same code which was failing with the RFC series,
now there isn't any error.
This is the output with pr_debug() enabled:

root@debian64:~/core# ./core
[    5.690268] prog '(null)': relo #-2115894237: kind <(null)>
(163299788), spec is
[    5.690272] prog '(null)': relo #-2115894246: (null) candidate #-2115185528
[    5.690392] prog '(null)': relo #2: patched insn #208 (LDX/ST/STX)
off 208 -> 208
[    5.691045] prog '(efault)': relo #-2115894237: kind <(null)>
(163299788), spec is
[    5.691047] prog '(efault)': relo #-2115894246: (null) candidate
#-2115185528
[    5.691148] prog '(efault)': relo #3: patched insn #208
(LDX/ST/STX) off 208 -> 208
[    5.692456] prog '(null)': relo #-2115894237: kind <(null)>
(163302708), spec is
[    5.692459] prog '(null)': relo #-2115894246: (null) candidate #-2115185668
[    5.692564] prog '(null)': relo #2: patched insn #104 (LDX/ST/STX)
off 104 -> 104
[    5.693179] prog '(efault)': relo #-2115894237: kind <(null)>
(163299788), spec is
[    5.693181] prog '(efault)': relo #-2115894246: (null) candidate
#-2115185528
[    5.693258] prog '(efault)': relo #3: patched insn #208
(LDX/ST/STX) off 208 -> 208
[    5.696141] prog '(null)': relo #-2115894237: kind <(null)>
(163302708), spec is
[    5.696143] prog '(null)': relo #-2115894246: (null) candidate #-2115185668
[    5.696255] prog '(null)': relo #2: patched insn #104 (LDX/ST/STX)
off 104 -> 104
[    5.696733] prog '(efault)': relo #-2115894237: kind <(null)>
(163299788), spec is
[    5.696734] prog '(efault)': relo #-2115894246: (null) candidate
#-2115185528
[    5.696833] prog '(efault)': relo #3: patched insn #208
(LDX/ST/STX) off 208 -> 208

And the syscall returns success:

bpf(BPF_PROG_TEST_RUN, {test={prog_fd=4, retval=0, data_size_in=0,
data_size_out=0, data_in=NULL, data_out=NULL, repeat=0, duration=0,
ctx_size_in=68, ctx_size_out=0, ctx_in=0x5590b97dd2a0, ctx_out=NULL}},
160) = 0

Regards,
-- 
per aspera ad upstream

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 03/12] bpf: Prepare relo_core.c for kernel duty.
  2021-11-12  5:02 ` [PATCH v2 bpf-next 03/12] bpf: Prepare relo_core.c for kernel duty Alexei Starovoitov
@ 2021-11-17  0:58   ` Andrii Nakryiko
  2021-11-19  3:20     ` Alexei Starovoitov
  0 siblings, 1 reply; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  0:58 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Make relo_core.c to be compiled with kernel and with libbpf.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  include/linux/btf.h       | 82 +++++++++++++++++++++++++++++++++++++++
>  kernel/bpf/Makefile       |  4 ++
>  kernel/bpf/btf.c          | 26 +++++++++++++
>  tools/lib/bpf/relo_core.c | 71 ++++++++++++++++++++++++++++-----
>  4 files changed, 174 insertions(+), 9 deletions(-)
>

[...]

>  static inline const struct btf_member *btf_type_member(const struct btf_type *t)
>  {
>         return (const struct btf_member *)(t + 1);
>  }
>
> +

accidental empty line or intentional?

> +static inline struct btf_array *btf_array(const struct btf_type *t)
> +{
> +       return (struct btf_array *)(t + 1);
> +}
> +

[...]

> +int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id,
> +                             const struct btf *targ_btf, __u32 targ_id)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
> +static bool bpf_core_is_flavor_sep(const char *s)
> +{
> +       /* check X___Y name pattern, where X and Y are not underscores */
> +       return s[0] != '_' &&                                 /* X */
> +              s[1] == '_' && s[2] == '_' && s[3] == '_' &&   /* ___ */
> +              s[4] != '_';                                   /* Y */
> +}
> +
> +size_t bpf_core_essential_name_len(const char *name)

I might have missed something, but this seems to be used only
internally, so should be static. Otherwise there would be a
compilation warning due to the missing prototype, no?

> +{
> +       size_t n = strlen(name);
> +       int i;
> +
> +       for (i = n - 5; i >= 0; i--) {
> +               if (bpf_core_is_flavor_sep(name + i))
> +                       return i + 1;
> +       }
> +       return n;
> +}

[...]

> +       t = btf_type_by_id(btf, type_id);
> +       t = btf_resolve_size(btf, t, &size);
> +       if (IS_ERR(t))
> +               return PTR_ERR(t);
> +       return size;
> +}

nit: empty line would be good here and after enum

> +enum libbpf_print_level {
> +       LIBBPF_WARN,
> +       LIBBPF_INFO,
> +       LIBBPF_DEBUG,
> +};
> +#undef pr_warn
> +#undef pr_info
> +#undef pr_debug
> +#define pr_warn(fmt, log, ...) bpf_log((void *)log, fmt, "", ##__VA_ARGS__)
> +#define pr_info(fmt, log, ...) bpf_log((void *)log, fmt, "", ##__VA_ARGS__)
> +#define pr_debug(fmt, log, ...)        bpf_log((void *)log, fmt, "", ##__VA_ARGS__)
> +#define libbpf_print(level, fmt, ...)  bpf_log((void *)prog_name, fmt, ##__VA_ARGS__)
> +#else
>  #include <stdio.h>
>  #include <string.h>
>  #include <errno.h>
> @@ -12,8 +64,9 @@
>  #include "btf.h"
>  #include "str_error.h"
>  #include "libbpf_internal.h"
> +#endif
>
> -#define BPF_CORE_SPEC_MAX_LEN 64
> +#define BPF_CORE_SPEC_MAX_LEN 32

This is worth calling out in the commit description, should have
practical implications, but good to mention.

>
>  /* represents BPF CO-RE field or array element accessor */
>  struct bpf_core_accessor {
> @@ -272,8 +325,8 @@ static int bpf_core_parse_spec(const struct btf *btf,
>                                 return sz;
>                         spec->bit_offset += access_idx * sz * 8;
>                 } else {
> -                       pr_warn("relo for [%u] %s (at idx %d) captures type [%d] of unexpected kind %s\n",
> -                               type_id, spec_str, i, id, btf_kind_str(t));
> +/*                     pr_warn("relo for [%u] %s (at idx %d) captures type [%d] of unexpected kind %s\n",
> +                               type_id, spec_str, i, id, btf_kind_str(t));*/

we can totally pass prog_name and add "prog '%s': " to uncomment this.
bpf_core_parse_spec() is called in the "context" of program, so it's
known

>                         return -EINVAL;
>                 }
>         }
> @@ -346,8 +399,8 @@ static int bpf_core_fields_are_compat(const struct btf *local_btf,
>                 targ_id = btf_array(targ_type)->type;
>                 goto recur;
>         default:
> -               pr_warn("unexpected kind %d relocated, local [%d], target [%d]\n",
> -                       btf_kind(local_type), local_id, targ_id);
> +/*             pr_warn("unexpected kind %d relocated, local [%d], target [%d]\n",
> +                       btf_kind(local_type), local_id, targ_id);*/

sigh... it's a bit too intrusive to pass prog_name here but it's also
highly unlikely that this happens (unless some compiler bug or
corruption) and even in that case it's semantically correct that
fields just don't match. So I'd just drop this pr_warn() instead of
commenting it out

>                 return 0;
>         }
>  }

[...]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 04/12] bpf: Define enum bpf_core_relo_kind as uapi.
  2021-11-12  5:02 ` [PATCH v2 bpf-next 04/12] bpf: Define enum bpf_core_relo_kind as uapi Alexei Starovoitov
@ 2021-11-17  1:04   ` Andrii Nakryiko
  0 siblings, 0 replies; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  1:04 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> enum bpf_core_relo_kind is generated by llvm and processed by libbpf.
> It's a de-facto uapi.
> With CO-RE in the kernel the bpf_core_relo_kind values become uapi de-jure.
> Also rename them with BPF_CORE_ prefix to distinguish from conflicting names in
> bpf_core_read.h. The enums bpf_field_info_kind, bpf_type_id_kind,
> bpf_type_info_kind, bpf_enum_value_kind are passing different values from bpf
> program into llvm.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  include/uapi/linux/bpf.h       | 19 ++++++++
>  tools/include/uapi/linux/bpf.h | 19 ++++++++
>  tools/lib/bpf/libbpf.c         |  2 +-
>  tools/lib/bpf/relo_core.c      | 84 +++++++++++++++++-----------------
>  tools/lib/bpf/relo_core.h      | 18 --------
>  5 files changed, 81 insertions(+), 61 deletions(-)
>

[...]

> index 3b9f8f18346c..7720af11f96f 100644
> --- a/tools/lib/bpf/relo_core.h
> +++ b/tools/lib/bpf/relo_core.h
> @@ -4,24 +4,6 @@
>  #ifndef __RELO_CORE_H
>  #define __RELO_CORE_H

Can you please add #include <linux/bpf.h> here for enum
bpf_core_relo_kind? It should be fine for both libbpf and kernel
modes. Implicit #include dependency is very confusing.

>
> -/* bpf_core_relo_kind encodes which aspect of captured field/type/enum value
> - * has to be adjusted by relocations.
> - */
> -enum bpf_core_relo_kind {
> -       BPF_FIELD_BYTE_OFFSET = 0,      /* field byte offset */
> -       BPF_FIELD_BYTE_SIZE = 1,        /* field size in bytes */
> -       BPF_FIELD_EXISTS = 2,           /* field existence in target kernel */
> -       BPF_FIELD_SIGNED = 3,           /* field signedness (0 - unsigned, 1 - signed) */
> -       BPF_FIELD_LSHIFT_U64 = 4,       /* bitfield-specific left bitshift */
> -       BPF_FIELD_RSHIFT_U64 = 5,       /* bitfield-specific right bitshift */
> -       BPF_TYPE_ID_LOCAL = 6,          /* type ID in local BPF object */
> -       BPF_TYPE_ID_TARGET = 7,         /* type ID in target kernel */
> -       BPF_TYPE_EXISTS = 8,            /* type existence in target kernel */
> -       BPF_TYPE_SIZE = 9,              /* type size in bytes */
> -       BPF_ENUMVAL_EXISTS = 10,        /* enum value existence in target kernel */
> -       BPF_ENUMVAL_VALUE = 11,         /* enum value integer value */
> -};
> -
>  /* The minimum bpf_core_relo checked by the loader
>   *
>   * CO-RE relocation captures the following data:
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 05/12] bpf: Pass a set of bpf_core_relo-s to prog_load command.
  2021-11-12  5:02 ` [PATCH v2 bpf-next 05/12] bpf: Pass a set of bpf_core_relo-s to prog_load command Alexei Starovoitov
@ 2021-11-17  1:17   ` Andrii Nakryiko
  2021-11-19  3:32     ` Alexei Starovoitov
  0 siblings, 1 reply; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  1:17 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> struct bpf_core_relo is generated by llvm and processed by libbpf.
> It's a de-facto uapi.
> With CO-RE in the kernel the struct bpf_core_relo becomes uapi de-jure.
> Add an ability to pass a set of 'struct bpf_core_relo' to prog_load command
> and let the kernel perform CO-RE relocations.
>
> Note the struct bpf_line_info and struct bpf_func_info have the same
> layout when passed from LLVM to libbpf and from libbpf to the kernel
> except "insn_off" fields means "byte offset" when LLVM generates it.
> Then libbpf converts it to "insn index" to pass to the kernel.
> The struct bpf_core_relo's "insn_off" field is always "byte offset".
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  include/linux/bpf.h            |  3 ++
>  include/uapi/linux/bpf.h       | 59 +++++++++++++++++++++++++++-
>  kernel/bpf/btf.c               |  7 ++++
>  kernel/bpf/syscall.c           |  2 +-
>  kernel/bpf/verifier.c          | 71 ++++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h | 59 +++++++++++++++++++++++++++-
>  tools/lib/bpf/relo_core.h      | 53 -------------------------
>  7 files changed, 198 insertions(+), 56 deletions(-)
>

[...]

>  static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)
>  {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1aafb43f61d1..c2246414e182 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10268,6 +10268,73 @@ static int check_btf_line(struct bpf_verifier_env *env,
>         return err;
>  }
>
> +static int check_core_relo(struct bpf_verifier_env *env,
> +                          const union bpf_attr *attr,
> +                          bpfptr_t uattr)
> +{
> +       u32 i, nr_core_relo, ncopy, expected_size, rec_size;
> +       struct bpf_core_relo core_relo = {};
> +       struct bpf_prog *prog = env->prog;
> +       const struct btf *btf = prog->aux->btf;
> +       bpfptr_t u_core_relo;
> +       int err;
> +
> +       nr_core_relo = attr->core_relo_cnt;
> +       if (!nr_core_relo)
> +               return 0;
> +       if (nr_core_relo > INT_MAX / sizeof(struct bpf_core_relo))
> +               return -EINVAL;
> +
> +       rec_size = attr->core_relo_rec_size;
> +       if (rec_size != sizeof(struct bpf_core_relo))
> +               return -EINVAL;

For func_info we allow trailing zeroes (in check_btf_func, we check
MIN_BPF_FUNCINFO_SIZE and MAX_FUNCINFO_REC_SIZE). Shouldn't we do
something like that here?

> +
> +       u_core_relo = make_bpfptr(attr->core_relo, uattr.is_kernel);
> +       expected_size = sizeof(struct bpf_core_relo);
> +       ncopy = min_t(u32, expected_size, rec_size);

I'm confused, a few lines above you errored out if expected_size != rec_size...

> +
> +       /* Unlike func_info and line_info, copy and apply each CO-RE
> +        * relocation record one at a time
> +        */
> +       for (i = 0; i < nr_core_relo; i++) {
> +               /* future proofing when sizeof(bpf_core_relo) changes */
> +               err = bpf_check_uarg_tail_zero(u_core_relo, expected_size, rec_size);
> +               if (err) {
> +                       if (err == -E2BIG) {
> +                               verbose(env, "nonzero tailing record in core_relo");
> +                               if (copy_to_bpfptr_offset(uattr,
> +                                                         offsetof(union bpf_attr, core_relo_rec_size),
> +                                                         &expected_size, sizeof(expected_size)))
> +                                       err = -EFAULT;
> +                       }
> +                       goto err;
> +               }
> +
> +               if (copy_from_bpfptr(&core_relo, u_core_relo, ncopy)) {
> +                       err = -EFAULT;
> +                       goto err;
> +               }
> +
> +               if (core_relo.insn_off % 8 || core_relo.insn_off / 8 >= prog->len) {
> +                       verbose(env, "Invalid core_relo[%u].insn_off:%u prog->len:%u\n",
> +                               i, core_relo.insn_off, prog->len);
> +                       err = -EINVAL;
> +                       goto err;
> +               }
> +
> +               err = bpf_core_relo_apply(&env->log, btf, &core_relo, i,
> +                                         &prog->insnsi[core_relo.insn_off / 8]);
> +               if (err)
> +                       goto err;
> +               bpfptr_add(&u_core_relo, rec_size);
> +       }
> +
> +       return 0;

don't need this return if you initialize err = 0 at the beginning?

> +
> +err:
> +       return err;
> +}
> +
>  static int check_btf_info(struct bpf_verifier_env *env,
>                           const union bpf_attr *attr,
>                           bpfptr_t uattr)

[...]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 06/12] bpf: Add bpf_core_add_cands() and wire it into bpf_core_apply_relo_insn().
  2021-11-12  5:02 ` [PATCH v2 bpf-next 06/12] bpf: Add bpf_core_add_cands() and wire it into bpf_core_apply_relo_insn() Alexei Starovoitov
@ 2021-11-17  1:31   ` Andrii Nakryiko
  2021-11-19  3:34     ` Alexei Starovoitov
  0 siblings, 1 reply; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  1:31 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Given BPF program's BTF perform a linear search through kernel BTFs for
> a possible candidate.
> Then wire the result into bpf_core_apply_relo_insn().
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  kernel/bpf/btf.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 137 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index efb7fa2f81a2..aeb591579282 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -25,6 +25,7 @@
>  #include <linux/kobject.h>
>  #include <linux/sysfs.h>
>  #include <net/sock.h>
> +#include "../tools/lib/bpf/relo_core.h"
>
>  /* BTF (BPF Type Format) is the meta data format which describes
>   * the data types of BPF program/map.  Hence, it basically focus
> @@ -6440,9 +6441,144 @@ size_t bpf_core_essential_name_len(const char *name)
>         return n;
>  }
>
> +static void bpf_core_free_cands(struct bpf_core_cand_list *cands)
> +{
> +       if (!cands)
> +               return;
> +        kfree(cands->cands);
> +        kfree(cands);

indentation is off?

> +}
> +
> +static int bpf_core_add_cands(struct bpf_verifier_log *log,
> +                             struct bpf_core_cand *local_cand,

and here?

> +                              size_t local_essent_len,
> +                              const struct btf *targ_btf,
> +                              int targ_start_id,
> +                              struct bpf_core_cand_list *cands)
> +{
> +       struct bpf_core_cand *new_cands, *cand;
> +       const struct btf_type *t;
> +       const char *targ_name;
> +       size_t targ_essent_len;
> +       int n, i;
> +

[...]

>  int bpf_core_relo_apply(struct bpf_verifier_log *log, const struct btf *btf,
>                         const struct bpf_core_relo *relo, int relo_idx,
>                         void *insn)
>  {
> -       return -EOPNOTSUPP;
> +       struct bpf_core_cand_list *cands = NULL;
> +       int err;
> +
> +       if (relo->kind != BPF_CORE_TYPE_ID_LOCAL) {
> +               cands = bpf_core_find_cands(log, btf, relo->type_id);
> +               if (IS_ERR(cands)) {
> +                       bpf_log(log, "target candidate search failed for %d\n",
> +                              relo->type_id);
> +                        return PTR_ERR(cands);

some indentation issues here as well

> +                }
> +       }
> +       err = bpf_core_apply_relo_insn((void *)log, insn, relo->insn_off / 8,
> +                                      relo, relo_idx, btf, cands);
> +       bpf_core_free_cands(cands);

Why did you decide to not persist the candidate list? It is a
significant slowdown even on moderately large BPF programs, as you are
linearly re-searching vmlinux BTF multiple times for the same root
type.

> +       return err;
>  }
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 07/12] libbpf: Use CO-RE in the kernel in light skeleton.
  2021-11-12  5:02 ` [PATCH v2 bpf-next 07/12] libbpf: Use CO-RE in the kernel in light skeleton Alexei Starovoitov
@ 2021-11-17  3:45   ` Andrii Nakryiko
  2021-11-19  3:57     ` Alexei Starovoitov
  0 siblings, 1 reply; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  3:45 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Without lskel the CO-RE relocations are processed by libbpf before any other
> work is done. Instead, when lksel is needed, remember relocation as RELO_CORE

typo: lskel

> kind. Then when loader prog is generated for a given bpf program pass CO-RE
> relos of that program to gen loader via bpf_gen__record_relo_core(). The gen
> loader will remember them as-is and pass it later as-is into the kernel.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  tools/lib/bpf/bpf_gen_internal.h |   3 +
>  tools/lib/bpf/gen_loader.c       |  41 +++++++++++-
>  tools/lib/bpf/libbpf.c           | 104 +++++++++++++++++++++++--------
>  3 files changed, 119 insertions(+), 29 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_gen_internal.h b/tools/lib/bpf/bpf_gen_internal.h
> index 75ca9fb857b2..ed162fdeecf6 100644
> --- a/tools/lib/bpf/bpf_gen_internal.h
> +++ b/tools/lib/bpf/bpf_gen_internal.h
> @@ -39,6 +39,8 @@ struct bpf_gen {
>         int error;
>         struct ksym_relo_desc *relos;
>         int relo_cnt;
> +       struct bpf_core_relo *core_relo;

this is named as a singular pointer to one relocation, core_relos
would be a more natural name for an array?


> +       int core_relo_cnt;
>         char attach_target[128];
>         int attach_kind;
>         struct ksym_desc *ksyms;
> @@ -61,5 +63,6 @@ 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,
>                             bool is_typeless, int kind, int insn_idx);

[...]

> @@ -6581,6 +6623,16 @@ static int bpf_program__record_externs(struct bpf_program *prog)
>                                                ext->is_weak, false, BTF_KIND_FUNC,
>                                                relo->insn_idx);
>                         break;
> +               case RELO_CORE: {

This is not an extern, it doesn't make sense to have it here. But I
also don't understand why we need to add RELO_CORE and extend struct
relo_desc in the first place, just to pass it as bpf_core_relo into
gen_loader. Why can't gen_loader just record this directly at the
place of record_relo_core() call?

> +                       struct bpf_core_relo cr = {
> +                               .insn_off = relo->insn_idx * 8,
> +                               .type_id = relo->type_id,
> +                               .access_str_off = relo->access_str_off,
> +                               .kind = relo->kind,
> +                       };
> +                       bpf_gen__record_relo_core(obj->gen_loader, &cr);
> +                       break;
> +               }
>                 default:
>                         continue;
>                 }
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 08/12] libbpf: Support init of inner maps in light skeleton.
  2021-11-12  5:02 ` [PATCH v2 bpf-next 08/12] libbpf: Support init of inner maps " Alexei Starovoitov
@ 2021-11-17  4:10   ` Andrii Nakryiko
  0 siblings, 0 replies; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  4:10 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Add ability to initialize inner maps in light skeleton.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---

LGTM.

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

>  tools/lib/bpf/bpf_gen_internal.h |  1 +
>  tools/lib/bpf/gen_loader.c       | 27 +++++++++++++++++++++++++++
>  tools/lib/bpf/libbpf.c           |  6 +++---
>  3 files changed, 31 insertions(+), 3 deletions(-)
>

[...]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 09/12] selftests/bpf: Convert kfunc test with CO-RE to lskel.
  2021-11-12  5:02 ` [PATCH v2 bpf-next 09/12] selftests/bpf: Convert kfunc test with CO-RE to lskel Alexei Starovoitov
@ 2021-11-17  4:13   ` Andrii Nakryiko
  0 siblings, 0 replies; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  4:13 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Convert kfunc_call_test_subprog to light skeleton.
>

Let's keep the libbpf variant of the test as well. Both kinds of
skeletons can co-exist, we already have tests like that that test the
same functionality in both modes.

> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  tools/testing/selftests/bpf/Makefile                |  3 ++-
>  tools/testing/selftests/bpf/prog_tests/kfunc_call.c | 10 +++++-----
>  2 files changed, 7 insertions(+), 6 deletions(-)
>

[...]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 10/12] selftests/bpf: Improve inner_map test coverage.
  2021-11-12  5:02 ` [PATCH v2 bpf-next 10/12] selftests/bpf: Improve inner_map test coverage Alexei Starovoitov
@ 2021-11-17  4:15   ` Andrii Nakryiko
  0 siblings, 0 replies; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  4:15 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:03 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Check that hash and array inner maps are properly initialized.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---

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

>  tools/testing/selftests/bpf/progs/map_ptr_kern.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>

[...]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 11/12] selftests/bpf: Convert map_ptr_kern test to use light skeleton.
  2021-11-12  5:02 ` [PATCH v2 bpf-next 11/12] selftests/bpf: Convert map_ptr_kern test to use light skeleton Alexei Starovoitov
@ 2021-11-17  4:16   ` Andrii Nakryiko
  0 siblings, 0 replies; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  4:16 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:03 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> To exercise CO-RE in the kernel further convert map_ptr_kern
> test to light skeleton.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---

We have plenty of CO-RE tests for libbpf mode, so it's fine to convert this one.

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

>  tools/testing/selftests/bpf/Makefile             |  2 +-
>  tools/testing/selftests/bpf/prog_tests/map_ptr.c | 16 +++++++---------
>  2 files changed, 8 insertions(+), 10 deletions(-)
>

[...]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 12/12] selftests/bpf: Additional test for CO-RE in the kernel.
  2021-11-12  5:02 ` [PATCH v2 bpf-next 12/12] selftests/bpf: Additional test for CO-RE in the kernel Alexei Starovoitov
@ 2021-11-17  4:18   ` Andrii Nakryiko
  2021-11-19  4:00     ` Alexei Starovoitov
  0 siblings, 1 reply; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  4:18 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 11, 2021 at 9:03 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> Additional test where randmap() function is appended to three different bpf
> programs. That action checks struct bpf_core_relo replication logic and offset
> adjustment.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  tools/testing/selftests/bpf/Makefile          |  2 +-
>  .../selftests/bpf/prog_tests/core_kern.c      | 21 +++++++
>  tools/testing/selftests/bpf/progs/core_kern.c | 60 +++++++++++++++++++
>  3 files changed, 82 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/core_kern.c
>  create mode 100644 tools/testing/selftests/bpf/progs/core_kern.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 539a70b3b770..df6a9865b3b5 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -326,7 +326,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 \
> -       kfunc_call_test_subprog.c map_ptr_kern.c
> +       kfunc_call_test_subprog.c map_ptr_kern.c core_kern.c
>  # Generate both light skeleton and libbpf skeleton for these
>  LSKELS_EXTRA := test_ksyms_module.c test_ksyms_weak.c
>  SKEL_BLACKLIST += $$(LSKELS)
> diff --git a/tools/testing/selftests/bpf/prog_tests/core_kern.c b/tools/testing/selftests/bpf/prog_tests/core_kern.c
> new file mode 100644
> index 000000000000..f64843c5728c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/core_kern.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2021 Facebook */
> +
> +#include "test_progs.h"
> +#include "core_kern.lskel.h"
> +
> +void test_core_kern_lskel(void)
> +{
> +       struct core_kern_lskel *skel;
> +       int err;
> +
> +       skel = core_kern_lskel__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "open_and_load"))
> +               goto cleanup;
> +
> +       err = core_kern_lskel__attach(skel);

Why attaching if you are never triggering it? This test is about
testing load, right? Let's drop the attach then.

> +       if (!ASSERT_OK(err, "attach"))
> +               goto cleanup;
> +cleanup:
> +       core_kern_lskel__destroy(skel);
> +}

[...]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel
  2021-11-17  0:48 ` [PATCH v2 bpf-next 00/12] bpf: CO-RE support " Matteo Croce
@ 2021-11-17  4:34   ` Andrii Nakryiko
  2021-11-17 15:28     ` Matteo Croce
  0 siblings, 1 reply; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-17  4:34 UTC (permalink / raw)
  To: Matteo Croce
  Cc: Alexei Starovoitov, David Miller, Daniel Borkmann,
	Andrii Nakryiko, bpf, Kernel Team

On Tue, Nov 16, 2021 at 4:48 PM Matteo Croce <mcroce@linux.microsoft.com> wrote:
>
> On Fri, Nov 12, 2021 at 6:02 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > v1->v2:
> > . Refactor uapi to pass 'struct bpf_core_relo' from LLVM into libbpf and further
> > into the kernel instead of bpf_core_apply_relo() bpf helper. Because of this
> > change the CO-RE algorithm has an ability to log error and debug events through
> > the standard bpf verifer log mechanism which was not possible with helper
> > approach.
> > . #define RELO_CORE macro was removed and replaced with btf_member_bit_offset() patch.
> >
> > This set introduces CO-RE support in the kernel.
> > There are several reasons to add such support:
> > 1. It's a step toward signed BPF programs.
> > 2. It allows golang like languages that struggle to adopt libbpf
> >    to take advantage of CO-RE powers.
> > 3. Currently the field accessed by 'ldx [R1 + 10]' insn is recognized
> >    by the verifier purely based on +10 offset. If R1 points to a union
> >    the verifier picks one of the fields at this offset.
> >    With CO-RE the kernel can disambiguate the field access.
> >
>
> Great, I tested the same code which was failing with the RFC series,
> now there isn't any error.
> This is the output with pr_debug() enabled:
>
> root@debian64:~/core# ./core
> [    5.690268] prog '(null)': relo #-2115894237: kind <(null)>
> (163299788), spec is
> [    5.690272] prog '(null)': relo #-2115894246: (null) candidate #-2115185528
> [    5.690392] prog '(null)': relo #2: patched insn #208 (LDX/ST/STX)
> off 208 -> 208
> [    5.691045] prog '(efault)': relo #-2115894237: kind <(null)>
> (163299788), spec is
> [    5.691047] prog '(efault)': relo #-2115894246: (null) candidate
> #-2115185528
> [    5.691148] prog '(efault)': relo #3: patched insn #208
> (LDX/ST/STX) off 208 -> 208
> [    5.692456] prog '(null)': relo #-2115894237: kind <(null)>
> (163302708), spec is
> [    5.692459] prog '(null)': relo #-2115894246: (null) candidate #-2115185668
> [    5.692564] prog '(null)': relo #2: patched insn #104 (LDX/ST/STX)
> off 104 -> 104
> [    5.693179] prog '(efault)': relo #-2115894237: kind <(null)>
> (163299788), spec is
> [    5.693181] prog '(efault)': relo #-2115894246: (null) candidate
> #-2115185528
> [    5.693258] prog '(efault)': relo #3: patched insn #208
> (LDX/ST/STX) off 208 -> 208
> [    5.696141] prog '(null)': relo #-2115894237: kind <(null)>
> (163302708), spec is
> [    5.696143] prog '(null)': relo #-2115894246: (null) candidate #-2115185668
> [    5.696255] prog '(null)': relo #2: patched insn #104 (LDX/ST/STX)
> off 104 -> 104
> [    5.696733] prog '(efault)': relo #-2115894237: kind <(null)>
> (163299788), spec is
> [    5.696734] prog '(efault)': relo #-2115894246: (null) candidate
> #-2115185528
> [    5.696833] prog '(efault)': relo #3: patched insn #208
> (LDX/ST/STX) off 208 -> 208

All the logged values are completely wrong, some corruption somewhere.

But I tried to see it for myself and I couldn't figure out how to get
these logs with lskel. How did you get the above?

Alexei, any guidance on how to get those verifier logs back with
test_progs? ./test_progs -vvv didn't help, I also checked trace_pipe
output, it was empty. I thought that maybe verifier truncates logs on
success and simulated failed prog validation, but still nothing.

>
> And the syscall returns success:
>
> bpf(BPF_PROG_TEST_RUN, {test={prog_fd=4, retval=0, data_size_in=0,
> data_size_out=0, data_in=NULL, data_out=NULL, repeat=0, duration=0,
> ctx_size_in=68, ctx_size_out=0, ctx_in=0x5590b97dd2a0, ctx_out=NULL}},
> 160) = 0
>
> Regards,
> --
> per aspera ad upstream

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel
  2021-11-17  4:34   ` Andrii Nakryiko
@ 2021-11-17 15:28     ` Matteo Croce
  2021-11-17 17:29       ` Alexei Starovoitov
  0 siblings, 1 reply; 38+ messages in thread
From: Matteo Croce @ 2021-11-17 15:28 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Alexei Starovoitov, David Miller, Daniel Borkmann,
	Andrii Nakryiko, bpf, Kernel Team

On Wed, Nov 17, 2021 at 5:34 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Nov 16, 2021 at 4:48 PM Matteo Croce <mcroce@linux.microsoft.com> wrote:
> >
> > On Fri, Nov 12, 2021 at 6:02 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > From: Alexei Starovoitov <ast@kernel.org>
> > >
> > > v1->v2:
> > > . Refactor uapi to pass 'struct bpf_core_relo' from LLVM into libbpf and further
> > > into the kernel instead of bpf_core_apply_relo() bpf helper. Because of this
> > > change the CO-RE algorithm has an ability to log error and debug events through
> > > the standard bpf verifer log mechanism which was not possible with helper
> > > approach.
> > > . #define RELO_CORE macro was removed and replaced with btf_member_bit_offset() patch.
> > >
> > > This set introduces CO-RE support in the kernel.
> > > There are several reasons to add such support:
> > > 1. It's a step toward signed BPF programs.
> > > 2. It allows golang like languages that struggle to adopt libbpf
> > >    to take advantage of CO-RE powers.
> > > 3. Currently the field accessed by 'ldx [R1 + 10]' insn is recognized
> > >    by the verifier purely based on +10 offset. If R1 points to a union
> > >    the verifier picks one of the fields at this offset.
> > >    With CO-RE the kernel can disambiguate the field access.
> > >
> >
> > Great, I tested the same code which was failing with the RFC series,
> > now there isn't any error.
> > This is the output with pr_debug() enabled:
> >
> > root@debian64:~/core# ./core
> > [    5.690268] prog '(null)': relo #-2115894237: kind <(null)>
> > (163299788), spec is
> > [    5.690272] prog '(null)': relo #-2115894246: (null) candidate #-2115185528
> > [    5.690392] prog '(null)': relo #2: patched insn #208 (LDX/ST/STX)
> > off 208 -> 208
> > [    5.691045] prog '(efault)': relo #-2115894237: kind <(null)>
> > (163299788), spec is
> > [    5.691047] prog '(efault)': relo #-2115894246: (null) candidate
> > #-2115185528
> > [    5.691148] prog '(efault)': relo #3: patched insn #208
> > (LDX/ST/STX) off 208 -> 208
> > [    5.692456] prog '(null)': relo #-2115894237: kind <(null)>
> > (163302708), spec is
> > [    5.692459] prog '(null)': relo #-2115894246: (null) candidate #-2115185668
> > [    5.692564] prog '(null)': relo #2: patched insn #104 (LDX/ST/STX)
> > off 104 -> 104
> > [    5.693179] prog '(efault)': relo #-2115894237: kind <(null)>
> > (163299788), spec is
> > [    5.693181] prog '(efault)': relo #-2115894246: (null) candidate
> > #-2115185528
> > [    5.693258] prog '(efault)': relo #3: patched insn #208
> > (LDX/ST/STX) off 208 -> 208
> > [    5.696141] prog '(null)': relo #-2115894237: kind <(null)>
> > (163302708), spec is
> > [    5.696143] prog '(null)': relo #-2115894246: (null) candidate #-2115185668
> > [    5.696255] prog '(null)': relo #2: patched insn #104 (LDX/ST/STX)
> > off 104 -> 104
> > [    5.696733] prog '(efault)': relo #-2115894237: kind <(null)>
> > (163299788), spec is
> > [    5.696734] prog '(efault)': relo #-2115894246: (null) candidate
> > #-2115185528
> > [    5.696833] prog '(efault)': relo #3: patched insn #208
> > (LDX/ST/STX) off 208 -> 208
>
> All the logged values are completely wrong, some corruption somewhere.
>
> But I tried to see it for myself and I couldn't figure out how to get
> these logs with lskel. How did you get the above?
>
> Alexei, any guidance on how to get those verifier logs back with
> test_progs? ./test_progs -vvv didn't help, I also checked trace_pipe
> output, it was empty. I thought that maybe verifier truncates logs on
> success and simulated failed prog validation, but still nothing.
>
> >
> > And the syscall returns success:
> >
> > bpf(BPF_PROG_TEST_RUN, {test={prog_fd=4, retval=0, data_size_in=0,
> > data_size_out=0, data_in=NULL, data_out=NULL, repeat=0, duration=0,
> > ctx_size_in=68, ctx_size_out=0, ctx_in=0x5590b97dd2a0, ctx_out=NULL}},
> > 160) = 0
> >
> > Regards,
> > --
> > per aspera ad upstream

Sorry, there was an off-by-one in the macro.
I just aliased all the pr_* to printk, this is the correct output:

# core/core
[    3.686333] prog '': relo #0: kind <byte_off> (0), spec is
[    3.686337] prog '': relo #0: matching candidate #0
[    3.686471] prog '': relo #0: patched insn #2 (LDX/ST/STX) off 208 -> 208
[    3.687209] prog '': relo #1: kind <byte_off> (0), spec is
[    3.687211] prog '': relo #1: matching candidate #0
[    3.687251] prog '': relo #1: patched insn #3 (LDX/ST/STX) off 208 -> 208
[    3.688193] prog '': relo #0: kind <byte_off> (0), spec is
[    3.688196] prog '': relo #0: matching candidate #0
[    3.688238] prog '': relo #0: patched insn #2 (LDX/ST/STX) off 104 -> 104
[    3.688781] prog '': relo #1: kind <byte_off> (0), spec is
[    3.688783] prog '': relo #1: matching candidate #0
[    3.688820] prog '': relo #1: patched insn #3 (LDX/ST/STX) off 208 -> 208
[    3.691529] prog '': relo #0: kind <byte_off> (0), spec is
[    3.691531] prog '': relo #0: matching candidate #0
[    3.691610] prog '': relo #0: patched insn #2 (LDX/ST/STX) off 104 -> 104
[    3.692158] prog '': relo #1: kind <byte_off> (0), spec is
[    3.692160] prog '': relo #1: matching candidate #0
[    3.692256] prog '': relo #1: patched insn #3 (LDX/ST/STX) off 208 -> 208

Regards,
-- 
per aspera ad upstream

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel
  2021-11-17 15:28     ` Matteo Croce
@ 2021-11-17 17:29       ` Alexei Starovoitov
  0 siblings, 0 replies; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-17 17:29 UTC (permalink / raw)
  To: Matteo Croce, Andrii Nakryiko
  Cc: Alexei Starovoitov, David Miller, Daniel Borkmann,
	Andrii Nakryiko, bpf, Kernel Team

On 11/17/21 7:28 AM, Matteo Croce wrote:
> On Wed, Nov 17, 2021 at 5:34 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Tue, Nov 16, 2021 at 4:48 PM Matteo Croce <mcroce@linux.microsoft.com> wrote:
>>>
>>> On Fri, Nov 12, 2021 at 6:02 AM Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>>>>
>>>> From: Alexei Starovoitov <ast@kernel.org>
>>>>
>>>> v1->v2:
>>>> . Refactor uapi to pass 'struct bpf_core_relo' from LLVM into libbpf and further
>>>> into the kernel instead of bpf_core_apply_relo() bpf helper. Because of this
>>>> change the CO-RE algorithm has an ability to log error and debug events through
>>>> the standard bpf verifer log mechanism which was not possible with helper
>>>> approach.
>>>> . #define RELO_CORE macro was removed and replaced with btf_member_bit_offset() patch.
>>>>
>>>> This set introduces CO-RE support in the kernel.
>>>> There are several reasons to add such support:
>>>> 1. It's a step toward signed BPF programs.
>>>> 2. It allows golang like languages that struggle to adopt libbpf
>>>>     to take advantage of CO-RE powers.
>>>> 3. Currently the field accessed by 'ldx [R1 + 10]' insn is recognized
>>>>     by the verifier purely based on +10 offset. If R1 points to a union
>>>>     the verifier picks one of the fields at this offset.
>>>>     With CO-RE the kernel can disambiguate the field access.
>>>>
>>>
>>> Great, I tested the same code which was failing with the RFC series,
>>> now there isn't any error.
>>> This is the output with pr_debug() enabled:
>>>
>>> root@debian64:~/core# ./core
>>> [    5.690268] prog '(null)': relo #-2115894237: kind <(null)>
>>> (163299788), spec is
>>> [    5.690272] prog '(null)': relo #-2115894246: (null) candidate #-2115185528
>>> [    5.690392] prog '(null)': relo #2: patched insn #208 (LDX/ST/STX)
>>> off 208 -> 208
>>> [    5.691045] prog '(efault)': relo #-2115894237: kind <(null)>
>>> (163299788), spec is
>>> [    5.691047] prog '(efault)': relo #-2115894246: (null) candidate
>>> #-2115185528
>>> [    5.691148] prog '(efault)': relo #3: patched insn #208
>>> (LDX/ST/STX) off 208 -> 208
>>> [    5.692456] prog '(null)': relo #-2115894237: kind <(null)>
>>> (163302708), spec is
>>> [    5.692459] prog '(null)': relo #-2115894246: (null) candidate #-2115185668
>>> [    5.692564] prog '(null)': relo #2: patched insn #104 (LDX/ST/STX)
>>> off 104 -> 104
>>> [    5.693179] prog '(efault)': relo #-2115894237: kind <(null)>
>>> (163299788), spec is
>>> [    5.693181] prog '(efault)': relo #-2115894246: (null) candidate
>>> #-2115185528
>>> [    5.693258] prog '(efault)': relo #3: patched insn #208
>>> (LDX/ST/STX) off 208 -> 208
>>> [    5.696141] prog '(null)': relo #-2115894237: kind <(null)>
>>> (163302708), spec is
>>> [    5.696143] prog '(null)': relo #-2115894246: (null) candidate #-2115185668
>>> [    5.696255] prog '(null)': relo #2: patched insn #104 (LDX/ST/STX)
>>> off 104 -> 104
>>> [    5.696733] prog '(efault)': relo #-2115894237: kind <(null)>
>>> (163299788), spec is
>>> [    5.696734] prog '(efault)': relo #-2115894246: (null) candidate
>>> #-2115185528
>>> [    5.696833] prog '(efault)': relo #3: patched insn #208
>>> (LDX/ST/STX) off 208 -> 208
>>
>> All the logged values are completely wrong, some corruption somewhere.
>>
>> But I tried to see it for myself and I couldn't figure out how to get
>> these logs with lskel. How did you get the above?
>>
>> Alexei, any guidance on how to get those verifier logs back with
>> test_progs? ./test_progs -vvv didn't help, I also checked trace_pipe
>> output, it was empty. I thought that maybe verifier truncates logs on
>> success and simulated failed prog validation, but still nothing.
>>
>>>
>>> And the syscall returns success:
>>>
>>> bpf(BPF_PROG_TEST_RUN, {test={prog_fd=4, retval=0, data_size_in=0,
>>> data_size_out=0, data_in=NULL, data_out=NULL, repeat=0, duration=0,
>>> ctx_size_in=68, ctx_size_out=0, ctx_in=0x5590b97dd2a0, ctx_out=NULL}},
>>> 160) = 0
>>>
>>> Regards,
>>> --
>>> per aspera ad upstream
> 
> Sorry, there was an off-by-one in the macro.
> I just aliased all the pr_* to printk, this is the correct output:

This hack is not necessary. The debug messages are going to the verifier
log and it's printed when prog fails to load.
To see successful log the hack to lskel's prog_load_and_run is needed.
I'll add -vvv support in the follow up, so it's easier.
Currently neither standard skel nor lskel support -v, unfortunately,
so a bit of plumbing is necessary (which is not related to "CO-RE in the 
kernel" work).

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 02/12] bpf: Rename btf_member accessors.
  2021-11-17  0:38   ` Andrii Nakryiko
@ 2021-11-19  3:17     ` Alexei Starovoitov
  2021-11-19 17:31       ` Andrii Nakryiko
  0 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-19  3:17 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Tue, Nov 16, 2021 at 04:38:18PM -0800, Andrii Nakryiko wrote:
> >
> > -static inline u32 btf_member_bit_offset(const struct btf_type *struct_type,
> > -                                       const struct btf_member *member)
> > +static inline u32 __btf_member_bit_offset(const struct btf_type *struct_type,
> > +                                         const struct btf_member *member)
> 
> a bit surprised you didn't choose to just remove them, given you had
> to touch all 24 places in the kernel that call this helper
> > -                       if (btf_member_bitfield_size(t, member)) {
> > +                       if (__btf_member_bitfield_size(t, member)) {
> 
> like in this case it would be btf_member_bitfield_size(t, j)

In this and few other cases it's indeed possible, but not in net/ipv4/bpf_tcp_ca.c
It has two callbacks:
struct bpf_struct_ops {
        const struct bpf_verifier_ops *verifier_ops;
        int (*init)(struct btf *btf);
        int (*check_member)(const struct btf_type *t,
                            const struct btf_member *member);
        int (*init_member)(const struct btf_type *t,
                           const struct btf_member *member,
so they cannot be changed without massive refactoring.
Also member pointer vs index is arguably a better api.
I'm not sure compiler can optimize index into pointer in case like below
and won't introduce redundant loads.

> >         for_each_member(i, t, member) {
> > -               moff = btf_member_bit_offset(t, member) / 8;
> > +               moff = __btf_member_bit_offset(t, member) / 8;
> 
> same here, seema like in all the cases we already have member_idx (i
> in this case)

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 03/12] bpf: Prepare relo_core.c for kernel duty.
  2021-11-17  0:58   ` Andrii Nakryiko
@ 2021-11-19  3:20     ` Alexei Starovoitov
  0 siblings, 0 replies; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-19  3:20 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Tue, Nov 16, 2021 at 04:58:23PM -0800, Andrii Nakryiko wrote:
> >
> > -#define BPF_CORE_SPEC_MAX_LEN 64
> > +#define BPF_CORE_SPEC_MAX_LEN 32
> 
> This is worth calling out in the commit description, should have
> practical implications, but good to mention.

good point.

> 
> >
> >  /* represents BPF CO-RE field or array element accessor */
> >  struct bpf_core_accessor {
> > @@ -272,8 +325,8 @@ static int bpf_core_parse_spec(const struct btf *btf,
> >                                 return sz;
> >                         spec->bit_offset += access_idx * sz * 8;
> >                 } else {
> > -                       pr_warn("relo for [%u] %s (at idx %d) captures type [%d] of unexpected kind %s\n",
> > -                               type_id, spec_str, i, id, btf_kind_str(t));
> > +/*                     pr_warn("relo for [%u] %s (at idx %d) captures type [%d] of unexpected kind %s\n",
> > +                               type_id, spec_str, i, id, btf_kind_str(t));*/
> 
> we can totally pass prog_name and add "prog '%s': " to uncomment this.
> bpf_core_parse_spec() is called in the "context" of program, so it's
> known

Sure.

> >                         return -EINVAL;
> >                 }
> >         }
> > @@ -346,8 +399,8 @@ static int bpf_core_fields_are_compat(const struct btf *local_btf,
> >                 targ_id = btf_array(targ_type)->type;
> >                 goto recur;
> >         default:
> > -               pr_warn("unexpected kind %d relocated, local [%d], target [%d]\n",
> > -                       btf_kind(local_type), local_id, targ_id);
> > +/*             pr_warn("unexpected kind %d relocated, local [%d], target [%d]\n",
> > +                       btf_kind(local_type), local_id, targ_id);*/
> 
> sigh... it's a bit too intrusive to pass prog_name here but it's also
> highly unlikely that this happens (unless some compiler bug or
> corruption) and even in that case it's semantically correct that
> fields just don't match. So I'd just drop this pr_warn() instead of
> commenting it out

makes sense to me. will do.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 05/12] bpf: Pass a set of bpf_core_relo-s to prog_load command.
  2021-11-17  1:17   ` Andrii Nakryiko
@ 2021-11-19  3:32     ` Alexei Starovoitov
  2021-11-19 17:35       ` Andrii Nakryiko
  0 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-19  3:32 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Tue, Nov 16, 2021 at 05:17:27PM -0800, Andrii Nakryiko wrote:
> > +
> > +       rec_size = attr->core_relo_rec_size;
> > +       if (rec_size != sizeof(struct bpf_core_relo))
> > +               return -EINVAL;
> 
> For func_info we allow trailing zeroes (in check_btf_func, we check
> MIN_BPF_FUNCINFO_SIZE and MAX_FUNCINFO_REC_SIZE). Shouldn't we do
> something like that here?

For both func_info and line_info the verifier indeed uses 252 as max rec_size.
I believe the logic is trying to help in the following case:
If llvm starts to produce a larger records the libbpf will test for the
new kernel and will sanize the newer records with zeros _in place_.

In case of bpf_core_relo the implementation of this patch doesn't
take llvm bytes as-is. The relo records saved and copied potentially
many times with every libbpf call relocations.
Then later they are prepared in the final form.

I don't mind doing the same 252 logic here, but when I wrote above
check it felt more natural to be strict and simple, but...

> > +
> > +       u_core_relo = make_bpfptr(attr->core_relo, uattr.is_kernel);
> > +       expected_size = sizeof(struct bpf_core_relo);
> > +       ncopy = min_t(u32, expected_size, rec_size);
> 
> I'm confused, a few lines above you errored out if expected_size != rec_size...

but then I kept this part and below bpf_check_uarg_tail_zero().
Clearly I couldn't make up my mind :)

So drop all future proofing ? or do 252 ?

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 06/12] bpf: Add bpf_core_add_cands() and wire it into bpf_core_apply_relo_insn().
  2021-11-17  1:31   ` Andrii Nakryiko
@ 2021-11-19  3:34     ` Alexei Starovoitov
  0 siblings, 0 replies; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-19  3:34 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Tue, Nov 16, 2021 at 05:31:14PM -0800, Andrii Nakryiko wrote:
> > +                }
> > +       }
> > +       err = bpf_core_apply_relo_insn((void *)log, insn, relo->insn_off / 8,
> > +                                      relo, relo_idx, btf, cands);
> > +       bpf_core_free_cands(cands);
> 
> Why did you decide to not persist the candidate list? It is a
> significant slowdown even on moderately large BPF programs, as you are
> linearly re-searching vmlinux BTF multiple times for the same root
> type.

Somehow I convinced myself that it's not a correct thing to do across
multiple relocations. Yeah. Will move the free to be after the loop.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 07/12] libbpf: Use CO-RE in the kernel in light skeleton.
  2021-11-17  3:45   ` Andrii Nakryiko
@ 2021-11-19  3:57     ` Alexei Starovoitov
  2021-11-19 17:59       ` Andrii Nakryiko
  0 siblings, 1 reply; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-19  3:57 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Tue, Nov 16, 2021 at 07:45:47PM -0800, Andrii Nakryiko wrote:
> On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > Without lskel the CO-RE relocations are processed by libbpf before any other
> > work is done. Instead, when lksel is needed, remember relocation as RELO_CORE
> 
> typo: lskel
> 
> > kind. Then when loader prog is generated for a given bpf program pass CO-RE
> > relos of that program to gen loader via bpf_gen__record_relo_core(). The gen
> > loader will remember them as-is and pass it later as-is into the kernel.
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ---
> >  tools/lib/bpf/bpf_gen_internal.h |   3 +
> >  tools/lib/bpf/gen_loader.c       |  41 +++++++++++-
> >  tools/lib/bpf/libbpf.c           | 104 +++++++++++++++++++++++--------
> >  3 files changed, 119 insertions(+), 29 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf_gen_internal.h b/tools/lib/bpf/bpf_gen_internal.h
> > index 75ca9fb857b2..ed162fdeecf6 100644
> > --- a/tools/lib/bpf/bpf_gen_internal.h
> > +++ b/tools/lib/bpf/bpf_gen_internal.h
> > @@ -39,6 +39,8 @@ struct bpf_gen {
> >         int error;
> >         struct ksym_relo_desc *relos;
> >         int relo_cnt;
> > +       struct bpf_core_relo *core_relo;
> 
> this is named as a singular pointer to one relocation, core_relos
> would be a more natural name for an array?

I had it with "s" at the beginning, but it was forcing core_relo_cnt variable
to be called core_relos_cnt to be consistent.
And later it spills this "consistency" into uapi core_relos_cnt in bpf_attr.
But here it conflicts with line_info_cnt and func_info_cnt.
Once I realized that I went back and got rid of this "s".

> 
> > +       int core_relo_cnt;
> >         char attach_target[128];
> >         int attach_kind;
> >         struct ksym_desc *ksyms;
> > @@ -61,5 +63,6 @@ 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,
> >                             bool is_typeless, int kind, int insn_idx);
> 
> [...]
> 
> > @@ -6581,6 +6623,16 @@ static int bpf_program__record_externs(struct bpf_program *prog)
> >                                                ext->is_weak, false, BTF_KIND_FUNC,
> >                                                relo->insn_idx);
> >                         break;
> > +               case RELO_CORE: {
> 
> This is not an extern, it doesn't make sense to have it here. But I
> also don't understand why we need to add RELO_CORE and extend struct
> relo_desc in the first place, just to pass it as bpf_core_relo into
> gen_loader. Why can't gen_loader just record this directly at the
> place of record_relo_core() call?

Sorry. I should have explained it in commit log.
The normal libbpf flow is to process CO-RE early before call relos happen.
In case of gen_loader the core relos have to be added to other relos to be
copied together when bpf static function is appended in different places to
other main bpf progs.
During the copy the append_subprog_relos() will adjust insn_idx for
normal relos and for RELO_CORE kind too.
When that is done each struct reloc_desc has good relos for specific main prog.

Just noticed that 'insn_idx += prog->sub_insn_off;' in this patch is redundant.
That was a left over of earlier debugging.

Also in bpf_object__relocate_data() the comment:
                case RELO_CORE:
                        /* handled already */
                        break;
is not correct either.
It should read "will be handled by bpf_program__record_externs() later".
Just before bpf_object_load_prog_instance().

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 12/12] selftests/bpf: Additional test for CO-RE in the kernel.
  2021-11-17  4:18   ` Andrii Nakryiko
@ 2021-11-19  4:00     ` Alexei Starovoitov
  0 siblings, 0 replies; 38+ messages in thread
From: Alexei Starovoitov @ 2021-11-19  4:00 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Tue, Nov 16, 2021 at 08:18:44PM -0800, Andrii Nakryiko wrote:
> > +
> > +       skel = core_kern_lskel__open_and_load();
> > +       if (!ASSERT_OK_PTR(skel, "open_and_load"))
> > +               goto cleanup;
> > +
> > +       err = core_kern_lskel__attach(skel);
> 
> Why attaching if you are never triggering it? This test is about
> testing load, right? Let's drop the attach then.

Good point. Indeed!

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 02/12] bpf: Rename btf_member accessors.
  2021-11-19  3:17     ` Alexei Starovoitov
@ 2021-11-19 17:31       ` Andrii Nakryiko
  0 siblings, 0 replies; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-19 17:31 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 18, 2021 at 7:17 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 16, 2021 at 04:38:18PM -0800, Andrii Nakryiko wrote:
> > >
> > > -static inline u32 btf_member_bit_offset(const struct btf_type *struct_type,
> > > -                                       const struct btf_member *member)
> > > +static inline u32 __btf_member_bit_offset(const struct btf_type *struct_type,
> > > +                                         const struct btf_member *member)
> >
> > a bit surprised you didn't choose to just remove them, given you had
> > to touch all 24 places in the kernel that call this helper
> > > -                       if (btf_member_bitfield_size(t, member)) {
> > > +                       if (__btf_member_bitfield_size(t, member)) {
> >
> > like in this case it would be btf_member_bitfield_size(t, j)
>
> In this and few other cases it's indeed possible, but not in net/ipv4/bpf_tcp_ca.c
> It has two callbacks:
> struct bpf_struct_ops {
>         const struct bpf_verifier_ops *verifier_ops;
>         int (*init)(struct btf *btf);
>         int (*check_member)(const struct btf_type *t,
>                             const struct btf_member *member);
>         int (*init_member)(const struct btf_type *t,
>                            const struct btf_member *member,
> so they cannot be changed without massive refactoring.

member_idx = member - btf_type_member(t);

But as I said, not a big deal.

> Also member pointer vs index is arguably a better api.

Not so sure about that, as it allows accidentally passing a completely
irrelevant member pointer from a different type. With member_idx you
can do some sanity checking and make sure you are not accessing
members out of bounds. But I don't really care, just wanted to point
out the possibility of eliminating a somewhat redundant helper.

> I'm not sure compiler can optimize index into pointer in case like below
> and won't introduce redundant loads.
>
> > >         for_each_member(i, t, member) {
> > > -               moff = btf_member_bit_offset(t, member) / 8;
> > > +               moff = __btf_member_bit_offset(t, member) / 8;
> >
> > same here, seema like in all the cases we already have member_idx (i
> > in this case)

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 05/12] bpf: Pass a set of bpf_core_relo-s to prog_load command.
  2021-11-19  3:32     ` Alexei Starovoitov
@ 2021-11-19 17:35       ` Andrii Nakryiko
  0 siblings, 0 replies; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-19 17:35 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 18, 2021 at 7:32 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 16, 2021 at 05:17:27PM -0800, Andrii Nakryiko wrote:
> > > +
> > > +       rec_size = attr->core_relo_rec_size;
> > > +       if (rec_size != sizeof(struct bpf_core_relo))
> > > +               return -EINVAL;
> >
> > For func_info we allow trailing zeroes (in check_btf_func, we check
> > MIN_BPF_FUNCINFO_SIZE and MAX_FUNCINFO_REC_SIZE). Shouldn't we do
> > something like that here?
>
> For both func_info and line_info the verifier indeed uses 252 as max rec_size.
> I believe the logic is trying to help in the following case:
> If llvm starts to produce a larger records the libbpf will test for the
> new kernel and will sanize the newer records with zeros _in place_.
>
> In case of bpf_core_relo the implementation of this patch doesn't
> take llvm bytes as-is. The relo records saved and copied potentially
> many times with every libbpf call relocations.
> Then later they are prepared in the final form.
>
> I don't mind doing the same 252 logic here, but when I wrote above
> check it felt more natural to be strict and simple, but...
>
> > > +
> > > +       u_core_relo = make_bpfptr(attr->core_relo, uattr.is_kernel);
> > > +       expected_size = sizeof(struct bpf_core_relo);
> > > +       ncopy = min_t(u32, expected_size, rec_size);
> >
> > I'm confused, a few lines above you errored out if expected_size != rec_size...
>
> but then I kept this part and below bpf_check_uarg_tail_zero().
> Clearly I couldn't make up my mind :)
>

Heh :)

> So drop all future proofing ? or do 252 ?

I think allowing bigger record sizes than the kernel knows about
(assuming zeroes in the tail) is a bit better, because then
sanitization won't require unnecessary memory copying. I'm actually
not sure why we restrict record size, if kernel can just copy just the
right amount of first X bytes of each record (we do the copy anyways),
so it feels like kernel should just make sure that the tail is zeroed,
like we do for bpf_attr, for example.

So I guess I vote for future proofing.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 bpf-next 07/12] libbpf: Use CO-RE in the kernel in light skeleton.
  2021-11-19  3:57     ` Alexei Starovoitov
@ 2021-11-19 17:59       ` Andrii Nakryiko
  0 siblings, 0 replies; 38+ messages in thread
From: Andrii Nakryiko @ 2021-11-19 17:59 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: David S. Miller, Daniel Borkmann, Andrii Nakryiko, bpf, Kernel Team

On Thu, Nov 18, 2021 at 7:57 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Nov 16, 2021 at 07:45:47PM -0800, Andrii Nakryiko wrote:
> > On Thu, Nov 11, 2021 at 9:02 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > From: Alexei Starovoitov <ast@kernel.org>
> > >
> > > Without lskel the CO-RE relocations are processed by libbpf before any other
> > > work is done. Instead, when lksel is needed, remember relocation as RELO_CORE
> >
> > typo: lskel
> >
> > > kind. Then when loader prog is generated for a given bpf program pass CO-RE
> > > relos of that program to gen loader via bpf_gen__record_relo_core(). The gen
> > > loader will remember them as-is and pass it later as-is into the kernel.
> > >
> > > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > > ---
> > >  tools/lib/bpf/bpf_gen_internal.h |   3 +
> > >  tools/lib/bpf/gen_loader.c       |  41 +++++++++++-
> > >  tools/lib/bpf/libbpf.c           | 104 +++++++++++++++++++++++--------
> > >  3 files changed, 119 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/bpf_gen_internal.h b/tools/lib/bpf/bpf_gen_internal.h
> > > index 75ca9fb857b2..ed162fdeecf6 100644
> > > --- a/tools/lib/bpf/bpf_gen_internal.h
> > > +++ b/tools/lib/bpf/bpf_gen_internal.h
> > > @@ -39,6 +39,8 @@ struct bpf_gen {
> > >         int error;
> > >         struct ksym_relo_desc *relos;
> > >         int relo_cnt;
> > > +       struct bpf_core_relo *core_relo;
> >
> > this is named as a singular pointer to one relocation, core_relos
> > would be a more natural name for an array?
>
> I had it with "s" at the beginning, but it was forcing core_relo_cnt variable
> to be called core_relos_cnt to be consistent.

Not really. In English it's "number of arguments" (plural), but
"argument count" (singular). There is some English term for these
"compound nouns". We use this naming quite consistently in libbpf code
(with some very early exceptions of sometimes calling arguments
"insns_cnt", which is actually wrong). You can even see this
consistency with relos and relo_cnt. So in this case it would be
core_relos + core_relo_cnt and that would be proper from English POV.

> And later it spills this "consistency" into uapi core_relos_cnt in bpf_attr.
> But here it conflicts with line_info_cnt and func_info_cnt.
> Once I realized that I went back and got rid of this "s".
>
> >
> > > +       int core_relo_cnt;
> > >         char attach_target[128];
> > >         int attach_kind;
> > >         struct ksym_desc *ksyms;
> > > @@ -61,5 +63,6 @@ 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,
> > >                             bool is_typeless, int kind, int insn_idx);
> >
> > [...]
> >
> > > @@ -6581,6 +6623,16 @@ static int bpf_program__record_externs(struct bpf_program *prog)
> > >                                                ext->is_weak, false, BTF_KIND_FUNC,
> > >                                                relo->insn_idx);
> > >                         break;
> > > +               case RELO_CORE: {
> >
> > This is not an extern, it doesn't make sense to have it here. But I
> > also don't understand why we need to add RELO_CORE and extend struct
> > relo_desc in the first place, just to pass it as bpf_core_relo into
> > gen_loader. Why can't gen_loader just record this directly at the
> > place of record_relo_core() call?
>
> Sorry. I should have explained it in commit log.
> The normal libbpf flow is to process CO-RE early before call relos happen.
> In case of gen_loader the core relos have to be added to other relos to be
> copied together when bpf static function is appended in different places to
> other main bpf progs.
> During the copy the append_subprog_relos() will adjust insn_idx for
> normal relos and for RELO_CORE kind too.
> When that is done each struct reloc_desc has good relos for specific main prog.
>

Yeah, I recalled that when thinking about BTFGen. Ok, this makes
gen_loader's life easier. But I'm uncomfortable with two things, which
should be easy to fix.

1) all these added fields to the relo_desc to record bpf_core_relo
struct fields. It's both unnecessarily open-coding bpf_core_relo and
increasing the memory usage. There could be tons of relocations for
big apps, so I'd like to not bloat memory unnecessarily. Good thing is
that we don't need to copy anything, we can just store the pointer to
bpf_core_relo (which is pointing to internals of struct btf_ext, so
will survive till bpf_object__close()). So I have something like this
in mind:

struct reloc_desc {
    enum reloc_type type;
    int insn_idx;
    union {
        struct {
            int map_idx;
            int sym_off;
        }
        const struct bpf_core_relo *core_relo;
    };
};

So reloc_desc contains type, instruction index, and additional
relo_type-specific info. We can later improve organization and naming,
if necessary, but for now it should be ok with anonymous union and
struct.

2) Given we need RELO_CORE only for gen_loader, we probably should
append those only if (obj->gen_loader). No need to do extra
allocations unnecessarily. Plus we can always change that in the
future. BTW, I was thinking over this week whether there is some reuse
and unification between your changes and BTFGen use case, but I
concluded that they are substantially different to not use exactly the
same mechanism. But see the other thread for details, I proposed some
breakdown of CO-RE relo logic which might affect what you are doing
(in a good way, I think).

WDYT?


> Just noticed that 'insn_idx += prog->sub_insn_off;' in this patch is redundant.
> That was a left over of earlier debugging.
>
> Also in bpf_object__relocate_data() the comment:
>                 case RELO_CORE:
>                         /* handled already */
>                         break;
> is not correct either.
> It should read "will be handled by bpf_program__record_externs() later".
> Just before bpf_object_load_prog_instance().

Ok, but then let's rename it to more generic
bpf_program_record_relos()? Not just externs anymore.

BTW, not a strict rule, but I'm trying to not use obj__method() naming
for non-public API nowadays, I'm finding it quite confusing in
practice (always worry when reading code that it's exposed in public
API). Not enough to go rename everything, but for new stuff I'd stick
to bpf_program_something_something().

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2021-11-19 17:59 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-12  5:02 [PATCH v2 bpf-next 00/12] bpf: CO-RE support in the kernel Alexei Starovoitov
2021-11-12  5:02 ` [PATCH v2 bpf-next 01/12] libbpf: s/btf__type_by_id/btf_type_by_id/ Alexei Starovoitov
2021-11-17  0:32   ` Andrii Nakryiko
2021-11-12  5:02 ` [PATCH v2 bpf-next 02/12] bpf: Rename btf_member accessors Alexei Starovoitov
2021-11-17  0:38   ` Andrii Nakryiko
2021-11-19  3:17     ` Alexei Starovoitov
2021-11-19 17:31       ` Andrii Nakryiko
2021-11-12  5:02 ` [PATCH v2 bpf-next 03/12] bpf: Prepare relo_core.c for kernel duty Alexei Starovoitov
2021-11-17  0:58   ` Andrii Nakryiko
2021-11-19  3:20     ` Alexei Starovoitov
2021-11-12  5:02 ` [PATCH v2 bpf-next 04/12] bpf: Define enum bpf_core_relo_kind as uapi Alexei Starovoitov
2021-11-17  1:04   ` Andrii Nakryiko
2021-11-12  5:02 ` [PATCH v2 bpf-next 05/12] bpf: Pass a set of bpf_core_relo-s to prog_load command Alexei Starovoitov
2021-11-17  1:17   ` Andrii Nakryiko
2021-11-19  3:32     ` Alexei Starovoitov
2021-11-19 17:35       ` Andrii Nakryiko
2021-11-12  5:02 ` [PATCH v2 bpf-next 06/12] bpf: Add bpf_core_add_cands() and wire it into bpf_core_apply_relo_insn() Alexei Starovoitov
2021-11-17  1:31   ` Andrii Nakryiko
2021-11-19  3:34     ` Alexei Starovoitov
2021-11-12  5:02 ` [PATCH v2 bpf-next 07/12] libbpf: Use CO-RE in the kernel in light skeleton Alexei Starovoitov
2021-11-17  3:45   ` Andrii Nakryiko
2021-11-19  3:57     ` Alexei Starovoitov
2021-11-19 17:59       ` Andrii Nakryiko
2021-11-12  5:02 ` [PATCH v2 bpf-next 08/12] libbpf: Support init of inner maps " Alexei Starovoitov
2021-11-17  4:10   ` Andrii Nakryiko
2021-11-12  5:02 ` [PATCH v2 bpf-next 09/12] selftests/bpf: Convert kfunc test with CO-RE to lskel Alexei Starovoitov
2021-11-17  4:13   ` Andrii Nakryiko
2021-11-12  5:02 ` [PATCH v2 bpf-next 10/12] selftests/bpf: Improve inner_map test coverage Alexei Starovoitov
2021-11-17  4:15   ` Andrii Nakryiko
2021-11-12  5:02 ` [PATCH v2 bpf-next 11/12] selftests/bpf: Convert map_ptr_kern test to use light skeleton Alexei Starovoitov
2021-11-17  4:16   ` Andrii Nakryiko
2021-11-12  5:02 ` [PATCH v2 bpf-next 12/12] selftests/bpf: Additional test for CO-RE in the kernel Alexei Starovoitov
2021-11-17  4:18   ` Andrii Nakryiko
2021-11-19  4:00     ` Alexei Starovoitov
2021-11-17  0:48 ` [PATCH v2 bpf-next 00/12] bpf: CO-RE support " Matteo Croce
2021-11-17  4:34   ` Andrii Nakryiko
2021-11-17 15:28     ` Matteo Croce
2021-11-17 17:29       ` Alexei Starovoitov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).