* [PATCH bpf-next v13 01/14] bpf: refactory struct_ops type initialization to a function.
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
@ 2023-12-09 0:26 ` thinker.li
2023-12-09 0:26 ` [PATCH bpf-next v13 02/14] bpf: get type information with BPF_ID_LIST thinker.li
` (12 subsequent siblings)
13 siblings, 0 replies; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:26 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Move the majority of the code to bpf_struct_ops_init_one(), which can then
be utilized for the initialization of newly registered dynamically
allocated struct_ops types in the following patches.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/btf.h | 1 +
kernel/bpf/bpf_struct_ops.c | 157 +++++++++++++++++++-----------------
kernel/bpf/btf.c | 5 ++
3 files changed, 89 insertions(+), 74 deletions(-)
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 59d404e22814..1d852dad7473 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -137,6 +137,7 @@ struct btf_struct_metas {
extern const struct file_operations btf_fops;
+const char *btf_get_name(const struct btf *btf);
void btf_get(struct btf *btf);
void btf_put(struct btf *btf);
int btf_new_fd(const union bpf_attr *attr, bpfptr_t uattr, u32 uattr_sz);
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 4d53c53fc5aa..5714e7e54f9c 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -110,102 +110,111 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
static const struct btf_type *module_type;
-void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
+static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
+ struct btf *btf,
+ struct bpf_verifier_log *log)
{
- s32 type_id, value_id, module_id;
const struct btf_member *member;
- struct bpf_struct_ops *st_ops;
const struct btf_type *t;
+ s32 type_id, value_id;
char value_name[128];
const char *mname;
- u32 i, j;
+ int i;
- /* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
-#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
+ if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
+ sizeof(value_name)) {
+ pr_warn("struct_ops name %s is too long\n",
+ st_ops->name);
+ return;
+ }
+ sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
- module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
- if (module_id < 0) {
- pr_warn("Cannot find struct module in btf_vmlinux\n");
+ value_id = btf_find_by_name_kind(btf, value_name,
+ BTF_KIND_STRUCT);
+ if (value_id < 0) {
+ pr_warn("Cannot find struct %s in %s\n",
+ value_name, btf_get_name(btf));
return;
}
- module_type = btf_type_by_id(btf, module_id);
- for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
- st_ops = bpf_struct_ops[i];
+ type_id = btf_find_by_name_kind(btf, st_ops->name,
+ BTF_KIND_STRUCT);
+ if (type_id < 0) {
+ pr_warn("Cannot find struct %s in %s\n",
+ st_ops->name, btf_get_name(btf));
+ return;
+ }
+ t = btf_type_by_id(btf, type_id);
+ if (btf_type_vlen(t) > BPF_STRUCT_OPS_MAX_NR_MEMBERS) {
+ pr_warn("Cannot support #%u members in struct %s\n",
+ btf_type_vlen(t), st_ops->name);
+ return;
+ }
- if (strlen(st_ops->name) + VALUE_PREFIX_LEN >=
- sizeof(value_name)) {
- pr_warn("struct_ops name %s is too long\n",
+ for_each_member(i, t, member) {
+ const struct btf_type *func_proto;
+
+ mname = btf_name_by_offset(btf, member->name_off);
+ if (!*mname) {
+ pr_warn("anon member in struct %s is not supported\n",
st_ops->name);
- continue;
+ break;
}
- sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
- value_id = btf_find_by_name_kind(btf, value_name,
- BTF_KIND_STRUCT);
- if (value_id < 0) {
- pr_warn("Cannot find struct %s in btf_vmlinux\n",
- value_name);
- continue;
+ 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;
}
- type_id = btf_find_by_name_kind(btf, st_ops->name,
- BTF_KIND_STRUCT);
- if (type_id < 0) {
- pr_warn("Cannot find struct %s in btf_vmlinux\n",
- st_ops->name);
- continue;
- }
- t = btf_type_by_id(btf, type_id);
- if (btf_type_vlen(t) > BPF_STRUCT_OPS_MAX_NR_MEMBERS) {
- pr_warn("Cannot support #%u members in struct %s\n",
- btf_type_vlen(t), st_ops->name);
- continue;
+ func_proto = btf_type_resolve_func_ptr(btf,
+ member->type,
+ NULL);
+ if (func_proto &&
+ btf_distill_func_proto(log, btf,
+ func_proto, mname,
+ &st_ops->func_models[i])) {
+ pr_warn("Error in parsing func ptr %s in struct %s\n",
+ mname, st_ops->name);
+ break;
}
+ }
- for_each_member(j, t, member) {
- const struct btf_type *func_proto;
+ if (i == btf_type_vlen(t)) {
+ if (st_ops->init(btf)) {
+ pr_warn("Error in init bpf_struct_ops %s\n",
+ st_ops->name);
+ } else {
+ st_ops->type_id = type_id;
+ st_ops->type = t;
+ st_ops->value_id = value_id;
+ st_ops->value_type = btf_type_by_id(btf,
+ value_id);
+ }
+ }
+}
- mname = btf_name_by_offset(btf, member->name_off);
- if (!*mname) {
- pr_warn("anon member in struct %s is not supported\n",
- st_ops->name);
- break;
- }
+void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
+{
+ struct bpf_struct_ops *st_ops;
+ s32 module_id;
+ u32 i;
- 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;
- }
+ /* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
+#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
+#include "bpf_struct_ops_types.h"
+#undef BPF_STRUCT_OPS_TYPE
- func_proto = btf_type_resolve_func_ptr(btf,
- member->type,
- NULL);
- if (func_proto &&
- btf_distill_func_proto(log, btf,
- func_proto, mname,
- &st_ops->func_models[j])) {
- pr_warn("Error in parsing func ptr %s in struct %s\n",
- mname, st_ops->name);
- break;
- }
- }
+ module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
+ if (module_id < 0) {
+ pr_warn("Cannot find struct module in %s\n", btf_get_name(btf));
+ return;
+ }
+ module_type = btf_type_by_id(btf, module_id);
- if (j == btf_type_vlen(t)) {
- if (st_ops->init(btf)) {
- pr_warn("Error in init bpf_struct_ops %s\n",
- st_ops->name);
- } else {
- st_ops->type_id = type_id;
- st_ops->type = t;
- st_ops->value_id = value_id;
- st_ops->value_type = btf_type_by_id(btf,
- value_id);
- }
- }
+ for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
+ st_ops = bpf_struct_ops[i];
+ bpf_struct_ops_init_one(st_ops, btf, log);
}
}
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 63cf4128fc05..6935a88ed190 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1707,6 +1707,11 @@ static void btf_free_rcu(struct rcu_head *rcu)
btf_free(btf);
}
+const char *btf_get_name(const struct btf *btf)
+{
+ return btf->name;
+}
+
void btf_get(struct btf *btf)
{
refcount_inc(&btf->refcnt);
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH bpf-next v13 02/14] bpf: get type information with BPF_ID_LIST
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
2023-12-09 0:26 ` [PATCH bpf-next v13 01/14] bpf: refactory struct_ops type initialization to a function thinker.li
@ 2023-12-09 0:26 ` thinker.li
2023-12-15 1:59 ` Martin KaFai Lau
2023-12-09 0:26 ` [PATCH bpf-next v13 03/14] bpf, net: introduce bpf_struct_ops_desc thinker.li
` (11 subsequent siblings)
13 siblings, 1 reply; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:26 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Get ready to remove bpf_struct_ops_init() in the future. By using
BPF_ID_LIST, it is possible to gather type information while building
instead of runtime.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
kernel/bpf/bpf_struct_ops.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 5714e7e54f9c..9f95e9fc00f3 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -108,7 +108,12 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
#endif
};
-static const struct btf_type *module_type;
+BTF_ID_LIST(st_ops_ids)
+BTF_ID(struct, module)
+
+enum {
+ IDX_MODULE_ID,
+};
static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
struct btf *btf,
@@ -197,7 +202,6 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
{
struct bpf_struct_ops *st_ops;
- s32 module_id;
u32 i;
/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
@@ -205,13 +209,6 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
#include "bpf_struct_ops_types.h"
#undef BPF_STRUCT_OPS_TYPE
- module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
- if (module_id < 0) {
- pr_warn("Cannot find struct module in %s\n", btf_get_name(btf));
- return;
- }
- module_type = btf_type_by_id(btf, module_id);
-
for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
st_ops = bpf_struct_ops[i];
bpf_struct_ops_init_one(st_ops, btf, log);
@@ -388,6 +385,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
const struct bpf_struct_ops *st_ops = st_map->st_ops;
struct bpf_struct_ops_value *uvalue, *kvalue;
+ const struct btf_type *module_type;
const struct btf_member *member;
const struct btf_type *t = st_ops->type;
struct bpf_tramp_links *tlinks;
@@ -435,6 +433,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
image = st_map->image;
image_end = st_map->image + PAGE_SIZE;
+ module_type = btf_type_by_id(btf_vmlinux, st_ops_ids[IDX_MODULE_ID]);
for_each_member(i, t, member) {
const struct btf_type *mtype, *ptype;
struct bpf_prog *prog;
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 02/14] bpf: get type information with BPF_ID_LIST
2023-12-09 0:26 ` [PATCH bpf-next v13 02/14] bpf: get type information with BPF_ID_LIST thinker.li
@ 2023-12-15 1:59 ` Martin KaFai Lau
0 siblings, 0 replies; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-15 1:59 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/8/23 4:26 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> Get ready to remove bpf_struct_ops_init() in the future. By using
> BPF_ID_LIST, it is possible to gather type information while building
s/BPF_ID_LIST/BTF_ID_LIST.
The same for the patch subject.
> +BTF_ID_LIST(st_ops_ids)
> +BTF_ID(struct, module)
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH bpf-next v13 03/14] bpf, net: introduce bpf_struct_ops_desc.
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
2023-12-09 0:26 ` [PATCH bpf-next v13 01/14] bpf: refactory struct_ops type initialization to a function thinker.li
2023-12-09 0:26 ` [PATCH bpf-next v13 02/14] bpf: get type information with BPF_ID_LIST thinker.li
@ 2023-12-09 0:26 ` thinker.li
2023-12-15 2:05 ` Martin KaFai Lau
2023-12-09 0:26 ` [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf thinker.li
` (10 subsequent siblings)
13 siblings, 1 reply; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:26 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee, netdev
From: Kui-Feng Lee <thinker.li@gmail.com>
Move some of members of bpf_struct_ops to bpf_struct_ops_desc. When we
introduce the new API to register new bpf_struct_ops types from modules,
bpf_struct_ops may destroyed when the module is unloaded. Moving these
members to bpf_struct_ops_desc make these data available even when the
module is unloaded.
type_id is unavailabe in bpf_struct_ops anymore. Modules should get it from
the btf received by kmod's init function.
Cc: netdev@vger.kernel.org
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/bpf.h | 13 ++++--
kernel/bpf/bpf_struct_ops.c | 80 +++++++++++++++++-----------------
kernel/bpf/verifier.c | 8 ++--
net/bpf/bpf_dummy_struct_ops.c | 9 +++-
net/ipv4/bpf_tcp_ca.c | 8 +++-
5 files changed, 70 insertions(+), 48 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c1a06263a4f3..fa43255f59bc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1681,17 +1681,22 @@ struct bpf_struct_ops {
void (*unreg)(void *kdata);
int (*update)(void *kdata, void *old_kdata);
int (*validate)(void *kdata);
- const struct btf_type *type;
- const struct btf_type *value_type;
const char *name;
struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
+};
+
+struct bpf_struct_ops_desc {
+ struct bpf_struct_ops *st_ops;
+
+ const struct btf_type *type;
+ const struct btf_type *value_type;
u32 type_id;
u32 value_id;
};
#if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
#define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
-const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id);
+const struct bpf_struct_ops_desc *bpf_struct_ops_find(u32 type_id);
void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log);
bool bpf_struct_ops_get(const void *kdata);
void bpf_struct_ops_put(const void *kdata);
@@ -1734,7 +1739,7 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
union bpf_attr __user *uattr);
#endif
#else
-static inline const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id)
+static inline const struct bpf_struct_ops_desc *bpf_struct_ops_find(u32 type_id)
{
return NULL;
}
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 9f95e9fc00f3..5fbf88e6f4e5 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -32,7 +32,7 @@ struct bpf_struct_ops_value {
struct bpf_struct_ops_map {
struct bpf_map map;
struct rcu_head rcu;
- const struct bpf_struct_ops *st_ops;
+ const struct bpf_struct_ops_desc *st_ops_desc;
/* protect map_update */
struct mutex lock;
/* link has all the bpf_links that is populated
@@ -92,9 +92,9 @@ enum {
__NR_BPF_STRUCT_OPS_TYPE,
};
-static struct bpf_struct_ops * const bpf_struct_ops[] = {
+static struct bpf_struct_ops_desc bpf_struct_ops[] = {
#define BPF_STRUCT_OPS_TYPE(_name) \
- [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name,
+ [BPF_STRUCT_OPS_TYPE_##_name] = { .st_ops = &bpf_##_name },
#include "bpf_struct_ops_types.h"
#undef BPF_STRUCT_OPS_TYPE
};
@@ -115,10 +115,11 @@ enum {
IDX_MODULE_ID,
};
-static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
- struct btf *btf,
- struct bpf_verifier_log *log)
+static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
+ struct btf *btf,
+ struct bpf_verifier_log *log)
{
+ struct bpf_struct_ops *st_ops = st_ops_desc->st_ops;
const struct btf_member *member;
const struct btf_type *t;
s32 type_id, value_id;
@@ -190,18 +191,18 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
pr_warn("Error in init bpf_struct_ops %s\n",
st_ops->name);
} else {
- st_ops->type_id = type_id;
- st_ops->type = t;
- st_ops->value_id = value_id;
- st_ops->value_type = btf_type_by_id(btf,
- value_id);
+ st_ops_desc->type_id = type_id;
+ st_ops_desc->type = t;
+ st_ops_desc->value_id = value_id;
+ st_ops_desc->value_type = btf_type_by_id(btf,
+ value_id);
}
}
}
void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
{
- struct bpf_struct_ops *st_ops;
+ struct bpf_struct_ops_desc *st_ops_desc;
u32 i;
/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
@@ -210,14 +211,14 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
#undef BPF_STRUCT_OPS_TYPE
for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
- st_ops = bpf_struct_ops[i];
- bpf_struct_ops_init_one(st_ops, btf, log);
+ st_ops_desc = &bpf_struct_ops[i];
+ bpf_struct_ops_desc_init(st_ops_desc, btf, log);
}
}
extern struct btf *btf_vmlinux;
-static const struct bpf_struct_ops *
+static const struct bpf_struct_ops_desc *
bpf_struct_ops_find_value(u32 value_id)
{
unsigned int i;
@@ -226,14 +227,14 @@ bpf_struct_ops_find_value(u32 value_id)
return NULL;
for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
- if (bpf_struct_ops[i]->value_id == value_id)
- return bpf_struct_ops[i];
+ if (bpf_struct_ops[i].value_id == value_id)
+ return &bpf_struct_ops[i];
}
return NULL;
}
-const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id)
+const struct bpf_struct_ops_desc *bpf_struct_ops_find(u32 type_id)
{
unsigned int i;
@@ -241,8 +242,8 @@ const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id)
return NULL;
for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
- if (bpf_struct_ops[i]->type_id == type_id)
- return bpf_struct_ops[i];
+ if (bpf_struct_ops[i].type_id == type_id)
+ return &bpf_struct_ops[i];
}
return NULL;
@@ -302,7 +303,7 @@ static void *bpf_struct_ops_map_lookup_elem(struct bpf_map *map, void *key)
static void bpf_struct_ops_map_put_progs(struct bpf_struct_ops_map *st_map)
{
- const struct btf_type *t = st_map->st_ops->type;
+ const struct btf_type *t = st_map->st_ops_desc->type;
u32 i;
for (i = 0; i < btf_type_vlen(t); i++) {
@@ -383,11 +384,12 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
void *value, u64 flags)
{
struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
- const struct bpf_struct_ops *st_ops = st_map->st_ops;
+ const struct bpf_struct_ops_desc *st_ops_desc = st_map->st_ops_desc;
+ const struct bpf_struct_ops *st_ops = st_ops_desc->st_ops;
struct bpf_struct_ops_value *uvalue, *kvalue;
const struct btf_type *module_type;
const struct btf_member *member;
- const struct btf_type *t = st_ops->type;
+ const struct btf_type *t = st_ops_desc->type;
struct bpf_tramp_links *tlinks;
void *udata, *kdata;
int prog_fd, err;
@@ -400,7 +402,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
if (*(u32 *)key != 0)
return -E2BIG;
- err = check_zero_holes(st_ops->value_type, value);
+ err = check_zero_holes(st_ops_desc->value_type, value);
if (err)
return err;
@@ -493,7 +495,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
}
if (prog->type != BPF_PROG_TYPE_STRUCT_OPS ||
- prog->aux->attach_btf_id != st_ops->type_id ||
+ prog->aux->attach_btf_id != st_ops_desc->type_id ||
prog->expected_attach_type != i) {
bpf_prog_put(prog);
err = -EINVAL;
@@ -588,7 +590,7 @@ static long bpf_struct_ops_map_delete_elem(struct bpf_map *map, void *key)
BPF_STRUCT_OPS_STATE_TOBEFREE);
switch (prev_state) {
case BPF_STRUCT_OPS_STATE_INUSE:
- st_map->st_ops->unreg(&st_map->kvalue.data);
+ st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data);
bpf_map_put(map);
return 0;
case BPF_STRUCT_OPS_STATE_TOBEFREE:
@@ -669,22 +671,22 @@ static int bpf_struct_ops_map_alloc_check(union bpf_attr *attr)
static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
{
- const struct bpf_struct_ops *st_ops;
+ const struct bpf_struct_ops_desc *st_ops_desc;
size_t st_map_size;
struct bpf_struct_ops_map *st_map;
const struct btf_type *t, *vt;
struct bpf_map *map;
int ret;
- st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
- if (!st_ops)
+ st_ops_desc = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
+ if (!st_ops_desc)
return ERR_PTR(-ENOTSUPP);
- vt = st_ops->value_type;
+ vt = st_ops_desc->value_type;
if (attr->value_size != vt->size)
return ERR_PTR(-EINVAL);
- t = st_ops->type;
+ t = st_ops_desc->type;
st_map_size = sizeof(*st_map) +
/* kvalue stores the
@@ -696,7 +698,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
if (!st_map)
return ERR_PTR(-ENOMEM);
- st_map->st_ops = st_ops;
+ st_map->st_ops_desc = st_ops_desc;
map = &st_map->map;
ret = bpf_jit_charge_modmem(PAGE_SIZE);
@@ -733,8 +735,8 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
static u64 bpf_struct_ops_map_mem_usage(const struct bpf_map *map)
{
struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
- const struct bpf_struct_ops *st_ops = st_map->st_ops;
- const struct btf_type *vt = st_ops->value_type;
+ const struct bpf_struct_ops_desc *st_ops_desc = st_map->st_ops_desc;
+ const struct btf_type *vt = st_ops_desc->value_type;
u64 usage;
usage = sizeof(*st_map) +
@@ -808,7 +810,7 @@ static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
/* st_link->map can be NULL if
* bpf_struct_ops_link_create() fails to register.
*/
- st_map->st_ops->unreg(&st_map->kvalue.data);
+ st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data);
bpf_map_put(&st_map->map);
}
kfree(st_link);
@@ -855,7 +857,7 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
if (!bpf_struct_ops_valid_to_reg(new_map))
return -EINVAL;
- if (!st_map->st_ops->update)
+ if (!st_map->st_ops_desc->st_ops->update)
return -EOPNOTSUPP;
mutex_lock(&update_mutex);
@@ -868,12 +870,12 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
old_st_map = container_of(old_map, struct bpf_struct_ops_map, map);
/* The new and old struct_ops must be the same type. */
- if (st_map->st_ops != old_st_map->st_ops) {
+ if (st_map->st_ops_desc != old_st_map->st_ops_desc) {
err = -EINVAL;
goto err_out;
}
- err = st_map->st_ops->update(st_map->kvalue.data, old_st_map->kvalue.data);
+ err = st_map->st_ops_desc->st_ops->update(st_map->kvalue.data, old_st_map->kvalue.data);
if (err)
goto err_out;
@@ -924,7 +926,7 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
if (err)
goto err_out;
- err = st_map->st_ops->reg(st_map->kvalue.data);
+ err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data);
if (err) {
bpf_link_cleanup(&link_primer);
link = NULL;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index fb690539d5f6..ce41bc17ac10 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20065,6 +20065,7 @@ static void print_verification_stats(struct bpf_verifier_env *env)
static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
{
const struct btf_type *t, *func_proto;
+ const struct bpf_struct_ops_desc *st_ops_desc;
const struct bpf_struct_ops *st_ops;
const struct btf_member *member;
struct bpf_prog *prog = env->prog;
@@ -20077,14 +20078,15 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
}
btf_id = prog->aux->attach_btf_id;
- st_ops = bpf_struct_ops_find(btf_id);
- if (!st_ops) {
+ st_ops_desc = bpf_struct_ops_find(btf_id);
+ if (!st_ops_desc) {
verbose(env, "attach_btf_id %u is not a supported struct\n",
btf_id);
return -ENOTSUPP;
}
+ st_ops = st_ops_desc->st_ops;
- t = st_ops->type;
+ t = st_ops_desc->type;
member_idx = prog->expected_attach_type;
if (member_idx >= btf_type_vlen(t)) {
verbose(env, "attach to invalid member idx %u of struct %s\n",
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index 2748f9d77b18..bd753dbccaf6 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -17,6 +17,8 @@ struct bpf_dummy_ops_test_args {
struct bpf_dummy_ops_state state;
};
+static struct btf *bpf_dummy_ops_btf;
+
static struct bpf_dummy_ops_test_args *
dummy_ops_init_args(const union bpf_attr *kattr, unsigned int nr)
{
@@ -85,9 +87,13 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
void *image = NULL;
unsigned int op_idx;
int prog_ret;
+ u32 type_id;
int err;
- if (prog->aux->attach_btf_id != st_ops->type_id)
+ type_id = btf_find_by_name_kind(bpf_dummy_ops_btf,
+ bpf_bpf_dummy_ops.name,
+ BTF_KIND_STRUCT);
+ if (prog->aux->attach_btf_id != type_id)
return -EOPNOTSUPP;
func_proto = prog->aux->attach_func_proto;
@@ -142,6 +148,7 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
static int bpf_dummy_init(struct btf *btf)
{
+ bpf_dummy_ops_btf = btf;
return 0;
}
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index c7bbd8f3c708..5bb56c9ad4e5 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -20,6 +20,7 @@ static u32 unsupported_ops[] = {
static const struct btf_type *tcp_sock_type;
static u32 tcp_sock_id, sock_id;
+static const struct btf_type *tcp_congestion_ops_type;
static int bpf_tcp_ca_init(struct btf *btf)
{
@@ -36,6 +37,11 @@ static int bpf_tcp_ca_init(struct btf *btf)
tcp_sock_id = type_id;
tcp_sock_type = btf_type_by_id(btf, tcp_sock_id);
+ type_id = btf_find_by_name_kind(btf, "tcp_congestion_ops", BTF_KIND_STRUCT);
+ if (type_id < 0)
+ return -EINVAL;
+ tcp_congestion_ops_type = btf_type_by_id(btf, type_id);
+
return 0;
}
@@ -149,7 +155,7 @@ static u32 prog_ops_moff(const struct bpf_prog *prog)
u32 midx;
midx = prog->expected_attach_type;
- t = bpf_tcp_congestion_ops.type;
+ t = tcp_congestion_ops_type;
m = &btf_type_member(t)[midx];
return __btf_member_bit_offset(t, m) / 8;
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 03/14] bpf, net: introduce bpf_struct_ops_desc.
2023-12-09 0:26 ` [PATCH bpf-next v13 03/14] bpf, net: introduce bpf_struct_ops_desc thinker.li
@ 2023-12-15 2:05 ` Martin KaFai Lau
0 siblings, 0 replies; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-15 2:05 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, netdev, bpf, ast, song, kernel-team, andrii,
drosen
On 12/8/23 4:26 PM, thinker.li@gmail.com wrote:
> diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
> index 2748f9d77b18..bd753dbccaf6 100644
> --- a/net/bpf/bpf_dummy_struct_ops.c
> +++ b/net/bpf/bpf_dummy_struct_ops.c
> @@ -17,6 +17,8 @@ struct bpf_dummy_ops_test_args {
> struct bpf_dummy_ops_state state;
> };
>
> +static struct btf *bpf_dummy_ops_btf;
> +
> static struct bpf_dummy_ops_test_args *
> dummy_ops_init_args(const union bpf_attr *kattr, unsigned int nr)
> {
> @@ -85,9 +87,13 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
> void *image = NULL;
> unsigned int op_idx;
> int prog_ret;
> + u32 type_id;
s32 type_id;
> int err;
>
> - if (prog->aux->attach_btf_id != st_ops->type_id)
> + type_id = btf_find_by_name_kind(bpf_dummy_ops_btf,
> + bpf_bpf_dummy_ops.name,
> + BTF_KIND_STRUCT);
if (type_id < 0)
return -EINVAL;
> + if (prog->aux->attach_btf_id != type_id)
> return -EOPNOTSUPP;
>
> func_proto = prog->aux->attach_func_proto;
> @@ -142,6 +148,7 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
>
> static int bpf_dummy_init(struct btf *btf)
> {
> + bpf_dummy_ops_btf = btf;
> return 0;
> }
>
> diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> index c7bbd8f3c708..5bb56c9ad4e5 100644
> --- a/net/ipv4/bpf_tcp_ca.c
> +++ b/net/ipv4/bpf_tcp_ca.c
> @@ -20,6 +20,7 @@ static u32 unsupported_ops[] = {
>
> static const struct btf_type *tcp_sock_type;
> static u32 tcp_sock_id, sock_id;
> +static const struct btf_type *tcp_congestion_ops_type;
>
> static int bpf_tcp_ca_init(struct btf *btf)
> {
> @@ -36,6 +37,11 @@ static int bpf_tcp_ca_init(struct btf *btf)
> tcp_sock_id = type_id;
> tcp_sock_type = btf_type_by_id(btf, tcp_sock_id);
>
> + type_id = btf_find_by_name_kind(btf, "tcp_congestion_ops", BTF_KIND_STRUCT);
> + if (type_id < 0)
> + return -EINVAL;
> + tcp_congestion_ops_type = btf_type_by_id(btf, type_id);
> +
> return 0;
> }
>
> @@ -149,7 +155,7 @@ static u32 prog_ops_moff(const struct bpf_prog *prog)
> u32 midx;
>
> midx = prog->expected_attach_type;
> - t = bpf_tcp_congestion_ops.type;
> + t = tcp_congestion_ops_type;
> m = &btf_type_member(t)[midx];
>
> return __btf_member_bit_offset(t, m) / 8;
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf.
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
` (2 preceding siblings ...)
2023-12-09 0:26 ` [PATCH bpf-next v13 03/14] bpf, net: introduce bpf_struct_ops_desc thinker.li
@ 2023-12-09 0:26 ` thinker.li
2023-12-15 2:22 ` Martin KaFai Lau
2023-12-09 0:27 ` [PATCH bpf-next v13 05/14] bpf: make struct_ops_map support btfs other than btf_vmlinux thinker.li
` (9 subsequent siblings)
13 siblings, 1 reply; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:26 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Maintain a registry of registered struct_ops types in the per-btf (module)
struct_ops_tab. This registry allows for easy lookup of struct_ops types
that are registered by a specific module.
It is a preparation work for supporting kernel module struct_ops in a
latter patch. Each struct_ops will be registered under its own kernel
module btf and will be stored in the newly added btf->struct_ops_tab. The
bpf verifier and bpf syscall (e.g. prog and map cmd) can find the
struct_ops and its btf type/size/id... information from
btf->struct_ops_tab.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/btf.h | 5 ++++
kernel/bpf/btf.c | 73 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+)
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 1d852dad7473..e2f4b85cf82a 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -584,4 +584,9 @@ static inline bool btf_type_is_struct_ptr(struct btf *btf, const struct btf_type
return btf_type_is_struct(t);
}
+struct bpf_struct_ops_desc;
+
+const struct bpf_struct_ops_desc *
+btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
+
#endif
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 6935a88ed190..edbe3cbf2dcc 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -241,6 +241,12 @@ struct btf_id_dtor_kfunc_tab {
struct btf_id_dtor_kfunc dtors[];
};
+struct btf_struct_ops_tab {
+ u32 cnt;
+ u32 capacity;
+ struct bpf_struct_ops_desc ops[];
+};
+
struct btf {
void *data;
struct btf_type **types;
@@ -258,6 +264,7 @@ struct btf {
struct btf_kfunc_set_tab *kfunc_set_tab;
struct btf_id_dtor_kfunc_tab *dtor_kfunc_tab;
struct btf_struct_metas *struct_meta_tab;
+ struct btf_struct_ops_tab *struct_ops_tab;
/* split BTF support */
struct btf *base_btf;
@@ -1688,11 +1695,20 @@ static void btf_free_struct_meta_tab(struct btf *btf)
btf->struct_meta_tab = NULL;
}
+static void btf_free_struct_ops_tab(struct btf *btf)
+{
+ struct btf_struct_ops_tab *tab = btf->struct_ops_tab;
+
+ kfree(tab);
+ btf->struct_ops_tab = NULL;
+}
+
static void btf_free(struct btf *btf)
{
btf_free_struct_meta_tab(btf);
btf_free_dtor_kfunc_tab(btf);
btf_free_kfunc_set_tab(btf);
+ btf_free_struct_ops_tab(btf);
kvfree(btf->types);
kvfree(btf->resolved_sizes);
kvfree(btf->resolved_ids);
@@ -8604,3 +8620,60 @@ bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log,
return !strncmp(reg_name, arg_name, cmp_len);
}
+
+static int
+btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops)
+{
+ struct btf_struct_ops_tab *tab, *new_tab;
+ int i;
+
+ if (!btf)
+ return -ENOENT;
+
+ /* Assume this function is called for a module when the module is
+ * loading.
+ */
+
+ tab = btf->struct_ops_tab;
+ if (!tab) {
+ tab = kzalloc(offsetof(struct btf_struct_ops_tab, ops[4]),
+ GFP_KERNEL);
+ if (!tab)
+ return -ENOMEM;
+ tab->capacity = 4;
+ btf->struct_ops_tab = tab;
+ }
+
+ for (i = 0; i < tab->cnt; i++)
+ if (tab->ops[i].st_ops == st_ops)
+ return -EEXIST;
+
+ if (tab->cnt == tab->capacity) {
+ new_tab = krealloc(tab,
+ offsetof(struct btf_struct_ops_tab,
+ ops[tab->capacity * 2]),
+ GFP_KERNEL);
+ if (!new_tab)
+ return -ENOMEM;
+ tab = new_tab;
+ tab->capacity *= 2;
+ btf->struct_ops_tab = tab;
+ }
+
+ tab->ops[btf->struct_ops_tab->cnt].st_ops = st_ops;
+
+ btf->struct_ops_tab->cnt++;
+
+ return 0;
+}
+
+const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_cnt)
+{
+ if (!btf)
+ return NULL;
+ if (!btf->struct_ops_tab)
+ return NULL;
+
+ *ret_cnt = btf->struct_ops_tab->cnt;
+ return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops;
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf.
2023-12-09 0:26 ` [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf thinker.li
@ 2023-12-15 2:22 ` Martin KaFai Lau
2023-12-15 21:42 ` Kui-Feng Lee
0 siblings, 1 reply; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-15 2:22 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/8/23 4:26 PM, thinker.li@gmail.com wrote:
> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_cnt)
> +{
> + if (!btf)
> + return NULL;
> + if (!btf->struct_ops_tab)
*ret_cnt = 0;
unless the later patch checks the return value NULL before using *ret_cnt.
Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.
The same should go for the "!btf" case above but I suspect the above !btf check
is unnecessary also and the caller should have checked for !btf itself instead
of expecting a list of struct_ops from a NULL btf. Lets continue the review on
the later patches for now to confirm where the above !btf case might happen.
> + return NULL;
> +
> + *ret_cnt = btf->struct_ops_tab->cnt;
> + return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops;
> +}
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf.
2023-12-15 2:22 ` Martin KaFai Lau
@ 2023-12-15 21:42 ` Kui-Feng Lee
2023-12-16 1:19 ` Martin KaFai Lau
0 siblings, 1 reply; 39+ messages in thread
From: Kui-Feng Lee @ 2023-12-15 21:42 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/14/23 18:22, Martin KaFai Lau wrote:
> On 12/8/23 4:26 PM, thinker.li@gmail.com wrote:
>> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf,
>> u32 *ret_cnt)
>> +{
>> + if (!btf)
>> + return NULL;
>> + if (!btf->struct_ops_tab)
>
> *ret_cnt = 0;
>
> unless the later patch checks the return value NULL before using *ret_cnt.
> Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.
>
> The same should go for the "!btf" case above but I suspect the above
> !btf check is unnecessary also and the caller should have checked for
> !btf itself instead of expecting a list of struct_ops from a NULL btf.
> Lets continue the review on the later patches for now to confirm where
> the above !btf case might happen.
Checking callers, I didn't find anything that make btf here NULL so far.
It is safe to remove !btf check. For the same reason as assigning
*ret_cnt for safe, this check should be fine here as well, right?
I don't have strong opinion here. What I though is to keep the values
as it is without any side-effect if the function call fails and if
possible. And, the callers should not expect the callee to set some
specific values when a call fails.
>
>> + return NULL;
>> +
>> + *ret_cnt = btf->struct_ops_tab->cnt;
>> + return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops;
>> +}
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf.
2023-12-15 21:42 ` Kui-Feng Lee
@ 2023-12-16 1:19 ` Martin KaFai Lau
2023-12-16 5:43 ` Kui-Feng Lee
0 siblings, 1 reply; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-16 1:19 UTC (permalink / raw)
To: Kui-Feng Lee, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/15/23 1:42 PM, Kui-Feng Lee wrote:
>
>
> On 12/14/23 18:22, Martin KaFai Lau wrote:
>> On 12/8/23 4:26 PM, thinker.li@gmail.com wrote:
>>> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32
>>> *ret_cnt)
>>> +{
>>> + if (!btf)
>>> + return NULL;
>>> + if (!btf->struct_ops_tab)
>>
>> *ret_cnt = 0;
>>
>> unless the later patch checks the return value NULL before using *ret_cnt.
>> Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.
>>
>> The same should go for the "!btf" case above but I suspect the above !btf
>> check is unnecessary also and the caller should have checked for !btf itself
>> instead of expecting a list of struct_ops from a NULL btf. Lets continue the
>> review on the later patches for now to confirm where the above !btf case might
>> happen.
>
> Checking callers, I didn't find anything that make btf here NULL so far.
> It is safe to remove !btf check. For the same reason as assigning
> *ret_cnt for safe, this check should be fine here as well, right?
If for safety, why ref_cnt is not checked for NULL also? The userspace passed-in
btf should have been checked for NULL long time before reaching here. There is
no need to be over protective here. It would really need a BUG_ON instead if btf
was NULL here (don't add a BUG_ON though).
afaict, no function in btf.c is checking the btf argument for NULL also.
>
> I don't have strong opinion here. What I though is to keep the values
> as it is without any side-effect if the function call fails and if
> possible. And, the callers should not expect the callee to set some
> specific values when a call fails.
For *ref_cnt stay uninit, there is a bug in patch 10 which exactly assumes 0 is
set in *ret_cnt when there is no struct_ops. It is a good signal on how this
function will be used.
I think it is arguable whether returning NULL here is failure. I would argue
getting a 0 struct_ops_desc array is not a failure. It is why the !btf case
confuses the return NULL case to mean a never would happen error instead of
meaning there is no struct_ops. Taking out the !btf case, NULL means there is no
struct_ops (instead of failure), so 0 cnt.
Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the local cnt
0 and write a warning comment here in btf_get_struct_ops() saying ret_cnt won't
be set when there is no struct_ops in the btf.
When looking at it again, how about moving the bpf_struct_ops_find_*() to btf.c.
Then it will avoid the need of the new btf_get_struct_ops() function.
bpf_struct_ops_find_*() can directly use the btf->struct_ops_tab.
>
>>
>>> + return NULL;
>>> +
>>> + *ret_cnt = btf->struct_ops_tab->cnt;
>>> + return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops;
>>> +}
>>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf.
2023-12-16 1:19 ` Martin KaFai Lau
@ 2023-12-16 5:43 ` Kui-Feng Lee
2023-12-16 16:48 ` Martin KaFai Lau
0 siblings, 1 reply; 39+ messages in thread
From: Kui-Feng Lee @ 2023-12-16 5:43 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/15/23 17:19, Martin KaFai Lau wrote:
> On 12/15/23 1:42 PM, Kui-Feng Lee wrote:
>>
>>
>> On 12/14/23 18:22, Martin KaFai Lau wrote:
>>> On 12/8/23 4:26 PM, thinker.li@gmail.com wrote:
>>>> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf
>>>> *btf, u32 *ret_cnt)
>>>> +{
>>>> + if (!btf)
>>>> + return NULL;
>>>> + if (!btf->struct_ops_tab)
>>>
>>> *ret_cnt = 0;
>>>
>>> unless the later patch checks the return value NULL before using
>>> *ret_cnt.
>>> Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.
>>>
>>> The same should go for the "!btf" case above but I suspect the above
>>> !btf check is unnecessary also and the caller should have checked for
>>> !btf itself instead of expecting a list of struct_ops from a NULL
>>> btf. Lets continue the review on the later patches for now to confirm
>>> where the above !btf case might happen.
>>
>> Checking callers, I didn't find anything that make btf here NULL so far.
>
>> It is safe to remove !btf check. For the same reason as assigning
>> *ret_cnt for safe, this check should be fine here as well, right?
>
> If for safety, why ref_cnt is not checked for NULL also? The userspace
> passed-in btf should have been checked for NULL long time before
> reaching here. There is no need to be over protective here. It would
> really need a BUG_ON instead if btf was NULL here (don't add a BUG_ON
> though).
>
> afaict, no function in btf.c is checking the btf argument for NULL also.
>
>>
>> I don't have strong opinion here. What I though is to keep the values
>> as it is without any side-effect if the function call fails and if
>> possible. And, the callers should not expect the callee to set some
>> specific values when a call fails.
>
> For *ref_cnt stay uninit, there is a bug in patch 10 which exactly
> assumes 0 is set in *ret_cnt when there is no struct_ops. It is a good
> signal on how this function will be used.
>
> I think it is arguable whether returning NULL here is failure. I would
> argue getting a 0 struct_ops_desc array is not a failure. It is why the
> !btf case confuses the return NULL case to mean a never would happen
> error instead of meaning there is no struct_ops. Taking out the !btf
> case, NULL means there is no struct_ops (instead of failure), so 0 cnt.
>
> Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the
> local cnt 0 and write a warning comment here in btf_get_struct_ops()
> saying ret_cnt won't be set when there is no struct_ops in the btf.
I will fix at the patch 10 by setting local cnt 0.
>
> When looking at it again, how about moving the bpf_struct_ops_find_*()
> to btf.c. Then it will avoid the need of the new btf_get_struct_ops()
> function. bpf_struct_ops_find_*() can directly use the btf->struct_ops_tab.
>
I prefer to keep them in bpf_struct_ops.c if it is ok to you.
Fixing the initialization issue of bpf_struct_ops_find()
should be enough.
>
>>
>>>
>>>> + return NULL;
>>>> +
>>>> + *ret_cnt = btf->struct_ops_tab->cnt;
>>>> + return (const struct bpf_struct_ops_desc
>>>> *)btf->struct_ops_tab->ops;
>>>> +}
>>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf.
2023-12-16 5:43 ` Kui-Feng Lee
@ 2023-12-16 16:48 ` Martin KaFai Lau
2023-12-17 7:09 ` Kui-Feng Lee
0 siblings, 1 reply; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-16 16:48 UTC (permalink / raw)
To: Kui-Feng Lee, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/15/23 9:43 PM, Kui-Feng Lee wrote:
>
>
> On 12/15/23 17:19, Martin KaFai Lau wrote:
>> On 12/15/23 1:42 PM, Kui-Feng Lee wrote:
>>>
>>>
>>> On 12/14/23 18:22, Martin KaFai Lau wrote:
>>>> On 12/8/23 4:26 PM, thinker.li@gmail.com wrote:
>>>>> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32
>>>>> *ret_cnt)
>>>>> +{
>>>>> + if (!btf)
>>>>> + return NULL;
>>>>> + if (!btf->struct_ops_tab)
>>>>
>>>> *ret_cnt = 0;
>>>>
>>>> unless the later patch checks the return value NULL before using *ret_cnt.
>>>> Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.
>>>>
>>>> The same should go for the "!btf" case above but I suspect the above !btf
>>>> check is unnecessary also and the caller should have checked for !btf itself
>>>> instead of expecting a list of struct_ops from a NULL btf. Lets continue the
>>>> review on the later patches for now to confirm where the above !btf case
>>>> might happen.
>>>
>>> Checking callers, I didn't find anything that make btf here NULL so far.
>>
>>> It is safe to remove !btf check. For the same reason as assigning
>>> *ret_cnt for safe, this check should be fine here as well, right?
>>
>> If for safety, why ref_cnt is not checked for NULL also? The userspace
>> passed-in btf should have been checked for NULL long time before reaching
>> here. There is no need to be over protective here. It would really need a
>> BUG_ON instead if btf was NULL here (don't add a BUG_ON though).
>>
>> afaict, no function in btf.c is checking the btf argument for NULL also.
>>
>>>
>>> I don't have strong opinion here. What I though is to keep the values
>>> as it is without any side-effect if the function call fails and if
>>> possible. And, the callers should not expect the callee to set some
>>> specific values when a call fails.
>>
>> For *ref_cnt stay uninit, there is a bug in patch 10 which exactly assumes 0
>> is set in *ret_cnt when there is no struct_ops. It is a good signal on how
>> this function will be used.
>>
>> I think it is arguable whether returning NULL here is failure. I would argue
>> getting a 0 struct_ops_desc array is not a failure. It is why the !btf case
>> confuses the return NULL case to mean a never would happen error instead of
>> meaning there is no struct_ops. Taking out the !btf case, NULL means there is
>> no struct_ops (instead of failure), so 0 cnt.
>>
>> Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the local
>> cnt 0 and write a warning comment here in btf_get_struct_ops() saying ret_cnt
>> won't be set when there is no struct_ops in the btf.
>
>
> I will fix at the patch 10 by setting local cnt 0.
>
>>
>> When looking at it again, how about moving the bpf_struct_ops_find_*() to
>> btf.c. Then it will avoid the need of the new btf_get_struct_ops() function.
>> bpf_struct_ops_find_*() can directly use the btf->struct_ops_tab.
>>
>
> I prefer to keep them in bpf_struct_ops.c if it is ok to you.
> Fixing the initialization issue of bpf_struct_ops_find()
> should be enough.
If choosing between fixing the bug in patch 10 and moving them to btf.c, I would
avoid patch 10 (and future) bug to begin with by moving them to btf.c. Moving
them should be cheap unless there is other dependency that I have overlooked.
>
>>
>>>
>>>>
>>>>> + return NULL;
>>>>> +
>>>>> + *ret_cnt = btf->struct_ops_tab->cnt;
>>>>> + return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops;
>>>>> +}
>>>>
>>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf.
2023-12-16 16:48 ` Martin KaFai Lau
@ 2023-12-17 7:09 ` Kui-Feng Lee
0 siblings, 0 replies; 39+ messages in thread
From: Kui-Feng Lee @ 2023-12-17 7:09 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/16/23 08:48, Martin KaFai Lau wrote:
> On 12/15/23 9:43 PM, Kui-Feng Lee wrote:
>>
>>
>> On 12/15/23 17:19, Martin KaFai Lau wrote:
>>> On 12/15/23 1:42 PM, Kui-Feng Lee wrote:
>>>>
>>>>
>>>> On 12/14/23 18:22, Martin KaFai Lau wrote:
>>>>> On 12/8/23 4:26 PM, thinker.li@gmail.com wrote:
>>>>>> +const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf
>>>>>> *btf, u32 *ret_cnt)
>>>>>> +{
>>>>>> + if (!btf)
>>>>>> + return NULL;
>>>>>> + if (!btf->struct_ops_tab)
>>>>>
>>>>> *ret_cnt = 0;
>>>>>
>>>>> unless the later patch checks the return value NULL before using
>>>>> *ret_cnt.
>>>>> Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.
>>>>>
>>>>> The same should go for the "!btf" case above but I suspect the
>>>>> above !btf check is unnecessary also and the caller should have
>>>>> checked for !btf itself instead of expecting a list of struct_ops
>>>>> from a NULL btf. Lets continue the review on the later patches for
>>>>> now to confirm where the above !btf case might happen.
>>>>
>>>> Checking callers, I didn't find anything that make btf here NULL so
>>>> far.
>>>
>>>> It is safe to remove !btf check. For the same reason as assigning
>>>> *ret_cnt for safe, this check should be fine here as well, right?
>>>
>>> If for safety, why ref_cnt is not checked for NULL also? The
>>> userspace passed-in btf should have been checked for NULL long time
>>> before reaching here. There is no need to be over protective here. It
>>> would really need a BUG_ON instead if btf was NULL here (don't add a
>>> BUG_ON though).
>>>
>>> afaict, no function in btf.c is checking the btf argument for NULL also.
>>>
>>>>
>>>> I don't have strong opinion here. What I though is to keep the values
>>>> as it is without any side-effect if the function call fails and if
>>>> possible. And, the callers should not expect the callee to set some
>>>> specific values when a call fails.
>>>
>>> For *ref_cnt stay uninit, there is a bug in patch 10 which exactly
>>> assumes 0 is set in *ret_cnt when there is no struct_ops. It is a
>>> good signal on how this function will be used.
>>>
>>> I think it is arguable whether returning NULL here is failure. I
>>> would argue getting a 0 struct_ops_desc array is not a failure. It is
>>> why the !btf case confuses the return NULL case to mean a never would
>>> happen error instead of meaning there is no struct_ops. Taking out
>>> the !btf case, NULL means there is no struct_ops (instead of
>>> failure), so 0 cnt.
>>>
>>> Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the
>>> local cnt 0 and write a warning comment here in btf_get_struct_ops()
>>> saying ret_cnt won't be set when there is no struct_ops in the btf.
>>
>>
>> I will fix at the patch 10 by setting local cnt 0
>>
>>>
>>> When looking at it again, how about moving the
>>> bpf_struct_ops_find_*() to btf.c. Then it will avoid the need of the
>>> new btf_get_struct_ops() function. bpf_struct_ops_find_*() can
>>> directly use the btf->struct_ops_tab.
>>>
>>
>> I prefer to keep them in bpf_struct_ops.c if it is ok to you.
>> Fixing the initialization issue of bpf_struct_ops_find()
>> should be enough.
>
> If choosing between fixing the bug in patch 10 and moving them to btf.c,
> I would avoid patch 10 (and future) bug to begin with by moving them to
> btf.c. Moving them should be cheap unless there is other dependency that
> I have overlooked.
Ok! Got it!
>
>>
>>>
>>>>
>>>>>
>>>>>> + return NULL;
>>>>>> +
>>>>>> + *ret_cnt = btf->struct_ops_tab->cnt;
>>>>>> + return (const struct bpf_struct_ops_desc
>>>>>> *)btf->struct_ops_tab->ops;
>>>>>> +}
>>>>>
>>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH bpf-next v13 05/14] bpf: make struct_ops_map support btfs other than btf_vmlinux.
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
` (3 preceding siblings ...)
2023-12-09 0:26 ` [PATCH bpf-next v13 04/14] bpf: add struct_ops_tab to btf thinker.li
@ 2023-12-09 0:27 ` thinker.li
2023-12-09 0:27 ` [PATCH bpf-next v13 06/14] bpf: lookup struct_ops types from a given module BTF thinker.li
` (8 subsequent siblings)
13 siblings, 0 replies; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:27 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Once new struct_ops can be registered from modules, btf_vmlinux is no
longer the only btf that struct_ops_map would face. st_map should remember
what btf it should use to get type information.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
kernel/bpf/bpf_struct_ops.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 5fbf88e6f4e5..2b0c402740cc 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -46,6 +46,8 @@ struct bpf_struct_ops_map {
* "links[]".
*/
void *image;
+ /* The owner moduler's btf. */
+ struct btf *btf;
/* uvalue->data stores the kernel struct
* (e.g. tcp_congestion_ops) that is more useful
* to userspace than the kvalue. For example,
@@ -314,7 +316,7 @@ static void bpf_struct_ops_map_put_progs(struct bpf_struct_ops_map *st_map)
}
}
-static int check_zero_holes(const struct btf_type *t, void *data)
+static int check_zero_holes(const struct btf *btf, const struct btf_type *t, void *data)
{
const struct btf_member *member;
u32 i, moff, msize, prev_mend = 0;
@@ -326,8 +328,8 @@ static int check_zero_holes(const struct btf_type *t, void *data)
memchr_inv(data + prev_mend, 0, moff - prev_mend))
return -EINVAL;
- mtype = btf_type_by_id(btf_vmlinux, member->type);
- mtype = btf_resolve_size(btf_vmlinux, mtype, &msize);
+ mtype = btf_type_by_id(btf, member->type);
+ mtype = btf_resolve_size(btf, mtype, &msize);
if (IS_ERR(mtype))
return PTR_ERR(mtype);
prev_mend = moff + msize;
@@ -402,12 +404,12 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
if (*(u32 *)key != 0)
return -E2BIG;
- err = check_zero_holes(st_ops_desc->value_type, value);
+ err = check_zero_holes(st_map->btf, st_ops_desc->value_type, value);
if (err)
return err;
uvalue = value;
- err = check_zero_holes(t, uvalue->data);
+ err = check_zero_holes(st_map->btf, t, uvalue->data);
if (err)
return err;
@@ -443,7 +445,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
u32 moff;
moff = __btf_member_bit_offset(t, member) / 8;
- ptype = btf_type_resolve_ptr(btf_vmlinux, member->type, NULL);
+ ptype = btf_type_resolve_ptr(st_map->btf, member->type, NULL);
if (ptype == module_type) {
if (*(void **)(udata + moff))
goto reset_unlock;
@@ -468,8 +470,8 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
if (!ptype || !btf_type_is_func_proto(ptype)) {
u32 msize;
- mtype = btf_type_by_id(btf_vmlinux, member->type);
- mtype = btf_resolve_size(btf_vmlinux, mtype, &msize);
+ mtype = btf_type_by_id(st_map->btf, member->type);
+ mtype = btf_resolve_size(st_map->btf, mtype, &msize);
if (IS_ERR(mtype)) {
err = PTR_ERR(mtype);
goto reset_unlock;
@@ -607,6 +609,7 @@ static long bpf_struct_ops_map_delete_elem(struct bpf_map *map, void *key)
static void bpf_struct_ops_map_seq_show_elem(struct bpf_map *map, void *key,
struct seq_file *m)
{
+ struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
void *value;
int err;
@@ -616,7 +619,8 @@ static void bpf_struct_ops_map_seq_show_elem(struct bpf_map *map, void *key,
err = bpf_struct_ops_map_sys_lookup_elem(map, key, value);
if (!err) {
- btf_type_seq_show(btf_vmlinux, map->btf_vmlinux_value_type_id,
+ btf_type_seq_show(st_map->btf,
+ map->btf_vmlinux_value_type_id,
value, m);
seq_puts(m, "\n");
}
@@ -726,6 +730,8 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
return ERR_PTR(-ENOMEM);
}
+ st_map->btf = btf_vmlinux;
+
mutex_init(&st_map->lock);
bpf_map_init_from_attr(map, attr);
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH bpf-next v13 06/14] bpf: lookup struct_ops types from a given module BTF.
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
` (4 preceding siblings ...)
2023-12-09 0:27 ` [PATCH bpf-next v13 05/14] bpf: make struct_ops_map support btfs other than btf_vmlinux thinker.li
@ 2023-12-09 0:27 ` thinker.li
2023-12-09 0:27 ` [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem thinker.li
` (7 subsequent siblings)
13 siblings, 0 replies; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:27 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
This is a preparation for searching for struct_ops types from a specified
module. BTF is always btf_vmlinux now. This patch passes a pointer of BTF
to bpf_struct_ops_find_value() and bpf_struct_ops_find(). Once the new
registration API of struct_ops types is used, other BTFs besides
btf_vmlinux can also be passed to them.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/bpf.h | 4 ++--
kernel/bpf/bpf_struct_ops.c | 11 ++++++-----
kernel/bpf/verifier.c | 2 +-
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index fa43255f59bc..91bcd62d6fcf 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1696,7 +1696,7 @@ struct bpf_struct_ops_desc {
#if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
#define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
-const struct bpf_struct_ops_desc *bpf_struct_ops_find(u32 type_id);
+const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *btf, u32 type_id);
void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log);
bool bpf_struct_ops_get(const void *kdata);
void bpf_struct_ops_put(const void *kdata);
@@ -1739,7 +1739,7 @@ int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
union bpf_attr __user *uattr);
#endif
#else
-static inline const struct bpf_struct_ops_desc *bpf_struct_ops_find(u32 type_id)
+static inline const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *btf, u32 type_id)
{
return NULL;
}
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 2b0c402740cc..ed4d84a8437c 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -221,11 +221,11 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
extern struct btf *btf_vmlinux;
static const struct bpf_struct_ops_desc *
-bpf_struct_ops_find_value(u32 value_id)
+bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
{
unsigned int i;
- if (!value_id || !btf_vmlinux)
+ if (!value_id || !btf)
return NULL;
for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
@@ -236,11 +236,12 @@ bpf_struct_ops_find_value(u32 value_id)
return NULL;
}
-const struct bpf_struct_ops_desc *bpf_struct_ops_find(u32 type_id)
+const struct bpf_struct_ops_desc *
+bpf_struct_ops_find(struct btf *btf, u32 type_id)
{
unsigned int i;
- if (!type_id || !btf_vmlinux)
+ if (!type_id || !btf)
return NULL;
for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
@@ -682,7 +683,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
struct bpf_map *map;
int ret;
- st_ops_desc = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
+ st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, attr->btf_vmlinux_value_type_id);
if (!st_ops_desc)
return ERR_PTR(-ENOTSUPP);
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ce41bc17ac10..6b45f56f8d4c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20078,7 +20078,7 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
}
btf_id = prog->aux->attach_btf_id;
- st_ops_desc = bpf_struct_ops_find(btf_id);
+ st_ops_desc = bpf_struct_ops_find(btf_vmlinux, btf_id);
if (!st_ops_desc) {
verbose(env, "attach_btf_id %u is not a supported struct\n",
btf_id);
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
` (5 preceding siblings ...)
2023-12-09 0:27 ` [PATCH bpf-next v13 06/14] bpf: lookup struct_ops types from a given module BTF thinker.li
@ 2023-12-09 0:27 ` thinker.li
2023-12-15 2:44 ` Martin KaFai Lau
2023-12-09 0:27 ` [PATCH bpf-next v13 08/14] bpf: hold module for bpf_struct_ops_map thinker.li
` (6 subsequent siblings)
13 siblings, 1 reply; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:27 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Pass the fd of a btf from the userspace to the bpf() syscall, and then
convert the fd into a btf. The btf is generated from the module that
defines the target BPF struct_ops type.
In order to inform the kernel about the module that defines the target
struct_ops type, the userspace program needs to provide a btf fd for the
respective module's btf. This btf contains essential information on the
types defined within the module, including the target struct_ops type.
A btf fd must be provided to the kernel for struct_ops maps struct_ops and
for the bpf programs attached to those maps.
In the case of the bpf programs, the attach_btf_obj_fd parameter is passed
as part of the bpf_attr and is converted into a btf. This btf is then
stored in the prog->aux->attach_btf field. Here, it just let the verifier
access attach_btf directly.
In the case of struct_ops maps, a btf fd is passed as value_type_btf_obj_fd
of bpf_attr. The bpf_struct_ops_map_alloc() function converts the fd to a
btf and stores it as st_map->btf.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/uapi/linux/bpf.h | 5 +++
kernel/bpf/bpf_struct_ops.c | 56 +++++++++++++++++++++++-----------
kernel/bpf/syscall.c | 2 +-
kernel/bpf/verifier.c | 9 ++++--
tools/include/uapi/linux/bpf.h | 5 +++
5 files changed, 56 insertions(+), 21 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index e0545201b55f..5c3838a97554 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1438,6 +1438,11 @@ union bpf_attr {
*/
__u64 map_extra;
__u32 map_token_fd;
+
+ __u32 value_type_btf_obj_fd; /* fd pointing to a BTF
+ * type data for
+ * btf_vmlinux_value_type_id.
+ */
};
struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index ed4d84a8437c..f943f8378e76 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -641,6 +641,7 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
bpf_jit_uncharge_modmem(PAGE_SIZE);
}
bpf_map_area_free(st_map->uvalue);
+ btf_put(st_map->btf);
bpf_map_area_free(st_map);
}
@@ -681,15 +682,30 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
struct bpf_struct_ops_map *st_map;
const struct btf_type *t, *vt;
struct bpf_map *map;
+ struct btf *btf;
int ret;
- st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, attr->btf_vmlinux_value_type_id);
- if (!st_ops_desc)
- return ERR_PTR(-ENOTSUPP);
+ if (attr->value_type_btf_obj_fd) {
+ /* The map holds btf for its whole life time. */
+ btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
+ if (IS_ERR(btf))
+ return ERR_PTR(PTR_ERR(btf));
+ } else {
+ btf = btf_vmlinux;
+ btf_get(btf);
+ }
+
+ st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
+ if (!st_ops_desc) {
+ ret = -ENOTSUPP;
+ goto errout;
+ }
vt = st_ops_desc->value_type;
- if (attr->value_size != vt->size)
- return ERR_PTR(-EINVAL);
+ if (attr->value_size != vt->size) {
+ ret = -EINVAL;
+ goto errout;
+ }
t = st_ops_desc->type;
@@ -700,17 +716,17 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
(vt->size - sizeof(struct bpf_struct_ops_value));
st_map = bpf_map_area_alloc(st_map_size, NUMA_NO_NODE);
- if (!st_map)
- return ERR_PTR(-ENOMEM);
+ if (!st_map) {
+ ret = -ENOMEM;
+ goto errout;
+ }
st_map->st_ops_desc = st_ops_desc;
map = &st_map->map;
ret = bpf_jit_charge_modmem(PAGE_SIZE);
- if (ret) {
- __bpf_struct_ops_map_free(map);
- return ERR_PTR(ret);
- }
+ if (ret)
+ goto errout_free;
st_map->image = arch_alloc_bpf_trampoline(PAGE_SIZE);
if (!st_map->image) {
@@ -719,24 +735,30 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
* here.
*/
bpf_jit_uncharge_modmem(PAGE_SIZE);
- __bpf_struct_ops_map_free(map);
- return ERR_PTR(-ENOMEM);
+ ret = -ENOMEM;
+ goto errout_free;
}
st_map->uvalue = bpf_map_area_alloc(vt->size, NUMA_NO_NODE);
st_map->links =
bpf_map_area_alloc(btf_type_vlen(t) * sizeof(struct bpf_links *),
NUMA_NO_NODE);
if (!st_map->uvalue || !st_map->links) {
- __bpf_struct_ops_map_free(map);
- return ERR_PTR(-ENOMEM);
+ ret = -ENOMEM;
+ goto errout_free;
}
-
- st_map->btf = btf_vmlinux;
+ st_map->btf = btf;
mutex_init(&st_map->lock);
bpf_map_init_from_attr(map, attr);
return map;
+
+errout_free:
+ __bpf_struct_ops_map_free(map);
+errout:
+ btf_put(btf);
+
+ return ERR_PTR(ret);
}
static u64 bpf_struct_ops_map_mem_usage(const struct bpf_map *map)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index aff045eed375..4aced7e58904 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1126,7 +1126,7 @@ static bool bpf_net_capable(void)
return capable(CAP_NET_ADMIN) || capable(CAP_SYS_ADMIN);
}
-#define BPF_MAP_CREATE_LAST_FIELD map_token_fd
+#define BPF_MAP_CREATE_LAST_FIELD value_type_btf_obj_fd
/* called via syscall */
static int map_create(union bpf_attr *attr)
{
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6b45f56f8d4c..795c16f9cf57 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20070,6 +20070,7 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
const struct btf_member *member;
struct bpf_prog *prog = env->prog;
u32 btf_id, member_idx;
+ struct btf *btf;
const char *mname;
if (!prog->gpl_compatible) {
@@ -20077,8 +20078,10 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
return -EINVAL;
}
+ btf = prog->aux->attach_btf;
+
btf_id = prog->aux->attach_btf_id;
- st_ops_desc = bpf_struct_ops_find(btf_vmlinux, btf_id);
+ st_ops_desc = bpf_struct_ops_find(btf, btf_id);
if (!st_ops_desc) {
verbose(env, "attach_btf_id %u is not a supported struct\n",
btf_id);
@@ -20095,8 +20098,8 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
}
member = &btf_type_member(t)[member_idx];
- mname = btf_name_by_offset(btf_vmlinux, member->name_off);
- func_proto = btf_type_resolve_func_ptr(btf_vmlinux, member->type,
+ mname = btf_name_by_offset(btf, member->name_off);
+ func_proto = btf_type_resolve_func_ptr(btf, member->type,
NULL);
if (!func_proto) {
verbose(env, "attach to invalid member %s(@idx %u) of struct %s\n",
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index e0545201b55f..5c3838a97554 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1438,6 +1438,11 @@ union bpf_attr {
*/
__u64 map_extra;
__u32 map_token_fd;
+
+ __u32 value_type_btf_obj_fd; /* fd pointing to a BTF
+ * type data for
+ * btf_vmlinux_value_type_id.
+ */
};
struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem
2023-12-09 0:27 ` [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem thinker.li
@ 2023-12-15 2:44 ` Martin KaFai Lau
2023-12-15 22:10 ` Kui-Feng Lee
0 siblings, 1 reply; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-15 2:44 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
> @@ -681,15 +682,30 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
> struct bpf_struct_ops_map *st_map;
> const struct btf_type *t, *vt;
> struct bpf_map *map;
> + struct btf *btf;
> int ret;
>
> - st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, attr->btf_vmlinux_value_type_id);
> - if (!st_ops_desc)
> - return ERR_PTR(-ENOTSUPP);
> + if (attr->value_type_btf_obj_fd) {
> + /* The map holds btf for its whole life time. */
> + btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
> + if (IS_ERR(btf))
> + return ERR_PTR(PTR_ERR(btf));
return ERR_CAST(btf);
It needs to check for btf_is_module:
if (!btf_is_module(btf)) {
btf_put(btf);
return ERR_PTR(-EINVAL);
}
> + } else {
> + btf = btf_vmlinux;
> + btf_get(btf);
btf_vmlinux could be NULL or a ERR_PTR? Lets take this chance to use
bpf_get_btf_vmlinux():
btf = bpf_get_btf_vmlinux();
if (IS_ERR(btf))
return ERR_CAST(btf);
Going back to patch 4. This should be the only case that btf will be NULL or
ERR_PTR?
will continue the remaining review later tonight.
pw-bot: cr
> + }
> +
> + st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
> + if (!st_ops_desc) {
> + ret = -ENOTSUPP;
> + goto errout;
> + }
>
> vt = st_ops_desc->value_type;
> - if (attr->value_size != vt->size)
> - return ERR_PTR(-EINVAL);
> + if (attr->value_size != vt->size) {
> + ret = -EINVAL;
> + goto errout;
> + }
>
> t = st_ops_desc->type;
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem
2023-12-15 2:44 ` Martin KaFai Lau
@ 2023-12-15 22:10 ` Kui-Feng Lee
2023-12-16 0:19 ` Martin KaFai Lau
0 siblings, 1 reply; 39+ messages in thread
From: Kui-Feng Lee @ 2023-12-15 22:10 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/14/23 18:44, Martin KaFai Lau wrote:
> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>> @@ -681,15 +682,30 @@ static struct bpf_map
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>> struct bpf_struct_ops_map *st_map;
>> const struct btf_type *t, *vt;
>> struct bpf_map *map;
>> + struct btf *btf;
>> int ret;
>> - st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux,
>> attr->btf_vmlinux_value_type_id);
>> - if (!st_ops_desc)
>> - return ERR_PTR(-ENOTSUPP);
>> + if (attr->value_type_btf_obj_fd) {
>> + /* The map holds btf for its whole life time. */
>> + btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>> + if (IS_ERR(btf))
>> + return ERR_PTR(PTR_ERR(btf));
>
> return ERR_CAST(btf);
>
> It needs to check for btf_is_module:
>
> if (!btf_is_module(btf)) {
> btf_put(btf);
> return ERR_PTR(-EINVAL);
> }
Even btf is btf_vmlinux the kernel's btf, it still works.
Although libbpf pass 0 as the value of value_type_btf_obj_fd for
btf_vmlinux now, it should be OK for a user space loader to
pass a fd of btf_vmlinux.
WDYT?
>
>> + } else {
>> + btf = btf_vmlinux;
>> + btf_get(btf);
>
> btf_vmlinux could be NULL or a ERR_PTR? Lets take this chance to use
> bpf_get_btf_vmlinux():
> btf = bpf_get_btf_vmlinux();
> if (IS_ERR(btf))
> return ERR_CAST(btf);
Sure!
>
> Going back to patch 4. This should be the only case that btf will be
> NULL or ERR_PTR?
>
> will continue the remaining review later tonight.
>
> pw-bot: cr
>
>> + }
>> +
>> + st_ops_desc = bpf_struct_ops_find_value(btf,
>> attr->btf_vmlinux_value_type_id);
>> + if (!st_ops_desc) {
>> + ret = -ENOTSUPP;
>> + goto errout;
>> + }
>> vt = st_ops_desc->value_type;
>> - if (attr->value_size != vt->size)
>> - return ERR_PTR(-EINVAL);
>> + if (attr->value_size != vt->size) {
>> + ret = -EINVAL;
>> + goto errout;
>> + }
>> t = st_ops_desc->type;
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem
2023-12-15 22:10 ` Kui-Feng Lee
@ 2023-12-16 0:19 ` Martin KaFai Lau
2023-12-16 5:55 ` Kui-Feng Lee
0 siblings, 1 reply; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-16 0:19 UTC (permalink / raw)
To: Kui-Feng Lee, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/15/23 2:10 PM, Kui-Feng Lee wrote:
>
>
> On 12/14/23 18:44, Martin KaFai Lau wrote:
>> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>>> @@ -681,15 +682,30 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union
>>> bpf_attr *attr)
>>> struct bpf_struct_ops_map *st_map;
>>> const struct btf_type *t, *vt;
>>> struct bpf_map *map;
>>> + struct btf *btf;
>>> int ret;
>>> - st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux,
>>> attr->btf_vmlinux_value_type_id);
>>> - if (!st_ops_desc)
>>> - return ERR_PTR(-ENOTSUPP);
>>> + if (attr->value_type_btf_obj_fd) {
>>> + /* The map holds btf for its whole life time. */
>>> + btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>>> + if (IS_ERR(btf))
>>> + return ERR_PTR(PTR_ERR(btf));
>>
>> return ERR_CAST(btf);
>>
>> It needs to check for btf_is_module:
>>
>> if (!btf_is_module(btf)) {
>> btf_put(btf);
>> return ERR_PTR(-EINVAL);
>> }
>
> Even btf is btf_vmlinux the kernel's btf, it still works.
btf could be a bpf program's btf. It needs to ensure it is a kernel module btf
here.
> Although libbpf pass 0 as the value of value_type_btf_obj_fd for
> btf_vmlinux now, it should be OK for a user space loader to
> pass a fd of btf_vmlinux.
>
> WDYT?
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem
2023-12-16 0:19 ` Martin KaFai Lau
@ 2023-12-16 5:55 ` Kui-Feng Lee
2023-12-16 6:07 ` Kui-Feng Lee
0 siblings, 1 reply; 39+ messages in thread
From: Kui-Feng Lee @ 2023-12-16 5:55 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/15/23 16:19, Martin KaFai Lau wrote:
> On 12/15/23 2:10 PM, Kui-Feng Lee wrote:
>>
>>
>> On 12/14/23 18:44, Martin KaFai Lau wrote:
>>> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>>>> @@ -681,15 +682,30 @@ static struct bpf_map
>>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>> struct bpf_struct_ops_map *st_map;
>>>> const struct btf_type *t, *vt;
>>>> struct bpf_map *map;
>>>> + struct btf *btf;
>>>> int ret;
>>>> - st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux,
>>>> attr->btf_vmlinux_value_type_id);
>>>> - if (!st_ops_desc)
>>>> - return ERR_PTR(-ENOTSUPP);
>>>> + if (attr->value_type_btf_obj_fd) {
>>>> + /* The map holds btf for its whole life time. */
>>>> + btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>>>> + if (IS_ERR(btf))
>>>> + return ERR_PTR(PTR_ERR(btf));
>>>
>>> return ERR_CAST(btf);
>>>
>>> It needs to check for btf_is_module:
>>>
>>> if (!btf_is_module(btf)) {
>>> btf_put(btf);
>>> return ERR_PTR(-EINVAL);
>>> }
>>
>> Even btf is btf_vmlinux the kernel's btf, it still works.
>
> btf could be a bpf program's btf. It needs to ensure it is a kernel
> module btf here.
Got it!
>
>> Although libbpf pass 0 as the value of value_type_btf_obj_fd for
>> btf_vmlinux now, it should be OK for a user space loader to
>> pass a fd of btf_vmlinux.
>>
>> WDYT?
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem
2023-12-16 5:55 ` Kui-Feng Lee
@ 2023-12-16 6:07 ` Kui-Feng Lee
2023-12-16 16:41 ` Martin KaFai Lau
0 siblings, 1 reply; 39+ messages in thread
From: Kui-Feng Lee @ 2023-12-16 6:07 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/15/23 21:55, Kui-Feng Lee wrote:
>
>
> On 12/15/23 16:19, Martin KaFai Lau wrote:
>> On 12/15/23 2:10 PM, Kui-Feng Lee wrote:
>>>
>>>
>>> On 12/14/23 18:44, Martin KaFai Lau wrote:
>>>> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>>>>> @@ -681,15 +682,30 @@ static struct bpf_map
>>>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>>> struct bpf_struct_ops_map *st_map;
>>>>> const struct btf_type *t, *vt;
>>>>> struct bpf_map *map;
>>>>> + struct btf *btf;
>>>>> int ret;
>>>>> - st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux,
>>>>> attr->btf_vmlinux_value_type_id);
>>>>> - if (!st_ops_desc)
>>>>> - return ERR_PTR(-ENOTSUPP);
>>>>> + if (attr->value_type_btf_obj_fd) {
>>>>> + /* The map holds btf for its whole life time. */
>>>>> + btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>>>>> + if (IS_ERR(btf))
>>>>> + return ERR_PTR(PTR_ERR(btf));
>>>>
>>>> return ERR_CAST(btf);
>>>>
>>>> It needs to check for btf_is_module:
>>>>
>>>> if (!btf_is_module(btf)) {
>>>> btf_put(btf);
>>>> return ERR_PTR(-EINVAL);
>>>> }
>>>
>>> Even btf is btf_vmlinux the kernel's btf, it still works.
>>
>> btf could be a bpf program's btf. It needs to ensure it is a kernel
>> module btf here.
>
> Got it!
Isn't btf_is_kernel() better here?
User space may pass a fd to btf_vmlinux.
>
>>
>>> Although libbpf pass 0 as the value of value_type_btf_obj_fd for
>>> btf_vmlinux now, it should be OK for a user space loader to
>>> pass a fd of btf_vmlinux.
>>>
>>> WDYT?
>>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem
2023-12-16 6:07 ` Kui-Feng Lee
@ 2023-12-16 16:41 ` Martin KaFai Lau
2023-12-16 19:38 ` Kui-Feng Lee
0 siblings, 1 reply; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-16 16:41 UTC (permalink / raw)
To: Kui-Feng Lee, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/15/23 10:07 PM, Kui-Feng Lee wrote:
>
>
> On 12/15/23 21:55, Kui-Feng Lee wrote:
>>
>>
>> On 12/15/23 16:19, Martin KaFai Lau wrote:
>>> On 12/15/23 2:10 PM, Kui-Feng Lee wrote:
>>>>
>>>>
>>>> On 12/14/23 18:44, Martin KaFai Lau wrote:
>>>>> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>>>>>> @@ -681,15 +682,30 @@ static struct bpf_map
>>>>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>>>> struct bpf_struct_ops_map *st_map;
>>>>>> const struct btf_type *t, *vt;
>>>>>> struct bpf_map *map;
>>>>>> + struct btf *btf;
>>>>>> int ret;
>>>>>> - st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux,
>>>>>> attr->btf_vmlinux_value_type_id);
>>>>>> - if (!st_ops_desc)
>>>>>> - return ERR_PTR(-ENOTSUPP);
>>>>>> + if (attr->value_type_btf_obj_fd) {
>>>>>> + /* The map holds btf for its whole life time. */
>>>>>> + btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>>>>>> + if (IS_ERR(btf))
>>>>>> + return ERR_PTR(PTR_ERR(btf));
>>>>>
>>>>> return ERR_CAST(btf);
>>>>>
>>>>> It needs to check for btf_is_module:
>>>>>
>>>>> if (!btf_is_module(btf)) {
>>>>> btf_put(btf);
>>>>> return ERR_PTR(-EINVAL);
>>>>> }
>>>>
>>>> Even btf is btf_vmlinux the kernel's btf, it still works.
>>>
>>> btf could be a bpf program's btf. It needs to ensure it is a kernel module
>>> btf here.
>>
>> Got it!
>
> Isn't btf_is_kernel() better here?
> User space may pass a fd to btf_vmlinux.
Limit it to btf_is_module. What is the benefit of supporting btf_vmlinux as a fd
while fd 0 already means btf_vmlinux?
kfunc does not support the btf_vmlinux as fd also, supporting in struct_ops
alone is confusing. I don't think the major user (libbpf) does this either, so
really not much usage other than a confusing API to have both fd == 0 and a
btf_vmlinux's fd to mean btf_vmlinux.
>
>>
>>>
>>>> Although libbpf pass 0 as the value of value_type_btf_obj_fd for
>>>> btf_vmlinux now, it should be OK for a user space loader to
>>>> pass a fd of btf_vmlinux.
>>>>
>>>> WDYT?
>>>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem
2023-12-16 16:41 ` Martin KaFai Lau
@ 2023-12-16 19:38 ` Kui-Feng Lee
0 siblings, 0 replies; 39+ messages in thread
From: Kui-Feng Lee @ 2023-12-16 19:38 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/16/23 08:41, Martin KaFai Lau wrote:
> On 12/15/23 10:07 PM, Kui-Feng Lee wrote:
>>
>>
>> On 12/15/23 21:55, Kui-Feng Lee wrote:
>>>
>>>
>>> On 12/15/23 16:19, Martin KaFai Lau wrote:
>>>> On 12/15/23 2:10 PM, Kui-Feng Lee wrote:
>>>>>
>>>>>
>>>>> On 12/14/23 18:44, Martin KaFai Lau wrote:
>>>>>> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>>>>>>> @@ -681,15 +682,30 @@ static struct bpf_map
>>>>>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>>>>> struct bpf_struct_ops_map *st_map;
>>>>>>> const struct btf_type *t, *vt;
>>>>>>> struct bpf_map *map;
>>>>>>> + struct btf *btf;
>>>>>>> int ret;
>>>>>>> - st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux,
>>>>>>> attr->btf_vmlinux_value_type_id);
>>>>>>> - if (!st_ops_desc)
>>>>>>> - return ERR_PTR(-ENOTSUPP);
>>>>>>> + if (attr->value_type_btf_obj_fd) {
>>>>>>> + /* The map holds btf for its whole life time. */
>>>>>>> + btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>>>>>>> + if (IS_ERR(btf))
>>>>>>> + return ERR_PTR(PTR_ERR(btf));
>>>>>>
>>>>>> return ERR_CAST(btf);
>>>>>>
>>>>>> It needs to check for btf_is_module:
>>>>>>
>>>>>> if (!btf_is_module(btf)) {
>>>>>> btf_put(btf);
>>>>>> return ERR_PTR(-EINVAL);
>>>>>> }
>>>>>
>>>>> Even btf is btf_vmlinux the kernel's btf, it still works.
>>>>
>>>> btf could be a bpf program's btf. It needs to ensure it is a kernel
>>>> module btf here.
>>>
>>> Got it!
>>
>> Isn't btf_is_kernel() better here?
>> User space may pass a fd to btf_vmlinux.
>
> Limit it to btf_is_module. What is the benefit of supporting btf_vmlinux
> as a fd while fd 0 already means btf_vmlinux?
>
> kfunc does not support the btf_vmlinux as fd also, supporting in
> struct_ops alone is confusing. I don't think the major user (libbpf)
> does this either, so really not much usage other than a confusing API to
> have both fd == 0 and a btf_vmlinux's fd to mean btf_vmlinux.
It is fair.
>
>>
>>>
>>>>
>>>>> Although libbpf pass 0 as the value of value_type_btf_obj_fd for
>>>>> btf_vmlinux now, it should be OK for a user space loader to
>>>>> pass a fd of btf_vmlinux.
>>>>>
>>>>> WDYT?
>>>>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH bpf-next v13 08/14] bpf: hold module for bpf_struct_ops_map.
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
` (6 preceding siblings ...)
2023-12-09 0:27 ` [PATCH bpf-next v13 07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem thinker.li
@ 2023-12-09 0:27 ` thinker.li
2023-12-15 5:54 ` Martin KaFai Lau
2023-12-09 0:27 ` [PATCH bpf-next v13 09/14] bpf: validate value_type thinker.li
` (5 subsequent siblings)
13 siblings, 1 reply; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:27 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
To ensure that a module remains accessible whenever a struct_ops object of
a struct_ops type provided by the module is still in use.
struct bpf_strct_ops_map doesn't hold a refcnt to btf anymore sicne a
module will hold a refcnt to it's btf already. But, struct_ops programs are
different. They hold their associated btf, not the module since they need
only btf to assure their types (signatures).
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/bpf.h | 1 +
include/linux/bpf_verifier.h | 1 +
kernel/bpf/bpf_struct_ops.c | 28 +++++++++++++++++++++++-----
kernel/bpf/verifier.c | 10 ++++++++++
4 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 91bcd62d6fcf..c5c7cc4552f5 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1681,6 +1681,7 @@ struct bpf_struct_ops {
void (*unreg)(void *kdata);
int (*update)(void *kdata, void *old_kdata);
int (*validate)(void *kdata);
+ struct module *owner;
const char *name;
struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
};
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 314b679fb494..01113bcdd479 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -651,6 +651,7 @@ struct bpf_verifier_env {
u32 prev_insn_idx;
struct bpf_prog *prog; /* eBPF program being verified */
const struct bpf_verifier_ops *ops;
+ struct module *attach_btf_mod; /* The owner module of prog->aux->attach_btf */
struct bpf_verifier_stack_elem *head; /* stack of verifier states to be processed */
int stack_size; /* number of states to be processed */
bool strict_alignment; /* perform strict pointer alignment checks */
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index f943f8378e76..a838f7c7d583 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -641,12 +641,15 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
bpf_jit_uncharge_modmem(PAGE_SIZE);
}
bpf_map_area_free(st_map->uvalue);
- btf_put(st_map->btf);
bpf_map_area_free(st_map);
}
static void bpf_struct_ops_map_free(struct bpf_map *map)
{
+ struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
+
+ module_put(st_map->st_ops_desc->st_ops->owner);
+
/* The struct_ops's function may switch to another struct_ops.
*
* For example, bpf_tcp_cc_x->init() may switch to
@@ -681,6 +684,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
size_t st_map_size;
struct bpf_struct_ops_map *st_map;
const struct btf_type *t, *vt;
+ struct module *mod = NULL;
struct bpf_map *map;
struct btf *btf;
int ret;
@@ -690,10 +694,20 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
if (IS_ERR(btf))
return ERR_PTR(PTR_ERR(btf));
- } else {
+
+ if (btf != btf_vmlinux) {
+ mod = btf_try_get_module(btf);
+ if (!mod) {
+ btf_put(btf);
+ return ERR_PTR(-EINVAL);
+ }
+ }
+ /* mod (NULL for btf_vmlinux) holds a refcnt to btf. We
+ * don't need an extra refcnt here.
+ */
+ btf_put(btf);
+ } else
btf = btf_vmlinux;
- btf_get(btf);
- }
st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
if (!st_ops_desc) {
@@ -756,7 +770,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
errout_free:
__bpf_struct_ops_map_free(map);
errout:
- btf_put(btf);
+ module_put(mod);
return ERR_PTR(ret);
}
@@ -886,6 +900,10 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
if (!bpf_struct_ops_valid_to_reg(new_map))
return -EINVAL;
+ /* The old map is holding the refcount for the owner module. The
+ * ownership of the owner module refcount is going to be
+ * transferred from the old map to the new map.
+ */
if (!st_map->st_ops_desc->st_ops->update)
return -EOPNOTSUPP;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 795c16f9cf57..c303cf2fb5ff 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20079,6 +20079,14 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
}
btf = prog->aux->attach_btf;
+ if (btf != btf_vmlinux) {
+ /* Make sure st_ops is valid through the lifetime of env */
+ env->attach_btf_mod = btf_try_get_module(btf);
+ if (!env->attach_btf_mod) {
+ verbose(env, "owner module of btf is not found\n");
+ return -ENOTSUPP;
+ }
+ }
btf_id = prog->aux->attach_btf_id;
st_ops_desc = bpf_struct_ops_find(btf, btf_id);
@@ -20792,6 +20800,8 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
env->prog->expected_attach_type = 0;
*prog = env->prog;
+
+ module_put(env->attach_btf_mod);
err_unlock:
if (!is_priv)
mutex_unlock(&bpf_verifier_lock);
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 08/14] bpf: hold module for bpf_struct_ops_map.
2023-12-09 0:27 ` [PATCH bpf-next v13 08/14] bpf: hold module for bpf_struct_ops_map thinker.li
@ 2023-12-15 5:54 ` Martin KaFai Lau
2023-12-15 23:25 ` Kui-Feng Lee
0 siblings, 1 reply; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-15 5:54 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> To ensure that a module remains accessible whenever a struct_ops object of
> a struct_ops type provided by the module is still in use.
>
> struct bpf_strct_ops_map doesn't hold a refcnt to btf anymore sicne a
s /bpf_strct_/bpf_struct_/
s/sicne/since/
> module will hold a refcnt to it's btf already. But, struct_ops programs are
> different. They hold their associated btf, not the module since they need
> only btf to assure their types (signatures).
The patch subject is not accurate. The patch holds the module refcnt when
verifying the bpf prog also. May be "hold module refcnt in struct_ops map
creation and prog verification".
The commit message also is inaccurate on the prog load. It did not mention the
module is also held when loading struct_ops prog but it is only held during the
verification time. Please explain why it is only needed during the verification
time.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
> include/linux/bpf.h | 1 +
> include/linux/bpf_verifier.h | 1 +
> kernel/bpf/bpf_struct_ops.c | 28 +++++++++++++++++++++++-----
> kernel/bpf/verifier.c | 10 ++++++++++
> 4 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 91bcd62d6fcf..c5c7cc4552f5 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1681,6 +1681,7 @@ struct bpf_struct_ops {
> void (*unreg)(void *kdata);
> int (*update)(void *kdata, void *old_kdata);
> int (*validate)(void *kdata);
> + struct module *owner;
> const char *name;
> struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
> };
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 314b679fb494..01113bcdd479 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -651,6 +651,7 @@ struct bpf_verifier_env {
> u32 prev_insn_idx;
> struct bpf_prog *prog; /* eBPF program being verified */
> const struct bpf_verifier_ops *ops;
> + struct module *attach_btf_mod; /* The owner module of prog->aux->attach_btf */
> struct bpf_verifier_stack_elem *head; /* stack of verifier states to be processed */
> int stack_size; /* number of states to be processed */
> bool strict_alignment; /* perform strict pointer alignment checks */
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index f943f8378e76..a838f7c7d583 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -641,12 +641,15 @@ static void __bpf_struct_ops_map_free(struct bpf_map *map)
> bpf_jit_uncharge_modmem(PAGE_SIZE);
> }
> bpf_map_area_free(st_map->uvalue);
> - btf_put(st_map->btf);
> bpf_map_area_free(st_map);
> }
>
> static void bpf_struct_ops_map_free(struct bpf_map *map)
> {
> + struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
> +
> + module_put(st_map->st_ops_desc->st_ops->owner);
The module_get was not done on st_ops->owner when st_map->btf is btf_vmlinux
(i.e. not module). Although it probably does not matter, I would feel more
comfortable if it only releases for the things that it did acquire earlier.
/* st_ops->owner was acquired during map_alloc to implicitly holds
* the btf's refcnt. The acquire was only done when btf_is_module()
* st_map->btf cannot be NULL here.
*/
if (btf_is_module(st_map->btf))
module_put(st_map->st_ops_desc->st_ops->owner);
> +
> /* The struct_ops's function may switch to another struct_ops.
> *
> * For example, bpf_tcp_cc_x->init() may switch to
> @@ -681,6 +684,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
> size_t st_map_size;
> struct bpf_struct_ops_map *st_map;
> const struct btf_type *t, *vt;
> + struct module *mod = NULL;
> struct bpf_map *map;
> struct btf *btf;
> int ret;
> @@ -690,10 +694,20 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
> btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
> if (IS_ERR(btf))
> return ERR_PTR(PTR_ERR(btf));
> - } else {
> +
> + if (btf != btf_vmlinux) {
> + mod = btf_try_get_module(btf);
> + if (!mod) {
> + btf_put(btf);
> + return ERR_PTR(-EINVAL);
> + }
> + }
> + /* mod (NULL for btf_vmlinux) holds a refcnt to btf. We
> + * don't need an extra refcnt here.
> + */
> + btf_put(btf);
> + } else
> btf = btf_vmlinux;
> - btf_get(btf);
> - }
>
> st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
> if (!st_ops_desc) {
> @@ -756,7 +770,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
> errout_free:
> __bpf_struct_ops_map_free(map);
> errout:
> - btf_put(btf);
> + module_put(mod);
>
> return ERR_PTR(ret);
> }
> @@ -886,6 +900,10 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
> if (!bpf_struct_ops_valid_to_reg(new_map))
> return -EINVAL;
>
> + /* The old map is holding the refcount for the owner module. The
> + * ownership of the owner module refcount is going to be
> + * transferred from the old map to the new map.
> + */
This part I don't understand. Both old and new map hold its own module's
refcount at map_alloc time and release its own module refcnt during map_free().
Where the module refcount transfer happened?
> if (!st_map->st_ops_desc->st_ops->update)
> return -EOPNOTSUPP;
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 795c16f9cf57..c303cf2fb5ff 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -20079,6 +20079,14 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
> }
>
> btf = prog->aux->attach_btf;
> + if (btf != btf_vmlinux) {
if (btf_is_module(btf)) {
> + /* Make sure st_ops is valid through the lifetime of env */
> + env->attach_btf_mod = btf_try_get_module(btf);
> + if (!env->attach_btf_mod) {
> + verbose(env, "owner module of btf is not found\n");
> + return -ENOTSUPP;
> + }
> + }
>
> btf_id = prog->aux->attach_btf_id;
> st_ops_desc = bpf_struct_ops_find(btf, btf_id);
> @@ -20792,6 +20800,8 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr, __u3
> env->prog->expected_attach_type = 0;
>
> *prog = env->prog;
> +
> + module_put(env->attach_btf_mod);
> err_unlock:
> if (!is_priv)
> mutex_unlock(&bpf_verifier_lock);
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 08/14] bpf: hold module for bpf_struct_ops_map.
2023-12-15 5:54 ` Martin KaFai Lau
@ 2023-12-15 23:25 ` Kui-Feng Lee
0 siblings, 0 replies; 39+ messages in thread
From: Kui-Feng Lee @ 2023-12-15 23:25 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/14/23 21:54, Martin KaFai Lau wrote:
> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> To ensure that a module remains accessible whenever a struct_ops
>> object of
>> a struct_ops type provided by the module is still in use.
>>
>> struct bpf_strct_ops_map doesn't hold a refcnt to btf anymore sicne a
>
> s /bpf_strct_/bpf_struct_/
>
> s/sicne/since/
>
>> module will hold a refcnt to it's btf already. But, struct_ops
>> programs are
>> different. They hold their associated btf, not the module since they need
>> only btf to assure their types (signatures).
>
> The patch subject is not accurate. The patch holds the module refcnt
> when verifying the bpf prog also. May be "hold module refcnt in
> struct_ops map creation and prog verification".
>
> The commit message also is inaccurate on the prog load. It did not
> mention the module is also held when loading struct_ops prog but it is
> only held during the verification time. Please explain why it is only
> needed during the verification time.
>
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>> include/linux/bpf.h | 1 +
>> include/linux/bpf_verifier.h | 1 +
>> kernel/bpf/bpf_struct_ops.c | 28 +++++++++++++++++++++++-----
>> kernel/bpf/verifier.c | 10 ++++++++++
>> 4 files changed, 35 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 91bcd62d6fcf..c5c7cc4552f5 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1681,6 +1681,7 @@ struct bpf_struct_ops {
>> void (*unreg)(void *kdata);
>> int (*update)(void *kdata, void *old_kdata);
>> int (*validate)(void *kdata);
>> + struct module *owner;
>> const char *name;
>> struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
>> };
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index 314b679fb494..01113bcdd479 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -651,6 +651,7 @@ struct bpf_verifier_env {
>> u32 prev_insn_idx;
>> struct bpf_prog *prog; /* eBPF program being verified */
>> const struct bpf_verifier_ops *ops;
>> + struct module *attach_btf_mod; /* The owner module of
>> prog->aux->attach_btf */
>> struct bpf_verifier_stack_elem *head; /* stack of verifier
>> states to be processed */
>> int stack_size; /* number of states to be processed */
>> bool strict_alignment; /* perform strict pointer
>> alignment checks */
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index f943f8378e76..a838f7c7d583 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -641,12 +641,15 @@ static void __bpf_struct_ops_map_free(struct
>> bpf_map *map)
>> bpf_jit_uncharge_modmem(PAGE_SIZE);
>> }
>> bpf_map_area_free(st_map->uvalue);
>> - btf_put(st_map->btf);
>> bpf_map_area_free(st_map);
>> }
>> static void bpf_struct_ops_map_free(struct bpf_map *map)
>> {
>> + struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map
>> *)map;
>> +
>> + module_put(st_map->st_ops_desc->st_ops->owner);
>
> The module_get was not done on st_ops->owner when st_map->btf is
> btf_vmlinux (i.e. not module). Although it probably does not matter, I
> would feel more comfortable if it only releases for the things that it
> did acquire earlier.
>
> /* st_ops->owner was acquired during map_alloc to implicitly holds
> * the btf's refcnt. The acquire was only done when btf_is_module()
> * st_map->btf cannot be NULL here.
> */
> if (btf_is_module(st_map->btf))
> module_put(st_map->st_ops_desc->st_ops->owner);
Sure! I will update it.
>
>> +
>> /* The struct_ops's function may switch to another struct_ops.
>> *
>> * For example, bpf_tcp_cc_x->init() may switch to
>> @@ -681,6 +684,7 @@ static struct bpf_map
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>> size_t st_map_size;
>> struct bpf_struct_ops_map *st_map;
>> const struct btf_type *t, *vt;
>> + struct module *mod = NULL;
>> struct bpf_map *map;
>> struct btf *btf;
>> int ret;
>> @@ -690,10 +694,20 @@ static struct bpf_map
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>> btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>> if (IS_ERR(btf))
>> return ERR_PTR(PTR_ERR(btf));
>> - } else {
>> +
>> + if (btf != btf_vmlinux) {
>> + mod = btf_try_get_module(btf);
>> + if (!mod) {
>> + btf_put(btf);
>> + return ERR_PTR(-EINVAL);
>> + }
>> + }
>> + /* mod (NULL for btf_vmlinux) holds a refcnt to btf. We
>> + * don't need an extra refcnt here.
>> + */
>> + btf_put(btf);
>> + } else
>> btf = btf_vmlinux;
>> - btf_get(btf);
>> - }
>> st_ops_desc = bpf_struct_ops_find_value(btf,
>> attr->btf_vmlinux_value_type_id);
>> if (!st_ops_desc) {
>> @@ -756,7 +770,7 @@ static struct bpf_map
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>> errout_free:
>> __bpf_struct_ops_map_free(map);
>> errout:
>> - btf_put(btf);
>> + module_put(mod);
>> return ERR_PTR(ret);
>> }
>> @@ -886,6 +900,10 @@ static int bpf_struct_ops_map_link_update(struct
>> bpf_link *link, struct bpf_map
>> if (!bpf_struct_ops_valid_to_reg(new_map))
>> return -EINVAL;
>> + /* The old map is holding the refcount for the owner module. The
>> + * ownership of the owner module refcount is going to be
>> + * transferred from the old map to the new map.
>> + */
>
> This part I don't understand. Both old and new map hold its own module's
> refcount at map_alloc time and release its own module refcnt during
> map_free().
> Where the module refcount transfer happened?
Sorry! This comment is not more valid. I will remove it.
>
>> if (!st_map->st_ops_desc->st_ops->update)
>> return -EOPNOTSUPP;
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 795c16f9cf57..c303cf2fb5ff 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -20079,6 +20079,14 @@ static int check_struct_ops_btf_id(struct
>> bpf_verifier_env *env)
>> }
>> btf = prog->aux->attach_btf;
>> + if (btf != btf_vmlinux) {
>
> if (btf_is_module(btf)) {
>
Got it!
>> + /* Make sure st_ops is valid through the lifetime of env */
>> + env->attach_btf_mod = btf_try_get_module(btf);
>> + if (!env->attach_btf_mod) {
>> + verbose(env, "owner module of btf is not found\n");
>> + return -ENOTSUPP;
>> + }
>> + }
>> btf_id = prog->aux->attach_btf_id;
>> st_ops_desc = bpf_struct_ops_find(btf, btf_id);
>> @@ -20792,6 +20800,8 @@ int bpf_check(struct bpf_prog **prog, union
>> bpf_attr *attr, bpfptr_t uattr, __u3
>> env->prog->expected_attach_type = 0;
>> *prog = env->prog;
>> +
>> + module_put(env->attach_btf_mod);
>> err_unlock:
>> if (!is_priv)
>> mutex_unlock(&bpf_verifier_lock);
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH bpf-next v13 09/14] bpf: validate value_type
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
` (7 preceding siblings ...)
2023-12-09 0:27 ` [PATCH bpf-next v13 08/14] bpf: hold module for bpf_struct_ops_map thinker.li
@ 2023-12-09 0:27 ` thinker.li
2023-12-15 6:02 ` Martin KaFai Lau
2023-12-09 0:27 ` [PATCH bpf-next v13 10/14] bpf, net: switch to dynamic registration thinker.li
` (4 subsequent siblings)
13 siblings, 1 reply; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:27 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
A value_type should consist of three components: refcnt, state, and data.
refcnt and state has been move to struct bpf_struct_ops_common_value to
make it easier to check the value type.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/bpf.h | 12 +++++
kernel/bpf/bpf_struct_ops.c | 93 ++++++++++++++++++++++++-------------
2 files changed, 72 insertions(+), 33 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c5c7cc4552f5..7384806ee74e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3321,4 +3321,16 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
return prog->aux->func_idx != 0;
}
+enum bpf_struct_ops_state {
+ BPF_STRUCT_OPS_STATE_INIT,
+ BPF_STRUCT_OPS_STATE_INUSE,
+ BPF_STRUCT_OPS_STATE_TOBEFREE,
+ BPF_STRUCT_OPS_STATE_READY,
+};
+
+struct bpf_struct_ops_common_value {
+ refcount_t refcnt;
+ enum bpf_struct_ops_state state;
+};
+
#endif /* _LINUX_BPF_H */
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index a838f7c7d583..a3d9ffbdba00 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -13,19 +13,8 @@
#include <linux/btf_ids.h>
#include <linux/rcupdate_wait.h>
-enum bpf_struct_ops_state {
- BPF_STRUCT_OPS_STATE_INIT,
- BPF_STRUCT_OPS_STATE_INUSE,
- BPF_STRUCT_OPS_STATE_TOBEFREE,
- BPF_STRUCT_OPS_STATE_READY,
-};
-
-#define BPF_STRUCT_OPS_COMMON_VALUE \
- refcount_t refcnt; \
- enum bpf_struct_ops_state state
-
struct bpf_struct_ops_value {
- BPF_STRUCT_OPS_COMMON_VALUE;
+ struct bpf_struct_ops_common_value common;
char data[] ____cacheline_aligned_in_smp;
};
@@ -80,8 +69,8 @@ static DEFINE_MUTEX(update_mutex);
#define BPF_STRUCT_OPS_TYPE(_name) \
extern struct bpf_struct_ops bpf_##_name; \
\
-struct bpf_struct_ops_##_name { \
- BPF_STRUCT_OPS_COMMON_VALUE; \
+struct bpf_struct_ops_##_name { \
+ struct bpf_struct_ops_common_value common; \
struct _name data ____cacheline_aligned_in_smp; \
};
#include "bpf_struct_ops_types.h"
@@ -112,11 +101,49 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
BTF_ID_LIST(st_ops_ids)
BTF_ID(struct, module)
+BTF_ID(struct, bpf_struct_ops_common_value)
enum {
IDX_MODULE_ID,
+ IDX_ST_OPS_COMMON_VALUE_ID,
};
+extern struct btf *btf_vmlinux;
+
+static bool is_valid_value_type(struct btf *btf, s32 value_id,
+ const struct btf_type *type,
+ const char *value_name)
+{
+ const struct btf_type *common_value_type;
+ const struct btf_member *member;
+ const struct btf_type *vt, *mt;
+
+ vt = btf_type_by_id(btf, value_id);
+ if (btf_vlen(vt) != 2) {
+ pr_warn("The number of %s's members should be 2, but we get %d\n",
+ value_name, btf_vlen(vt));
+ return false;
+ }
+ member = btf_type_member(vt);
+ mt = btf_type_by_id(btf, member->type);
+ common_value_type = btf_type_by_id(btf_vmlinux,
+ st_ops_ids[IDX_ST_OPS_COMMON_VALUE_ID]);
+ if (mt != common_value_type) {
+ pr_warn("The first member of %s should be bpf_struct_ops_common_value\n",
+ value_name);
+ return false;
+ }
+ member++;
+ mt = btf_type_by_id(btf, member->type);
+ if (mt != type) {
+ pr_warn("The second member of %s should be %s\n",
+ value_name, btf_name_by_offset(btf, type->name_off));
+ return false;
+ }
+
+ return true;
+}
+
static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
struct btf *btf,
struct bpf_verifier_log *log)
@@ -137,14 +164,6 @@ static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
}
sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
- value_id = btf_find_by_name_kind(btf, value_name,
- BTF_KIND_STRUCT);
- if (value_id < 0) {
- pr_warn("Cannot find struct %s in %s\n",
- value_name, btf_get_name(btf));
- return;
- }
-
type_id = btf_find_by_name_kind(btf, st_ops->name,
BTF_KIND_STRUCT);
if (type_id < 0) {
@@ -159,6 +178,16 @@ static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
return;
}
+ value_id = btf_find_by_name_kind(btf, value_name,
+ BTF_KIND_STRUCT);
+ if (value_id < 0) {
+ pr_warn("Cannot find struct %s in %s\n",
+ value_name, btf_get_name(btf));
+ return;
+ }
+ if (!is_valid_value_type(btf, value_id, t, value_name))
+ return;
+
for_each_member(i, t, member) {
const struct btf_type *func_proto;
@@ -218,8 +247,6 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
}
}
-extern struct btf *btf_vmlinux;
-
static const struct bpf_struct_ops_desc *
bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
{
@@ -275,7 +302,7 @@ int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
kvalue = &st_map->kvalue;
/* Pair with smp_store_release() during map_update */
- state = smp_load_acquire(&kvalue->state);
+ state = smp_load_acquire(&kvalue->common.state);
if (state == BPF_STRUCT_OPS_STATE_INIT) {
memset(value, 0, map->value_size);
return 0;
@@ -286,7 +313,7 @@ int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
*/
uvalue = value;
memcpy(uvalue, st_map->uvalue, map->value_size);
- uvalue->state = state;
+ uvalue->common.state = state;
/* This value offers the user space a general estimate of how
* many sockets are still utilizing this struct_ops for TCP
@@ -294,7 +321,7 @@ int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
* should sufficiently meet our present goals.
*/
refcnt = atomic64_read(&map->refcnt) - atomic64_read(&map->usercnt);
- refcount_set(&uvalue->refcnt, max_t(s64, refcnt, 0));
+ refcount_set(&uvalue->common.refcnt, max_t(s64, refcnt, 0));
return 0;
}
@@ -414,7 +441,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
if (err)
return err;
- if (uvalue->state || refcount_read(&uvalue->refcnt))
+ if (uvalue->common.state || refcount_read(&uvalue->common.refcnt))
return -EINVAL;
tlinks = kcalloc(BPF_TRAMP_MAX, sizeof(*tlinks), GFP_KERNEL);
@@ -426,7 +453,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
mutex_lock(&st_map->lock);
- if (kvalue->state != BPF_STRUCT_OPS_STATE_INIT) {
+ if (kvalue->common.state != BPF_STRUCT_OPS_STATE_INIT) {
err = -EBUSY;
goto unlock;
}
@@ -540,7 +567,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
*
* Pair with smp_load_acquire() during lookup_elem().
*/
- smp_store_release(&kvalue->state, BPF_STRUCT_OPS_STATE_READY);
+ smp_store_release(&kvalue->common.state, BPF_STRUCT_OPS_STATE_READY);
goto unlock;
}
@@ -558,7 +585,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
* It ensures the above udata updates (e.g. prog->aux->id)
* can be seen once BPF_STRUCT_OPS_STATE_INUSE is set.
*/
- smp_store_release(&kvalue->state, BPF_STRUCT_OPS_STATE_INUSE);
+ smp_store_release(&kvalue->common.state, BPF_STRUCT_OPS_STATE_INUSE);
goto unlock;
}
@@ -588,7 +615,7 @@ static long bpf_struct_ops_map_delete_elem(struct bpf_map *map, void *key)
if (st_map->map.map_flags & BPF_F_LINK)
return -EOPNOTSUPP;
- prev_state = cmpxchg(&st_map->kvalue.state,
+ prev_state = cmpxchg(&st_map->kvalue.common.state,
BPF_STRUCT_OPS_STATE_INUSE,
BPF_STRUCT_OPS_STATE_TOBEFREE);
switch (prev_state) {
@@ -838,7 +865,7 @@ static bool bpf_struct_ops_valid_to_reg(struct bpf_map *map)
return map->map_type == BPF_MAP_TYPE_STRUCT_OPS &&
map->map_flags & BPF_F_LINK &&
/* Pair with smp_store_release() during map_update */
- smp_load_acquire(&st_map->kvalue.state) == BPF_STRUCT_OPS_STATE_READY;
+ smp_load_acquire(&st_map->kvalue.common.state) == BPF_STRUCT_OPS_STATE_READY;
}
static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 09/14] bpf: validate value_type
2023-12-09 0:27 ` [PATCH bpf-next v13 09/14] bpf: validate value_type thinker.li
@ 2023-12-15 6:02 ` Martin KaFai Lau
2023-12-15 23:52 ` Kui-Feng Lee
0 siblings, 1 reply; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-15 6:02 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index c5c7cc4552f5..7384806ee74e 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -3321,4 +3321,16 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
> return prog->aux->func_idx != 0;
> }
>
> +enum bpf_struct_ops_state {
> + BPF_STRUCT_OPS_STATE_INIT,
> + BPF_STRUCT_OPS_STATE_INUSE,
> + BPF_STRUCT_OPS_STATE_TOBEFREE,
> + BPF_STRUCT_OPS_STATE_READY,
> +};
> +
> +struct bpf_struct_ops_common_value {
> + refcount_t refcnt;
> + enum bpf_struct_ops_state state;
> +};
nit. Move these up closer to the existing 'struct bpf_struct_ops' and related
functions. Probably under 'struct bpf_struct_ops'.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 09/14] bpf: validate value_type
2023-12-15 6:02 ` Martin KaFai Lau
@ 2023-12-15 23:52 ` Kui-Feng Lee
0 siblings, 0 replies; 39+ messages in thread
From: Kui-Feng Lee @ 2023-12-15 23:52 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/14/23 22:02, Martin KaFai Lau wrote:
> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index c5c7cc4552f5..7384806ee74e 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -3321,4 +3321,16 @@ static inline bool bpf_is_subprog(const struct
>> bpf_prog *prog)
>> return prog->aux->func_idx != 0;
>> }
>> +enum bpf_struct_ops_state {
>> + BPF_STRUCT_OPS_STATE_INIT,
>> + BPF_STRUCT_OPS_STATE_INUSE,
>> + BPF_STRUCT_OPS_STATE_TOBEFREE,
>> + BPF_STRUCT_OPS_STATE_READY,
>> +};
>> +
>> +struct bpf_struct_ops_common_value {
>> + refcount_t refcnt;
>> + enum bpf_struct_ops_state state;
>> +};
>
> nit. Move these up closer to the existing 'struct bpf_struct_ops' and
> related functions. Probably under 'struct bpf_struct_ops'.
>
Sure! I will move all following changes to the place closer to
struct bpf_struct_ops.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH bpf-next v13 10/14] bpf, net: switch to dynamic registration
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
` (8 preceding siblings ...)
2023-12-09 0:27 ` [PATCH bpf-next v13 09/14] bpf: validate value_type thinker.li
@ 2023-12-09 0:27 ` thinker.li
2023-12-15 6:51 ` Martin KaFai Lau
2023-12-09 0:27 ` [PATCH bpf-next v13 11/14] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
` (3 subsequent siblings)
13 siblings, 1 reply; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:27 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee, netdev
From: Kui-Feng Lee <thinker.li@gmail.com>
Replace the static list of struct_ops types with per-btf struct_ops_tab to
enable dynamic registration.
Both bpf_dummy_ops and bpf_tcp_ca now utilize the registration function
instead of being listed in bpf_struct_ops_types.h.
Cc: netdev@vger.kernel.org
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/bpf.h | 28 ++++++++--
include/linux/btf.h | 2 +
kernel/bpf/bpf_struct_ops.c | 90 ++++++++++---------------------
kernel/bpf/bpf_struct_ops_types.h | 12 -----
kernel/bpf/btf.c | 49 +++++++++++++++--
net/bpf/bpf_dummy_struct_ops.c | 13 ++++-
net/ipv4/bpf_tcp_ca.c | 14 +++--
7 files changed, 119 insertions(+), 89 deletions(-)
delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7384806ee74e..c881befa35f5 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1698,7 +1698,6 @@ struct bpf_struct_ops_desc {
#if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
#define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *btf, u32 type_id);
-void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log);
bool bpf_struct_ops_get(const void *kdata);
void bpf_struct_ops_put(const void *kdata);
int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
@@ -1744,10 +1743,6 @@ static inline const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *
{
return NULL;
}
-static inline void bpf_struct_ops_init(struct btf *btf,
- struct bpf_verifier_log *log)
-{
-}
static inline bool bpf_try_module_get(const void *data, struct module *owner)
{
return try_module_get(owner);
@@ -3321,6 +3316,14 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
return prog->aux->func_idx != 0;
}
+int register_bpf_struct_ops(struct bpf_struct_ops *st_ops);
+
+#define REGISTER_BPF_STRUCT_OPS(st_ops, type) \
+({ \
+ BTF_STRUCT_OPS_TYPE_EMIT(type); \
+ register_bpf_struct_ops(st_ops); \
+})
+
enum bpf_struct_ops_state {
BPF_STRUCT_OPS_STATE_INIT,
BPF_STRUCT_OPS_STATE_INUSE,
@@ -3333,4 +3336,19 @@ struct bpf_struct_ops_common_value {
enum bpf_struct_ops_state state;
};
+/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
+ * the map's value exposed to the userspace and its btf-type-id is
+ * stored at the map->btf_vmlinux_value_type_id.
+ *
+ */
+#define DEFINE_STRUCT_OPS_VALUE_TYPE(_name) \
+struct bpf_struct_ops_##_name { \
+ struct bpf_struct_ops_common_value common; \
+ struct _name data ____cacheline_aligned_in_smp; \
+}
+
+int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
+ struct btf *btf,
+ struct bpf_verifier_log *log);
+
#endif /* _LINUX_BPF_H */
diff --git a/include/linux/btf.h b/include/linux/btf.h
index e2f4b85cf82a..cabab3db5216 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -12,6 +12,8 @@
#include <uapi/linux/bpf.h>
#define BTF_TYPE_EMIT(type) ((void)(type *)0)
+#define BTF_STRUCT_OPS_TYPE_EMIT(type) \
+ ((void)(struct bpf_struct_ops_##type *)0)
#define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
/* These need to be macros, as the expressions are used in assembler input */
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index a3d9ffbdba00..fd26716fa0f9 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -61,35 +61,6 @@ static DEFINE_MUTEX(update_mutex);
#define VALUE_PREFIX "bpf_struct_ops_"
#define VALUE_PREFIX_LEN (sizeof(VALUE_PREFIX) - 1)
-/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
- * the map's value exposed to the userspace and its btf-type-id is
- * stored at the map->btf_vmlinux_value_type_id.
- *
- */
-#define BPF_STRUCT_OPS_TYPE(_name) \
-extern struct bpf_struct_ops bpf_##_name; \
- \
-struct bpf_struct_ops_##_name { \
- struct bpf_struct_ops_common_value common; \
- struct _name data ____cacheline_aligned_in_smp; \
-};
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-
-enum {
-#define BPF_STRUCT_OPS_TYPE(_name) BPF_STRUCT_OPS_TYPE_##_name,
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
- __NR_BPF_STRUCT_OPS_TYPE,
-};
-
-static struct bpf_struct_ops_desc bpf_struct_ops[] = {
-#define BPF_STRUCT_OPS_TYPE(_name) \
- [BPF_STRUCT_OPS_TYPE_##_name] = { .st_ops = &bpf_##_name },
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-};
-
const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
};
@@ -144,9 +115,9 @@ static bool is_valid_value_type(struct btf *btf, s32 value_id,
return true;
}
-static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
- struct btf *btf,
- struct bpf_verifier_log *log)
+int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
+ struct btf *btf,
+ struct bpf_verifier_log *log)
{
struct bpf_struct_ops *st_ops = st_ops_desc->st_ops;
const struct btf_member *member;
@@ -160,7 +131,7 @@ static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
sizeof(value_name)) {
pr_warn("struct_ops name %s is too long\n",
st_ops->name);
- return;
+ return -EINVAL;
}
sprintf(value_name, "%s%s", VALUE_PREFIX, st_ops->name);
@@ -169,13 +140,13 @@ static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
if (type_id < 0) {
pr_warn("Cannot find struct %s in %s\n",
st_ops->name, btf_get_name(btf));
- return;
+ return -EINVAL;
}
t = btf_type_by_id(btf, type_id);
if (btf_type_vlen(t) > BPF_STRUCT_OPS_MAX_NR_MEMBERS) {
pr_warn("Cannot support #%u members in struct %s\n",
btf_type_vlen(t), st_ops->name);
- return;
+ return -EINVAL;
}
value_id = btf_find_by_name_kind(btf, value_name,
@@ -183,10 +154,10 @@ static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
if (value_id < 0) {
pr_warn("Cannot find struct %s in %s\n",
value_name, btf_get_name(btf));
- return;
+ return -EINVAL;
}
if (!is_valid_value_type(btf, value_id, t, value_name))
- return;
+ return -EINVAL;
for_each_member(i, t, member) {
const struct btf_type *func_proto;
@@ -195,13 +166,13 @@ static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
if (!*mname) {
pr_warn("anon member in struct %s is not supported\n",
st_ops->name);
- break;
+ return -EOPNOTSUPP;
}
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;
+ return -EOPNOTSUPP;
}
func_proto = btf_type_resolve_func_ptr(btf,
@@ -213,7 +184,7 @@ static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
&st_ops->func_models[i])) {
pr_warn("Error in parsing func ptr %s in struct %s\n",
mname, st_ops->name);
- break;
+ return -EINVAL;
}
}
@@ -221,6 +192,7 @@ static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
if (st_ops->init(btf)) {
pr_warn("Error in init bpf_struct_ops %s\n",
st_ops->name);
+ return -EINVAL;
} else {
st_ops_desc->type_id = type_id;
st_ops_desc->type = t;
@@ -229,35 +201,24 @@ static void bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
value_id);
}
}
-}
-void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
-{
- struct bpf_struct_ops_desc *st_ops_desc;
- u32 i;
-
- /* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
-#define BPF_STRUCT_OPS_TYPE(_name) BTF_TYPE_EMIT(struct bpf_struct_ops_##_name);
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-
- for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
- st_ops_desc = &bpf_struct_ops[i];
- bpf_struct_ops_desc_init(st_ops_desc, btf, log);
- }
+ return 0;
}
static const struct bpf_struct_ops_desc *
bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
{
+ const struct bpf_struct_ops_desc *st_ops_list;
unsigned int i;
+ u32 cnt = 0;
- if (!value_id || !btf)
+ if (!value_id)
return NULL;
- for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
- if (bpf_struct_ops[i].value_id == value_id)
- return &bpf_struct_ops[i];
+ st_ops_list = btf_get_struct_ops(btf, &cnt);
+ for (i = 0; i < cnt; i++) {
+ if (st_ops_list[i].value_id == value_id)
+ return &st_ops_list[i];
}
return NULL;
@@ -266,14 +227,17 @@ bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
const struct bpf_struct_ops_desc *
bpf_struct_ops_find(struct btf *btf, u32 type_id)
{
+ const struct bpf_struct_ops_desc *st_ops_list;
unsigned int i;
+ u32 cnt;
- if (!type_id || !btf)
+ if (!type_id)
return NULL;
- for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
- if (bpf_struct_ops[i].type_id == type_id)
- return &bpf_struct_ops[i];
+ st_ops_list = btf_get_struct_ops(btf, &cnt);
+ for (i = 0; i < cnt; i++) {
+ if (st_ops_list[i].type_id == type_id)
+ return &st_ops_list[i];
}
return NULL;
diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h
deleted file mode 100644
index 5678a9ddf817..000000000000
--- a/kernel/bpf/bpf_struct_ops_types.h
+++ /dev/null
@@ -1,12 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/* internal file - do not include directly */
-
-#ifdef CONFIG_BPF_JIT
-#ifdef CONFIG_NET
-BPF_STRUCT_OPS_TYPE(bpf_dummy_ops)
-#endif
-#ifdef CONFIG_INET
-#include <net/tcp.h>
-BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
-#endif
-#endif
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index edbe3cbf2dcc..5545dee3ff54 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -19,6 +19,7 @@
#include <linux/bpf_verifier.h>
#include <linux/btf.h>
#include <linux/btf_ids.h>
+#include <linux/bpf.h>
#include <linux/bpf_lsm.h>
#include <linux/skmsg.h>
#include <linux/perf_event.h>
@@ -5792,8 +5793,6 @@ struct btf *btf_parse_vmlinux(void)
/* btf_parse_vmlinux() runs under bpf_verifier_lock */
bpf_ctx_convert.t = btf_type_by_id(btf, bpf_ctx_convert_btf_id[0]);
- bpf_struct_ops_init(btf, log);
-
refcount_set(&btf->refcnt, 1);
err = btf_alloc_id(btf);
@@ -8621,11 +8620,21 @@ bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log,
return !strncmp(reg_name, arg_name, cmp_len);
}
+#ifndef CONFIG_BPF_JIT
+int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
+ struct btf *btf,
+ struct bpf_verifier_log *log)
+{
+ return -ENOTSUPP;
+}
+#endif /* CONFIG_BPF_JIT */
+
static int
-btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops)
+btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops,
+ struct bpf_verifier_log *log)
{
struct btf_struct_ops_tab *tab, *new_tab;
- int i;
+ int i, err;
if (!btf)
return -ENOENT;
@@ -8662,6 +8671,10 @@ btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops)
tab->ops[btf->struct_ops_tab->cnt].st_ops = st_ops;
+ err = bpf_struct_ops_desc_init(&tab->ops[btf->struct_ops_tab->cnt], btf, log);
+ if (err)
+ return err;
+
btf->struct_ops_tab->cnt++;
return 0;
@@ -8677,3 +8690,31 @@ const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf *btf, u32 *ret_c
*ret_cnt = btf->struct_ops_tab->cnt;
return (const struct bpf_struct_ops_desc *)btf->struct_ops_tab->ops;
}
+
+int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)
+{
+ struct bpf_verifier_log *log;
+ struct btf *btf;
+ int err = 0;
+
+ btf = btf_get_module_btf(st_ops->owner);
+ if (!btf)
+ return -EINVAL;
+
+ log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
+ if (!log) {
+ err = -ENOMEM;
+ goto errout;
+ }
+
+ log->level = BPF_LOG_KERNEL;
+
+ err = btf_add_struct_ops(btf, st_ops, log);
+
+errout:
+ kfree(log);
+ btf_put(btf);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index bd753dbccaf6..2bb2625ffbbf 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -7,7 +7,7 @@
#include <linux/bpf.h>
#include <linux/btf.h>
-extern struct bpf_struct_ops bpf_bpf_dummy_ops;
+static struct bpf_struct_ops bpf_bpf_dummy_ops;
/* A common type for test_N with return value in bpf_dummy_ops */
typedef int (*dummy_ops_test_ret_fn)(struct bpf_dummy_ops_state *state, ...);
@@ -222,11 +222,13 @@ static int bpf_dummy_reg(void *kdata)
return -EOPNOTSUPP;
}
+DEFINE_STRUCT_OPS_VALUE_TYPE(bpf_dummy_ops);
+
static void bpf_dummy_unreg(void *kdata)
{
}
-struct bpf_struct_ops bpf_bpf_dummy_ops = {
+static struct bpf_struct_ops bpf_bpf_dummy_ops = {
.verifier_ops = &bpf_dummy_verifier_ops,
.init = bpf_dummy_init,
.check_member = bpf_dummy_ops_check_member,
@@ -234,4 +236,11 @@ struct bpf_struct_ops bpf_bpf_dummy_ops = {
.reg = bpf_dummy_reg,
.unreg = bpf_dummy_unreg,
.name = "bpf_dummy_ops",
+ .owner = THIS_MODULE,
};
+
+static int __init bpf_dummy_struct_ops_init(void)
+{
+ return REGISTER_BPF_STRUCT_OPS(&bpf_bpf_dummy_ops, bpf_dummy_ops);
+}
+late_initcall(bpf_dummy_struct_ops_init);
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index 5bb56c9ad4e5..6d2728dc600e 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -12,7 +12,7 @@
#include <net/bpf_sk_storage.h>
/* "extern" is to avoid sparse warning. It is only used in bpf_struct_ops.c. */
-extern struct bpf_struct_ops bpf_tcp_congestion_ops;
+static struct bpf_struct_ops bpf_tcp_congestion_ops;
static u32 unsupported_ops[] = {
offsetof(struct tcp_congestion_ops, get_info),
@@ -277,7 +277,9 @@ static int bpf_tcp_ca_validate(void *kdata)
return tcp_validate_congestion_control(kdata);
}
-struct bpf_struct_ops bpf_tcp_congestion_ops = {
+DEFINE_STRUCT_OPS_VALUE_TYPE(tcp_congestion_ops);
+
+static struct bpf_struct_ops bpf_tcp_congestion_ops = {
.verifier_ops = &bpf_tcp_ca_verifier_ops,
.reg = bpf_tcp_ca_reg,
.unreg = bpf_tcp_ca_unreg,
@@ -287,10 +289,16 @@ struct bpf_struct_ops bpf_tcp_congestion_ops = {
.init = bpf_tcp_ca_init,
.validate = bpf_tcp_ca_validate,
.name = "tcp_congestion_ops",
+ .owner = THIS_MODULE,
};
static int __init bpf_tcp_ca_kfunc_init(void)
{
- return register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_tcp_ca_kfunc_set);
+ int ret;
+
+ ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_tcp_ca_kfunc_set);
+ ret = ret ?: REGISTER_BPF_STRUCT_OPS(&bpf_tcp_congestion_ops, tcp_congestion_ops);
+
+ return ret;
}
late_initcall(bpf_tcp_ca_kfunc_init);
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 10/14] bpf, net: switch to dynamic registration
2023-12-09 0:27 ` [PATCH bpf-next v13 10/14] bpf, net: switch to dynamic registration thinker.li
@ 2023-12-15 6:51 ` Martin KaFai Lau
0 siblings, 0 replies; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-15 6:51 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, netdev, bpf, ast, song, kernel-team, andrii,
drosen
On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 7384806ee74e..c881befa35f5 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1698,7 +1698,6 @@ struct bpf_struct_ops_desc {
> #if defined(CONFIG_BPF_JIT) && defined(CONFIG_BPF_SYSCALL)
> #define BPF_MODULE_OWNER ((void *)((0xeB9FUL << 2) + POISON_POINTER_DELTA))
> const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *btf, u32 type_id);
> -void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log);
> bool bpf_struct_ops_get(const void *kdata);
> void bpf_struct_ops_put(const void *kdata);
> int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map, void *key,
> @@ -1744,10 +1743,6 @@ static inline const struct bpf_struct_ops_desc *bpf_struct_ops_find(struct btf *
> {
> return NULL;
> }
> -static inline void bpf_struct_ops_init(struct btf *btf,
> - struct bpf_verifier_log *log)
> -{
> -}
> static inline bool bpf_try_module_get(const void *data, struct module *owner)
> {
> return try_module_get(owner);
> @@ -3321,6 +3316,14 @@ static inline bool bpf_is_subprog(const struct bpf_prog *prog)
> return prog->aux->func_idx != 0;
> }
>
> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops);
This probably should be in btf.h like register_btf_kfunc_id_set().
and should have an empty implementation when CONFIG_BPF_SYSCALL is not set.
> +
> +#define REGISTER_BPF_STRUCT_OPS(st_ops, type) \
Add comment here to suggest the module writer to use REGISTER_BPF_STRUCT_OPS
instead of register_bpf_struct_ops().
> +({ \
> + BTF_STRUCT_OPS_TYPE_EMIT(type); \
Directly use BTF_TYPE_EMIT(struct bpf_struct_ops_##type) here.
Also, is it possible to do the DEFINE_STRUCT_OPS_VALUE_TYPE() type here such
that the module writer does not need to. It will be nice if
REGISTER_BPF_STRUCT_OPS does the define value type and emit value type together.
> + register_bpf_struct_ops(st_ops); \
> +})
> +
> enum bpf_struct_ops_state {
> BPF_STRUCT_OPS_STATE_INIT,
> BPF_STRUCT_OPS_STATE_INUSE,
> @@ -3333,4 +3336,19 @@ struct bpf_struct_ops_common_value {
> enum bpf_struct_ops_state state;
> };
>
> +/* bpf_struct_ops_##_name (e.g. bpf_struct_ops_tcp_congestion_ops) is
> + * the map's value exposed to the userspace and its btf-type-id is
> + * stored at the map->btf_vmlinux_value_type_id.
> + *
> + */
> +#define DEFINE_STRUCT_OPS_VALUE_TYPE(_name) \
> +struct bpf_struct_ops_##_name { \
> + struct bpf_struct_ops_common_value common; \
> + struct _name data ____cacheline_aligned_in_smp; \
> +}
> +
> +int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> + struct btf *btf,
> + struct bpf_verifier_log *log);
> +
nit. same as the comment in previous patch 8. Move these up closer to other
struct_ops structs and functions. bpf_struct_ops_desc_init is implemented in
bpf_struct_ops.c and it should be in the same CONFIG_BPF_JIT guard earlier in
this file. Also, where is the empty bpf_struct_ops_desc_init() when
CONFIG_BPF_JIT is not set?
> #endif /* _LINUX_BPF_H */
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index e2f4b85cf82a..cabab3db5216 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -12,6 +12,8 @@
> #include <uapi/linux/bpf.h>
>
> #define BTF_TYPE_EMIT(type) ((void)(type *)0)
> +#define BTF_STRUCT_OPS_TYPE_EMIT(type) \
> + ((void)(struct bpf_struct_ops_##type *)0)
Remove this new macro. It is almost the same as BTF_TYPE_EMIT and it is only
used once in REGISTER_BPF_STRUCT_OPS. module writer will use
REGISTER_BPF_STRUCT_OPS and no need to figure out what type needs to be emitted.
> #define BTF_TYPE_EMIT_ENUM(enum_val) ((void)enum_val)
>
> /* These need to be macros, as the expressions are used in assembler input */
[ ... ]
> static const struct bpf_struct_ops_desc *
> bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
> {
> + const struct bpf_struct_ops_desc *st_ops_list;
> unsigned int i;
> + u32 cnt = 0;
>
> - if (!value_id || !btf)
> + if (!value_id)
> return NULL;
>
> - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
> - if (bpf_struct_ops[i].value_id == value_id)
> - return &bpf_struct_ops[i];
> + st_ops_list = btf_get_struct_ops(btf, &cnt);
> + for (i = 0; i < cnt; i++) {
> + if (st_ops_list[i].value_id == value_id)
> + return &st_ops_list[i];
> }
>
> return NULL;
> @@ -266,14 +227,17 @@ bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
> const struct bpf_struct_ops_desc *
> bpf_struct_ops_find(struct btf *btf, u32 type_id)
> {
> + const struct bpf_struct_ops_desc *st_ops_list;
> unsigned int i;
> + u32 cnt;
cnt is not initialized here. The above bpf_struct_ops_find_value() did init cnt
to 0 though.
>
> - if (!type_id || !btf)
> + if (!type_id)
> return NULL;
>
> - for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
> - if (bpf_struct_ops[i].type_id == type_id)
> - return &bpf_struct_ops[i];
> + st_ops_list = btf_get_struct_ops(btf, &cnt);
> + for (i = 0; i < cnt; i++) {
If st_ops_list is NULL, cnt could be anything here. Lets fix patch 4 to set cnt
to 0 when btf->struct_ops_tab is empty.
> + if (st_ops_list[i].type_id == type_id)
> + return &st_ops_list[i];
> }
>
> return NULL;
> diff --git a/kernel/bpf/bpf_struct_ops_types.h b/kernel/bpf/bpf_struct_ops_types.h
> deleted file mode 100644
> index 5678a9ddf817..000000000000
> --- a/kernel/bpf/bpf_struct_ops_types.h
> +++ /dev/null
> @@ -1,12 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -/* internal file - do not include directly */
> -
> -#ifdef CONFIG_BPF_JIT
> -#ifdef CONFIG_NET
> -BPF_STRUCT_OPS_TYPE(bpf_dummy_ops)
> -#endif
> -#ifdef CONFIG_INET
> -#include <net/tcp.h>
> -BPF_STRUCT_OPS_TYPE(tcp_congestion_ops)
> -#endif
> -#endif
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index edbe3cbf2dcc..5545dee3ff54 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -19,6 +19,7 @@
> #include <linux/bpf_verifier.h>
> #include <linux/btf.h>
> #include <linux/btf_ids.h>
> +#include <linux/bpf.h>
> #include <linux/bpf_lsm.h>
> #include <linux/skmsg.h>
> #include <linux/perf_event.h>
> @@ -5792,8 +5793,6 @@ struct btf *btf_parse_vmlinux(void)
> /* btf_parse_vmlinux() runs under bpf_verifier_lock */
> bpf_ctx_convert.t = btf_type_by_id(btf, bpf_ctx_convert_btf_id[0]);
>
> - bpf_struct_ops_init(btf, log);
> -
> refcount_set(&btf->refcnt, 1);
>
> err = btf_alloc_id(btf);
> @@ -8621,11 +8620,21 @@ bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log,
> return !strncmp(reg_name, arg_name, cmp_len);
> }
>
> +#ifndef CONFIG_BPF_JIT
> +int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> + struct btf *btf,
> + struct bpf_verifier_log *log)
> +{
> + return -ENOTSUPP;
> +}
> +#endif /* CONFIG_BPF_JIT */
ah. It is here. This should be in bpf.h.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH bpf-next v13 11/14] libbpf: Find correct module BTFs for struct_ops maps and progs.
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
` (9 preceding siblings ...)
2023-12-09 0:27 ` [PATCH bpf-next v13 10/14] bpf, net: switch to dynamic registration thinker.li
@ 2023-12-09 0:27 ` thinker.li
2023-12-09 0:27 ` [PATCH bpf-next v13 12/14] bpf: export btf_ctx_access to modules thinker.li
` (2 subsequent siblings)
13 siblings, 0 replies; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:27 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Locate the module BTFs for struct_ops maps and progs and pass them to the
kernel. This ensures that the kernel correctly resolves type IDs from the
appropriate module BTFs.
For the map of a struct_ops object, the FD of the module BTF is set to
bpf_map to keep a reference to the module BTF. The FD is passed to the
kernel as value_type_btf_obj_fd when the struct_ops object is loaded.
For a bpf_struct_ops prog, attach_btf_obj_fd of bpf_prog is the FD of a
module BTF in the kernel.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/lib/bpf/bpf.c | 4 +++-
tools/lib/bpf/bpf.h | 5 +++--
tools/lib/bpf/libbpf.c | 38 ++++++++++++++++++++++++++++----------
3 files changed, 34 insertions(+), 13 deletions(-)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index f4e1da3c6d5f..14ed20b73b86 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -169,7 +169,8 @@ int bpf_map_create(enum bpf_map_type map_type,
__u32 max_entries,
const struct bpf_map_create_opts *opts)
{
- const size_t attr_sz = offsetofend(union bpf_attr, map_token_fd);
+ const size_t attr_sz = offsetofend(union bpf_attr,
+ value_type_btf_obj_fd);
union bpf_attr attr;
int fd;
@@ -191,6 +192,7 @@ int bpf_map_create(enum bpf_map_type map_type,
attr.btf_key_type_id = OPTS_GET(opts, btf_key_type_id, 0);
attr.btf_value_type_id = OPTS_GET(opts, btf_value_type_id, 0);
attr.btf_vmlinux_value_type_id = OPTS_GET(opts, btf_vmlinux_value_type_id, 0);
+ attr.value_type_btf_obj_fd = OPTS_GET(opts, value_type_btf_obj_fd, 0);
attr.inner_map_fd = OPTS_GET(opts, inner_map_fd, 0);
attr.map_flags = OPTS_GET(opts, map_flags, 0);
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 991b86bfe7e4..1cecfde0c732 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -53,9 +53,10 @@ struct bpf_map_create_opts {
__u32 map_ifindex;
__u32 token_fd;
- size_t :0;
+ __u32 value_type_btf_obj_fd;
+ size_t:0;
};
-#define bpf_map_create_opts__last_field token_fd
+#define bpf_map_create_opts__last_field value_type_btf_obj_fd
LIBBPF_API int bpf_map_create(enum bpf_map_type map_type,
const char *map_name,
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index ac54ebc0629f..d6709ebd1d8a 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -527,6 +527,7 @@ struct bpf_map {
struct bpf_map_def def;
__u32 numa_node;
__u32 btf_var_idx;
+ int mod_btf_fd;
__u32 btf_key_type_id;
__u32 btf_value_type_id;
__u32 btf_vmlinux_value_type_id;
@@ -930,22 +931,29 @@ find_member_by_name(const struct btf *btf, const struct btf_type *t,
return NULL;
}
+static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
+ __u16 kind, struct btf **res_btf,
+ struct module_btf **res_mod_btf);
+
#define STRUCT_OPS_VALUE_PREFIX "bpf_struct_ops_"
static int find_btf_by_prefix_kind(const struct btf *btf, const char *prefix,
const char *name, __u32 kind);
static int
-find_struct_ops_kern_types(const struct btf *btf, const char *tname,
+find_struct_ops_kern_types(struct bpf_object *obj, const char *tname,
+ struct module_btf **mod_btf,
const struct btf_type **type, __u32 *type_id,
const struct btf_type **vtype, __u32 *vtype_id,
const struct btf_member **data_member)
{
const struct btf_type *kern_type, *kern_vtype;
const struct btf_member *kern_data_member;
+ struct btf *btf;
__s32 kern_vtype_id, kern_type_id;
__u32 i;
- kern_type_id = btf__find_by_name_kind(btf, tname, BTF_KIND_STRUCT);
+ kern_type_id = find_ksym_btf_id(obj, tname, BTF_KIND_STRUCT,
+ &btf, mod_btf);
if (kern_type_id < 0) {
pr_warn("struct_ops init_kern: struct %s is not found in kernel BTF\n",
tname);
@@ -999,14 +1007,16 @@ static bool bpf_map__is_struct_ops(const struct bpf_map *map)
}
/* Init the map's fields that depend on kern_btf */
-static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
- const struct btf *btf,
- const struct btf *kern_btf)
+static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
{
const struct btf_member *member, *kern_member, *kern_data_member;
const struct btf_type *type, *kern_type, *kern_vtype;
__u32 i, kern_type_id, kern_vtype_id, kern_data_off;
+ struct bpf_object *obj = map->obj;
+ const struct btf *btf = obj->btf;
struct bpf_struct_ops *st_ops;
+ const struct btf *kern_btf;
+ struct module_btf *mod_btf;
void *data, *kern_data;
const char *tname;
int err;
@@ -1014,16 +1024,19 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
st_ops = map->st_ops;
type = st_ops->type;
tname = st_ops->tname;
- err = find_struct_ops_kern_types(kern_btf, tname,
+ err = find_struct_ops_kern_types(obj, tname, &mod_btf,
&kern_type, &kern_type_id,
&kern_vtype, &kern_vtype_id,
&kern_data_member);
if (err)
return err;
+ kern_btf = mod_btf ? mod_btf->btf : obj->btf_vmlinux;
+
pr_debug("struct_ops init_kern %s: type_id:%u kern_type_id:%u kern_vtype_id:%u\n",
map->name, st_ops->type_id, kern_type_id, kern_vtype_id);
+ map->mod_btf_fd = mod_btf ? mod_btf->fd : 0;
map->def.value_size = kern_vtype->size;
map->btf_vmlinux_value_type_id = kern_vtype_id;
@@ -1099,6 +1112,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map,
return -ENOTSUP;
}
+ if (mod_btf)
+ prog->attach_btf_obj_fd = mod_btf->fd;
prog->attach_btf_id = kern_type_id;
prog->expected_attach_type = kern_member_idx;
@@ -1141,8 +1156,7 @@ static int bpf_object__init_kern_struct_ops_maps(struct bpf_object *obj)
if (!bpf_map__is_struct_ops(map))
continue;
- err = bpf_map__init_kern_struct_ops(map, obj->btf,
- obj->btf_vmlinux);
+ err = bpf_map__init_kern_struct_ops(map);
if (err)
return err;
}
@@ -5212,8 +5226,10 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map, b
create_attr.numa_node = map->numa_node;
create_attr.map_extra = map->map_extra;
- if (bpf_map__is_struct_ops(map))
+ if (bpf_map__is_struct_ops(map)) {
create_attr.btf_vmlinux_value_type_id = map->btf_vmlinux_value_type_id;
+ create_attr.value_type_btf_obj_fd = map->mod_btf_fd;
+ }
if (obj->btf && btf__fd(obj->btf) >= 0) {
create_attr.btf_fd = btf__fd(obj->btf);
@@ -9557,7 +9573,9 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog, const char *attac
*btf_obj_fd = 0;
*btf_type_id = 1;
} else {
- err = find_kernel_btf_id(prog->obj, attach_name, attach_type, btf_obj_fd, btf_type_id);
+ err = find_kernel_btf_id(prog->obj, attach_name,
+ attach_type, btf_obj_fd,
+ btf_type_id);
}
if (err) {
pr_warn("prog '%s': failed to find kernel BTF type ID of '%s': %d\n",
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH bpf-next v13 12/14] bpf: export btf_ctx_access to modules.
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
` (10 preceding siblings ...)
2023-12-09 0:27 ` [PATCH bpf-next v13 11/14] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
@ 2023-12-09 0:27 ` thinker.li
2023-12-09 0:27 ` [PATCH bpf-next v13 13/14] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
2023-12-09 0:27 ` [PATCH bpf-next v13 14/14] bpf: pass btf object id in bpf_map_info thinker.li
13 siblings, 0 replies; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:27 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
The module requires the use of btf_ctx_access() to invoke
bpf_tracing_btf_ctx_access() from a module. This function is valuable for
implementing validation functions that ensure proper access to ctx.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
kernel/bpf/btf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 5545dee3ff54..d9cdf41e8f34 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6142,6 +6142,7 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
__btf_name_by_offset(btf, t->name_off));
return true;
}
+EXPORT_SYMBOL_GPL(btf_ctx_access);
enum bpf_struct_walk_result {
/* < 0 error */
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH bpf-next v13 13/14] selftests/bpf: test case for register_bpf_struct_ops().
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
` (11 preceding siblings ...)
2023-12-09 0:27 ` [PATCH bpf-next v13 12/14] bpf: export btf_ctx_access to modules thinker.li
@ 2023-12-09 0:27 ` thinker.li
2023-12-15 7:17 ` Martin KaFai Lau
2023-12-09 0:27 ` [PATCH bpf-next v13 14/14] bpf: pass btf object id in bpf_map_info thinker.li
13 siblings, 1 reply; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:27 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Create a new struct_ops type called bpf_testmod_ops within the bpf_testmod
module. When a struct_ops object is registered, the bpf_testmod module will
invoke test_2 from the module.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 52 +++++++++++
.../selftests/bpf/bpf_testmod/bpf_testmod.h | 5 +
.../bpf/prog_tests/test_struct_ops_module.c | 92 +++++++++++++++++++
.../selftests/bpf/progs/struct_ops_module.c | 30 ++++++
.../selftests/bpf/progs/testmod_unload.c | 25 +++++
5 files changed, 204 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_module.c
create mode 100644 tools/testing/selftests/bpf/progs/testmod_unload.c
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 91907b321f91..804a67bbb479 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2020 Facebook */
+#include <linux/bpf.h>
#include <linux/btf.h>
#include <linux/btf_ids.h>
#include <linux/error-injection.h>
@@ -520,11 +521,61 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_static_unused_arg)
BTF_ID_FLAGS(func, bpf_kfunc_call_test_offset)
BTF_SET8_END(bpf_testmod_check_kfunc_ids)
+DEFINE_STRUCT_OPS_VALUE_TYPE(bpf_testmod_ops);
+
+static int bpf_testmod_ops_init(struct btf *btf)
+{
+ return 0;
+}
+
+static bool bpf_testmod_ops_is_valid_access(int off, int size,
+ enum bpf_access_type type,
+ const struct bpf_prog *prog,
+ struct bpf_insn_access_aux *info)
+{
+ return bpf_tracing_btf_ctx_access(off, size, type, prog, info);
+}
+
+static int bpf_testmod_ops_init_member(const struct btf_type *t,
+ const struct btf_member *member,
+ void *kdata, const void *udata)
+{
+ return 0;
+}
+
static const struct btf_kfunc_id_set bpf_testmod_kfunc_set = {
.owner = THIS_MODULE,
.set = &bpf_testmod_check_kfunc_ids,
};
+static const struct bpf_verifier_ops bpf_testmod_verifier_ops = {
+ .is_valid_access = bpf_testmod_ops_is_valid_access,
+};
+
+static int bpf_dummy_reg(void *kdata)
+{
+ struct bpf_testmod_ops *ops = kdata;
+ int r;
+
+ r = ops->test_2(4, 3);
+
+ return 0;
+}
+
+static void bpf_dummy_unreg(void *kdata)
+{
+}
+
+struct bpf_struct_ops bpf_bpf_testmod_ops = {
+ .verifier_ops = &bpf_testmod_verifier_ops,
+ .init = bpf_testmod_ops_init,
+ .init_member = bpf_testmod_ops_init_member,
+ .reg = bpf_dummy_reg,
+ .unreg = bpf_dummy_unreg,
+ .name = "bpf_testmod_ops",
+ .owner = THIS_MODULE,
+};
+
extern int bpf_fentry_test1(int a);
static int bpf_testmod_init(void)
@@ -535,6 +586,7 @@ static int bpf_testmod_init(void)
ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_testmod_kfunc_set);
ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_testmod_kfunc_set);
ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_testmod_kfunc_set);
+ ret = ret ?: REGISTER_BPF_STRUCT_OPS(&bpf_bpf_testmod_ops, bpf_testmod_ops);
if (ret < 0)
return ret;
if (bpf_fentry_test1(0) < 0)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
index f32793efe095..ca5435751c79 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
@@ -28,4 +28,9 @@ struct bpf_iter_testmod_seq {
int cnt;
};
+struct bpf_testmod_ops {
+ int (*test_1)(void);
+ int (*test_2)(int a, int b);
+};
+
#endif /* _BPF_TESTMOD_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
new file mode 100644
index 000000000000..55a4c6ed92aa
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -0,0 +1,92 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <time.h>
+
+#include "struct_ops_module.skel.h"
+#include "testmod_unload.skel.h"
+
+static void test_regular_load(void)
+{
+ DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+ struct struct_ops_module *skel;
+ struct testmod_unload *skel_unload;
+ struct bpf_link *link_map_free = NULL;
+ struct bpf_link *link;
+ int err, i;
+
+ skel = struct_ops_module__open_opts(&opts);
+ if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
+ return;
+
+ err = struct_ops_module__load(skel);
+ if (!ASSERT_OK(err, "struct_ops_module_load"))
+ goto cleanup;
+
+ link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+ ASSERT_OK_PTR(link, "attach_test_mod_1");
+
+ /* test_2() will be called from bpf_dummy_reg() in bpf_testmod.c */
+ ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");
+
+ bpf_link__destroy(link);
+
+cleanup:
+ skel_unload = testmod_unload__open_and_load();
+
+ if (ASSERT_OK_PTR(skel_unload, "testmod_unload_open"))
+ link_map_free = bpf_program__attach(skel_unload->progs.trace_map_free);
+ struct_ops_module__destroy(skel);
+
+ if (!ASSERT_OK_PTR(link_map_free, "create_link_map_free"))
+ return;
+
+ /* Wait for the struct_ops map to be freed. Struct_ops maps hold a
+ * refcount to the module btf. And, this function unloads and then
+ * loads bpf_testmod. Without waiting the map to be freed, the next
+ * test may fail to unload the bpf_testmod module since the map is
+ * still holding a refcnt to the module.
+ */
+ for (i = 0; i < 10; i++) {
+ if (skel_unload->bss->bpf_testmod_put)
+ break;
+ usleep(100000);
+ }
+ ASSERT_EQ(skel_unload->bss->bpf_testmod_put, 1, "map_free");
+
+ bpf_link__destroy(link_map_free);
+ testmod_unload__destroy(skel_unload);
+}
+
+static void test_load_without_module(void)
+{
+ DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+ struct struct_ops_module *skel;
+ int err;
+
+ err = unload_bpf_testmod(false);
+ if (!ASSERT_OK(err, "unload_bpf_testmod"))
+ return;
+
+ skel = struct_ops_module__open_opts(&opts);
+ if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
+ goto cleanup;
+ err = struct_ops_module__load(skel);
+ ASSERT_ERR(err, "struct_ops_module_load");
+
+ struct_ops_module__destroy(skel);
+
+cleanup:
+ /* Without this, the next test may fail */
+ load_bpf_testmod(false);
+}
+
+void serial_test_struct_ops_module(void)
+{
+ if (test__start_subtest("regular_load"))
+ test_regular_load();
+
+ if (test__start_subtest("load_without_module"))
+ test_load_without_module();
+}
+
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c
new file mode 100644
index 000000000000..cb305d04342f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+
+char _license[] SEC("license") = "GPL";
+
+int test_2_result = 0;
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_1)
+{
+ return 0xdeadbeef;
+}
+
+SEC("struct_ops/test_2")
+int BPF_PROG(test_2, int a, int b)
+{
+ test_2_result = a + b;
+ return a + b;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_1 = {
+ .test_1 = (void *)test_1,
+ .test_2 = (void *)test_2,
+};
+
diff --git a/tools/testing/selftests/bpf/progs/testmod_unload.c b/tools/testing/selftests/bpf/progs/testmod_unload.c
new file mode 100644
index 000000000000..bb17914ecca3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/testmod_unload.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_core_read.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+int bpf_testmod_put = 0;
+
+SEC("fentry/__bpf_struct_ops_map_free")
+int BPF_PROG(trace_map_free, struct bpf_map *map)
+{
+ static const char name[] = "testmod_1";
+ int i;
+
+ for (i = 0; i < sizeof(name); i++) {
+ if (map->name[i] != name[i])
+ return 0;
+ }
+
+ bpf_testmod_put = 1;
+
+ return 0;
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 13/14] selftests/bpf: test case for register_bpf_struct_ops().
2023-12-09 0:27 ` [PATCH bpf-next v13 13/14] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
@ 2023-12-15 7:17 ` Martin KaFai Lau
2023-12-17 7:32 ` Kui-Feng Lee
0 siblings, 1 reply; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-15 7:17 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> new file mode 100644
> index 000000000000..55a4c6ed92aa
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
> +#include <test_progs.h>
> +#include <time.h>
> +
> +#include "struct_ops_module.skel.h"
> +#include "testmod_unload.skel.h"
> +
> +static void test_regular_load(void)
> +{
> + DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> + struct struct_ops_module *skel;
> + struct testmod_unload *skel_unload;
> + struct bpf_link *link_map_free = NULL;
> + struct bpf_link *link;
> + int err, i;
> +
> + skel = struct_ops_module__open_opts(&opts);
> + if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
> + return;
> +
> + err = struct_ops_module__load(skel);
> + if (!ASSERT_OK(err, "struct_ops_module_load"))
> + goto cleanup;
> +
> + link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
> + ASSERT_OK_PTR(link, "attach_test_mod_1");
> +
> + /* test_2() will be called from bpf_dummy_reg() in bpf_testmod.c */
> + ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");
> +
> + bpf_link__destroy(link);
> +
> +cleanup:
> + skel_unload = testmod_unload__open_and_load();
> +
> + if (ASSERT_OK_PTR(skel_unload, "testmod_unload_open"))
> + link_map_free = bpf_program__attach(skel_unload->progs.trace_map_free);
> + struct_ops_module__destroy(skel);
> +
> + if (!ASSERT_OK_PTR(link_map_free, "create_link_map_free"))
> + return;
> +
> + /* Wait for the struct_ops map to be freed. Struct_ops maps hold a
> + * refcount to the module btf. And, this function unloads and then
> + * loads bpf_testmod. Without waiting the map to be freed, the next
> + * test may fail to unload the bpf_testmod module since the map is
> + * still holding a refcnt to the module.
> + */
> + for (i = 0; i < 10; i++) {
> + if (skel_unload->bss->bpf_testmod_put)
> + break;
> + usleep(100000);
> + }
> + ASSERT_EQ(skel_unload->bss->bpf_testmod_put, 1, "map_free");
> +
> + bpf_link__destroy(link_map_free);
> + testmod_unload__destroy(skel_unload);
> +}
> +
> +static void test_load_without_module(void)
> +{
> + DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
> + struct struct_ops_module *skel;
> + int err;
> +
> + err = unload_bpf_testmod(false);
> + if (!ASSERT_OK(err, "unload_bpf_testmod"))
> + return;
> +
> + skel = struct_ops_module__open_opts(&opts);
> + if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
> + goto cleanup;
> + err = struct_ops_module__load(skel);
Both the module btf and the .ko itself are gone from the kernel now?
This is basically testing libbpf cannot find 'struct bpf_testmod_ops' from the
running kernel?
How about create another struct_ops_module_notfound.c bpf program:
SEC(".struct_ops.link")
struct bpf_testmod_ops_notfound testmod_1 = {
.test_1 = (void *)test_1,
.test_2 = (void *)test_2,
};
and avoid the usleep() wait and the unload_bpf_testmod()?
> + ASSERT_ERR(err, "struct_ops_module_load");
> +
> + struct_ops_module__destroy(skel);
> +
> +cleanup:
> + /* Without this, the next test may fail */
> + load_bpf_testmod(false);
> +}
> +
> +void serial_test_struct_ops_module(void)
> +{
> + if (test__start_subtest("regular_load"))
> + test_regular_load();
> +
> + if (test__start_subtest("load_without_module"))
> + test_load_without_module();
> +}
> +
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 13/14] selftests/bpf: test case for register_bpf_struct_ops().
2023-12-15 7:17 ` Martin KaFai Lau
@ 2023-12-17 7:32 ` Kui-Feng Lee
0 siblings, 0 replies; 39+ messages in thread
From: Kui-Feng Lee @ 2023-12-17 7:32 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/14/23 23:17, Martin KaFai Lau wrote:
> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>> diff --git
>> a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> new file mode 100644
>> index 000000000000..55a4c6ed92aa
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> @@ -0,0 +1,92 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
>> +#include <test_progs.h>
>> +#include <time.h>
>> +
>> +#include "struct_ops_module.skel.h"
>> +#include "testmod_unload.skel.h"
>> +
>> +static void test_regular_load(void)
>> +{
>> + DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
>> + struct struct_ops_module *skel;
>> + struct testmod_unload *skel_unload;
>> + struct bpf_link *link_map_free = NULL;
>> + struct bpf_link *link;
>> + int err, i;
>> +
>> + skel = struct_ops_module__open_opts(&opts);
>> + if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
>> + return;
>> +
>> + err = struct_ops_module__load(skel);
>> + if (!ASSERT_OK(err, "struct_ops_module_load"))
>> + goto cleanup;
>> +
>> + link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
>> + ASSERT_OK_PTR(link, "attach_test_mod_1");
>> +
>> + /* test_2() will be called from bpf_dummy_reg() in bpf_testmod.c */
>> + ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");
>> +
>> + bpf_link__destroy(link);
>> +
>> +cleanup:
>> + skel_unload = testmod_unload__open_and_load();
>> +
>> + if (ASSERT_OK_PTR(skel_unload, "testmod_unload_open"))
>> + link_map_free =
>> bpf_program__attach(skel_unload->progs.trace_map_free);
>> + struct_ops_module__destroy(skel);
>> +
>> + if (!ASSERT_OK_PTR(link_map_free, "create_link_map_free"))
>> + return;
>> +
>> + /* Wait for the struct_ops map to be freed. Struct_ops maps hold a
>> + * refcount to the module btf. And, this function unloads and then
>> + * loads bpf_testmod. Without waiting the map to be freed, the next
>> + * test may fail to unload the bpf_testmod module since the map is
>> + * still holding a refcnt to the module.
>> + */
>> + for (i = 0; i < 10; i++) {
>> + if (skel_unload->bss->bpf_testmod_put)
>> + break;
>> + usleep(100000);
>> + }
>> + ASSERT_EQ(skel_unload->bss->bpf_testmod_put, 1, "map_free");
>> +
>> + bpf_link__destroy(link_map_free);
>> + testmod_unload__destroy(skel_unload);
>> +}
>> +
>> +static void test_load_without_module(void)
>> +{
>> + DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
>> + struct struct_ops_module *skel;
>> + int err;
>> +
>> + err = unload_bpf_testmod(false);
>> + if (!ASSERT_OK(err, "unload_bpf_testmod"))
>> + return;
>> +
>> + skel = struct_ops_module__open_opts(&opts);
>> + if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
>> + goto cleanup;
>> + err = struct_ops_module__load(skel);
>
> Both the module btf and the .ko itself are gone from the kernel now?
> This is basically testing libbpf cannot find 'struct bpf_testmod_ops'
> from the running kernel?
Yes, you are right! So, I just rewrote this by calling bpf_map_create()
instead of calling the skeleton. To simplify the test, I actually use
bpf_map_info of an existing map created from the skeleton as inputs to
bpf_map_create(). And, the btf_obj_id (or btf_vmlinux_id) is used and
tested here.
>
> How about create another struct_ops_module_notfound.c bpf program:
> SEC(".struct_ops.link")
> struct bpf_testmod_ops_notfound testmod_1 = {
> .test_1 = (void *)test_1,
> .test_2 = (void *)test_2,
> };
>
> and avoid the usleep() wait and the unload_bpf_testmod()?
In order to skip finding module btf for using bpf_map_create(),
I use the skeleton to create a map first to get its bpf_map_info.
So, it still needs to load and unload the same module.
>
>> + ASSERT_ERR(err, "struct_ops_module_load");
>> +
>> + struct_ops_module__destroy(skel);
>> +
>> +cleanup:
>> + /* Without this, the next test may fail */
>> + load_bpf_testmod(false);
>> +}
>> +
>> +void serial_test_struct_ops_module(void)
>> +{
>> + if (test__start_subtest("regular_load"))
>> + test_regular_load();
>> +
>> + if (test__start_subtest("load_without_module"))
>> + test_load_without_module();
>> +}
>> +
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH bpf-next v13 14/14] bpf: pass btf object id in bpf_map_info.
2023-12-09 0:26 [PATCH bpf-next v13 00/14] Registrating struct_ops types from modules thinker.li
` (12 preceding siblings ...)
2023-12-09 0:27 ` [PATCH bpf-next v13 13/14] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
@ 2023-12-09 0:27 ` thinker.li
2023-12-15 7:46 ` Martin KaFai Lau
13 siblings, 1 reply; 39+ messages in thread
From: thinker.li @ 2023-12-09 0:27 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: sinquersw, kuifeng, Kui-Feng Lee
From: Kui-Feng Lee <thinker.li@gmail.com>
Include btf object id (btf_obj_id) in bpf_map_info so that tools (ex:
bpftools struct_ops dump) know the correct btf from the kernel to look up
type information of struct_ops types.
Since struct_ops types can be defined and registered in a module. The
type information of a struct_ops type are defined in the btf of the
module defining it. The userspace tools need to know which btf is for
the module defining a struct_ops type.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/bpf.h | 1 +
include/uapi/linux/bpf.h | 2 +-
kernel/bpf/bpf_struct_ops.c | 7 +++++++
kernel/bpf/syscall.c | 2 ++
tools/include/uapi/linux/bpf.h | 2 +-
5 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c881befa35f5..26103d8a4374 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3350,5 +3350,6 @@ struct bpf_struct_ops_##_name { \
int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
struct btf *btf,
struct bpf_verifier_log *log);
+void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct bpf_map *map);
#endif /* _LINUX_BPF_H */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 5c3838a97554..716c6b28764d 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -6534,7 +6534,7 @@ struct bpf_map_info {
__u32 btf_id;
__u32 btf_key_type_id;
__u32 btf_value_type_id;
- __u32 :32; /* alignment pad */
+ __u32 btf_obj_id;
__u64 map_extra;
} __attribute__((aligned(8)));
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index fd26716fa0f9..51c0de75aa85 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -979,3 +979,10 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
kfree(link);
return err;
}
+
+void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct bpf_map *map)
+{
+ struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
+
+ info->btf_obj_id = btf_obj_id(st_map->btf);
+}
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4aced7e58904..3cab56cd02ff 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4715,6 +4715,8 @@ static int bpf_map_get_info_by_fd(struct file *file,
info.btf_value_type_id = map->btf_value_type_id;
}
info.btf_vmlinux_value_type_id = map->btf_vmlinux_value_type_id;
+ if (map->map_type == BPF_MAP_TYPE_STRUCT_OPS)
+ bpf_map_struct_ops_info_fill(&info, map);
if (bpf_map_is_offloaded(map)) {
err = bpf_map_offload_info_fill(&info, map);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 5c3838a97554..716c6b28764d 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -6534,7 +6534,7 @@ struct bpf_map_info {
__u32 btf_id;
__u32 btf_key_type_id;
__u32 btf_value_type_id;
- __u32 :32; /* alignment pad */
+ __u32 btf_obj_id;
__u64 map_extra;
} __attribute__((aligned(8)));
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 14/14] bpf: pass btf object id in bpf_map_info.
2023-12-09 0:27 ` [PATCH bpf-next v13 14/14] bpf: pass btf object id in bpf_map_info thinker.li
@ 2023-12-15 7:46 ` Martin KaFai Lau
2023-12-17 7:35 ` Kui-Feng Lee
0 siblings, 1 reply; 39+ messages in thread
From: Martin KaFai Lau @ 2023-12-15 7:46 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> Include btf object id (btf_obj_id) in bpf_map_info so that tools (ex:
> bpftools struct_ops dump) know the correct btf from the kernel to look up
> type information of struct_ops types.
>
> Since struct_ops types can be defined and registered in a module. The
> type information of a struct_ops type are defined in the btf of the
> module defining it. The userspace tools need to know which btf is for
> the module defining a struct_ops type.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
> include/linux/bpf.h | 1 +
> include/uapi/linux/bpf.h | 2 +-
> kernel/bpf/bpf_struct_ops.c | 7 +++++++
> kernel/bpf/syscall.c | 2 ++
> tools/include/uapi/linux/bpf.h | 2 +-
> 5 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index c881befa35f5..26103d8a4374 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -3350,5 +3350,6 @@ struct bpf_struct_ops_##_name { \
> int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
> struct btf *btf,
> struct bpf_verifier_log *log);
> +void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct bpf_map *map);
This needs to be in the CONFIG_BPF_JIT guard also.
>
> #endif /* _LINUX_BPF_H */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 5c3838a97554..716c6b28764d 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -6534,7 +6534,7 @@ struct bpf_map_info {
> __u32 btf_id;
> __u32 btf_key_type_id;
> __u32 btf_value_type_id;
> - __u32 :32; /* alignment pad */
> + __u32 btf_obj_id;
may be "btf_vmlinux_id" to make it clear it is a kernel btf and should be used
with map_info->btf_vmlinux_value_type_id.
> __u64 map_extra;
> } __attribute__((aligned(8)));
>
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index fd26716fa0f9..51c0de75aa85 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -979,3 +979,10 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
> kfree(link);
> return err;
> }
> +
> +void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct bpf_map *map)
> +{
> + struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map *)map;
> +
> + info->btf_obj_id = btf_obj_id(st_map->btf);
> +}
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 4aced7e58904..3cab56cd02ff 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -4715,6 +4715,8 @@ static int bpf_map_get_info_by_fd(struct file *file,
> info.btf_value_type_id = map->btf_value_type_id;
> }
> info.btf_vmlinux_value_type_id = map->btf_vmlinux_value_type_id;
> + if (map->map_type == BPF_MAP_TYPE_STRUCT_OPS)
> + bpf_map_struct_ops_info_fill(&info, map);
This patch should be moved earlier in the set instead of after the selftest
patch 13. May be after patch 5 where st_map->btf is added.
and where is the test for this?
>
> if (bpf_map_is_offloaded(map)) {
> err = bpf_map_offload_info_fill(&info, map);
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 5c3838a97554..716c6b28764d 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -6534,7 +6534,7 @@ struct bpf_map_info {
> __u32 btf_id;
> __u32 btf_key_type_id;
> __u32 btf_value_type_id;
> - __u32 :32; /* alignment pad */
> + __u32 btf_obj_id;
> __u64 map_extra;
> } __attribute__((aligned(8)));
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH bpf-next v13 14/14] bpf: pass btf object id in bpf_map_info.
2023-12-15 7:46 ` Martin KaFai Lau
@ 2023-12-17 7:35 ` Kui-Feng Lee
0 siblings, 0 replies; 39+ messages in thread
From: Kui-Feng Lee @ 2023-12-17 7:35 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 12/14/23 23:46, Martin KaFai Lau wrote:
> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Include btf object id (btf_obj_id) in bpf_map_info so that tools (ex:
>> bpftools struct_ops dump) know the correct btf from the kernel to look up
>> type information of struct_ops types.
>>
>> Since struct_ops types can be defined and registered in a module. The
>> type information of a struct_ops type are defined in the btf of the
>> module defining it. The userspace tools need to know which btf is for
>> the module defining a struct_ops type.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>> include/linux/bpf.h | 1 +
>> include/uapi/linux/bpf.h | 2 +-
>> kernel/bpf/bpf_struct_ops.c | 7 +++++++
>> kernel/bpf/syscall.c | 2 ++
>> tools/include/uapi/linux/bpf.h | 2 +-
>> 5 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index c881befa35f5..26103d8a4374 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -3350,5 +3350,6 @@ struct bpf_struct_ops_##_name
>> { \
>> int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
>> struct btf *btf,
>> struct bpf_verifier_log *log);
>> +void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct
>> bpf_map *map);
>
> This needs to be in the CONFIG_BPF_JIT guard also.
Got it!
>
>> #endif /* _LINUX_BPF_H */
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 5c3838a97554..716c6b28764d 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -6534,7 +6534,7 @@ struct bpf_map_info {
>> __u32 btf_id;
>> __u32 btf_key_type_id;
>> __u32 btf_value_type_id;
>> - __u32 :32; /* alignment pad */
>> + __u32 btf_obj_id;
>
> may be "btf_vmlinux_id" to make it clear it is a kernel btf and should
> be used with map_info->btf_vmlinux_value_type_id.
Sure!
>
>> __u64 map_extra;
>> } __attribute__((aligned(8)));
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index fd26716fa0f9..51c0de75aa85 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -979,3 +979,10 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
>> kfree(link);
>> return err;
>> }
>> +
>> +void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct
>> bpf_map *map)
>> +{
>> + struct bpf_struct_ops_map *st_map = (struct bpf_struct_ops_map
>> *)map;
>> +
>> + info->btf_obj_id = btf_obj_id(st_map->btf);
>> +}
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 4aced7e58904..3cab56cd02ff 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -4715,6 +4715,8 @@ static int bpf_map_get_info_by_fd(struct file
>> *file,
>> info.btf_value_type_id = map->btf_value_type_id;
>> }
>> info.btf_vmlinux_value_type_id = map->btf_vmlinux_value_type_id;
>> + if (map->map_type == BPF_MAP_TYPE_STRUCT_OPS)
>> + bpf_map_struct_ops_info_fill(&info, map);
>
> This patch should be moved earlier in the set instead of after the
> selftest patch 13. May be after patch 5 where st_map->btf is added.
No problem
>
> and where is the test for this?
I use bpf_map_info as a part of calling bpf_map_create() while
testmod.ko is unloaded. It check if this change work as well.
>
>> if (bpf_map_is_offloaded(map)) {
>> err = bpf_map_offload_info_fill(&info, map);
>> diff --git a/tools/include/uapi/linux/bpf.h
>> b/tools/include/uapi/linux/bpf.h
>> index 5c3838a97554..716c6b28764d 100644
>> --- a/tools/include/uapi/linux/bpf.h
>> +++ b/tools/include/uapi/linux/bpf.h
>> @@ -6534,7 +6534,7 @@ struct bpf_map_info {
>> __u32 btf_id;
>> __u32 btf_key_type_id;
>> __u32 btf_value_type_id;
>> - __u32 :32; /* alignment pad */
>> + __u32 btf_obj_id;
>> __u64 map_extra;
>> } __attribute__((aligned(8)));
>
^ permalink raw reply [flat|nested] 39+ messages in thread