* [PATCH bpf-next v5 1/9] bpf: refactory struct_ops type initialization to a function.
2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
@ 2023-10-17 16:22 ` thinker.li
2023-10-17 16:22 ` [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf thinker.li
` (7 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:22 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>
---
kernel/bpf/bpf_struct_ops.c | 157 +++++++++++++++++++-----------------
1 file changed, 83 insertions(+), 74 deletions(-)
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index db6176fb64dc..627cf1ea840a 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 btf_vmlinux\n",
+ value_name);
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 btf_vmlinux\n",
+ st_ops->name);
+ 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 btf_vmlinux\n");
+ 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);
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf.
2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
2023-10-17 16:22 ` [PATCH bpf-next v5 1/9] bpf: refactory struct_ops type initialization to a function thinker.li
@ 2023-10-17 16:22 ` thinker.li
2023-10-19 0:00 ` Martin KaFai Lau
2023-10-19 2:28 ` Martin KaFai Lau
2023-10-17 16:23 ` [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map thinker.li
` (6 subsequent siblings)
8 siblings, 2 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:22 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.
Every struct_ops type should have an associated module BTF to provide type
information since we are going to allow modules to define and register new
struct_ops types. Once this change is made, the bpf_struct_ops subsystem
knows where to look up type info with just a bpf_struct_ops.
The subsystem looks up struct_ops types from a given module BTF although it
is always btf_vmlinux now. Once start using struct_ops_tab, btfs other than
btf_vmlinux can be used as well.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/bpf.h | 5 +--
include/linux/btf.h | 6 ++++
kernel/bpf/bpf_struct_ops.c | 17 ++++-----
kernel/bpf/btf.c | 70 +++++++++++++++++++++++++++++++++++++
kernel/bpf/verifier.c | 2 +-
5 files changed, 89 insertions(+), 11 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 30063a760b5a..e6a648af2daa 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1626,6 +1626,7 @@ struct bpf_struct_ops {
void (*unreg)(void *kdata);
int (*update)(void *kdata, void *old_kdata);
int (*validate)(void *kdata);
+ struct btf *btf;
const struct btf_type *type;
const struct btf_type *value_type;
const char *name;
@@ -1636,7 +1637,7 @@ struct bpf_struct_ops {
#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 *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);
@@ -1679,7 +1680,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 *bpf_struct_ops_find(struct btf *btf, u32 type_id)
{
return NULL;
}
diff --git a/include/linux/btf.h b/include/linux/btf.h
index 928113a80a95..aa2ba77648be 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -571,4 +571,10 @@ 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;
+
+int btf_add_struct_ops(struct bpf_struct_ops *st_ops);
+const struct bpf_struct_ops **
+btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
+
#endif
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 627cf1ea840a..7758f66ad734 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -185,6 +185,7 @@ 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->btf = btf;
st_ops->type_id = type_id;
st_ops->type = t;
st_ops->value_id = value_id;
@@ -221,7 +222,7 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
extern struct btf *btf_vmlinux;
static const struct bpf_struct_ops *
-bpf_struct_ops_find_value(u32 value_id)
+bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
{
unsigned int i;
@@ -236,7 +237,7 @@ bpf_struct_ops_find_value(u32 value_id)
return NULL;
}
-const struct bpf_struct_ops *bpf_struct_ops_find(u32 type_id)
+const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf, u32 type_id)
{
unsigned int i;
@@ -316,7 +317,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;
@@ -328,8 +329,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;
@@ -395,12 +396,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->value_type, value);
+ err = check_zero_holes(st_ops->btf, st_ops->value_type, value);
if (err)
return err;
uvalue = value;
- err = check_zero_holes(t, uvalue->data);
+ err = check_zero_holes(st_ops->btf, t, uvalue->data);
if (err)
return err;
@@ -671,7 +672,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
struct bpf_map *map;
int ret;
- st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
+ st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
if (!st_ops)
return ERR_PTR(-ENOTSUPP);
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index f93e835d90af..be5144dbb53d 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 *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);
@@ -8601,3 +8617,57 @@ bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log,
return !strncmp(reg_name, arg_name, cmp_len);
}
+
+int btf_add_struct_ops(struct bpf_struct_ops *st_ops)
+{
+ struct btf_struct_ops_tab *tab, *new_tab;
+ struct btf *btf = st_ops->btf;
+ 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)
+ return -EEXIST;
+
+ if (tab->cnt == tab->capacity) {
+ new_tab = krealloc(tab, sizeof(*tab) +
+ sizeof(struct bpf_struct_ops *) *
+ tab->capacity * 2, GFP_KERNEL);
+ if (!new_tab)
+ return -ENOMEM;
+ tab = new_tab;
+ tab->capacity *= 2;
+ btf->struct_ops_tab = tab;
+ }
+
+ btf->struct_ops_tab->ops[btf->struct_ops_tab->cnt++] = st_ops;
+
+ return 0;
+}
+
+const struct bpf_struct_ops **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 **)btf->struct_ops_tab->ops;
+}
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a7178ecf676d..6564a03c425d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19631,7 +19631,7 @@ 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);
+ st_ops = bpf_struct_ops_find(btf_vmlinux, btf_id);
if (!st_ops) {
verbose(env, "attach_btf_id %u is not a supported struct\n",
btf_id);
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf.
2023-10-17 16:22 ` [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf thinker.li
@ 2023-10-19 0:00 ` Martin KaFai Lau
2023-10-19 0:33 ` Kui-Feng Lee
2023-10-19 2:28 ` Martin KaFai Lau
1 sibling, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-19 0:00 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 10/17/23 9:22 AM, thinker.li@gmail.com wrote:
> static const struct bpf_struct_ops *
> -bpf_struct_ops_find_value(u32 value_id)
> +bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
> {
> unsigned int i;
>
[ ... ]
> @@ -671,7 +672,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
> struct bpf_map *map;
> int ret;
>
> - st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
> + st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
This patch does not compile because of the argument ordering.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf.
2023-10-19 0:00 ` Martin KaFai Lau
@ 2023-10-19 0:33 ` Kui-Feng Lee
0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-19 0:33 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 10/18/23 17:00, Martin KaFai Lau wrote:
> On 10/17/23 9:22 AM, thinker.li@gmail.com wrote:
>> static const struct bpf_struct_ops *
>> -bpf_struct_ops_find_value(u32 value_id)
>> +bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
>> {
>> unsigned int i;
>
> [ ... ]
>
>> @@ -671,7 +672,7 @@ static struct bpf_map
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>> struct bpf_map *map;
>> int ret;
>> - st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id);
>> + st_ops =
>> bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
>
> This patch does not compile because of the argument ordering.
>
>
Looks like I split the patch wrongly! Will fix it!
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf.
2023-10-17 16:22 ` [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf thinker.li
2023-10-19 0:00 ` Martin KaFai Lau
@ 2023-10-19 2:28 ` Martin KaFai Lau
2023-10-19 16:15 ` Kui-Feng Lee
1 sibling, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-19 2:28 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 10/17/23 9:22 AM, thinker.li@gmail.com wrote:
> +const struct bpf_struct_ops **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 **)btf->struct_ops_tab->ops;
Is it possible that the module is already gone here? If that is the case, the
st_ops pointer probably cannot be used?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf.
2023-10-19 2:28 ` Martin KaFai Lau
@ 2023-10-19 16:15 ` Kui-Feng Lee
0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-19 16:15 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 10/18/23 19:28, Martin KaFai Lau wrote:
> On 10/17/23 9:22 AM, thinker.li@gmail.com wrote:
>> +const struct bpf_struct_ops **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 **)btf->struct_ops_tab->ops;
>
> Is it possible that the module is already gone here? If that is the
> case, the st_ops pointer probably cannot be used?
The callers should call bpf_try_get_module() before calling this
function. I will check the code to ensure all callers do that in
the next version.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
2023-10-17 16:22 ` [PATCH bpf-next v5 1/9] bpf: refactory struct_ops type initialization to a function thinker.li
2023-10-17 16:22 ` [PATCH bpf-next v5 2/9] bpf: add struct_ops_tab to btf thinker.li
@ 2023-10-17 16:23 ` thinker.li
2023-10-19 0:36 ` Martin KaFai Lau
2023-10-17 16:23 ` [PATCH bpf-next v5 4/9] bpf: validate value_type thinker.li
` (5 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 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.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/bpf.h | 1 +
kernel/bpf/bpf_struct_ops.c | 21 ++++++++++++++++++---
2 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index e6a648af2daa..1e1647c8b0ce 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1627,6 +1627,7 @@ struct bpf_struct_ops {
int (*update)(void *kdata, void *old_kdata);
int (*validate)(void *kdata);
struct btf *btf;
+ struct module *owner;
const struct btf_type *type;
const struct btf_type *value_type;
const char *name;
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 7758f66ad734..b561245fe235 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -112,6 +112,7 @@ static const struct btf_type *module_type;
static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
struct btf *btf,
+ struct module *owner,
struct bpf_verifier_log *log)
{
const struct btf_member *member;
@@ -186,6 +187,7 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
st_ops->name);
} else {
st_ops->btf = btf;
+ st_ops->owner = owner;
st_ops->type_id = type_id;
st_ops->type = t;
st_ops->value_id = value_id;
@@ -193,6 +195,7 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
value_id);
}
}
+
}
void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
@@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
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);
+ bpf_struct_ops_init_one(st_ops, btf, NULL, log);
}
}
@@ -630,6 +633,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);
+ module_put(st_map->st_ops->owner);
bpf_map_area_free(st_map);
}
@@ -676,9 +680,18 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
if (!st_ops)
return ERR_PTR(-ENOTSUPP);
+ /* If st_ops->owner is NULL, it means the struct_ops is
+ * statically defined in the kernel. We don't need to
+ * take a refcount on it.
+ */
+ if (st_ops->owner && !btf_try_get_module(st_ops->btf))
+ return ERR_PTR(-EINVAL);
+
vt = st_ops->value_type;
- if (attr->value_size != vt->size)
+ if (attr->value_size != vt->size) {
+ module_put(st_ops->owner);
return ERR_PTR(-EINVAL);
+ }
t = st_ops->type;
@@ -689,8 +702,10 @@ 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)
+ if (!st_map) {
+ module_put(st_ops->owner);
return ERR_PTR(-ENOMEM);
+ }
st_map->st_ops = st_ops;
map = &st_map->map;
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
2023-10-17 16:23 ` [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map thinker.li
@ 2023-10-19 0:36 ` Martin KaFai Lau
2023-10-19 16:29 ` Kui-Feng Lee
0 siblings, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-19 0:36 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 10/17/23 9:23 AM, 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.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
> include/linux/bpf.h | 1 +
> kernel/bpf/bpf_struct_ops.c | 21 ++++++++++++++++++---
> 2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index e6a648af2daa..1e1647c8b0ce 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1627,6 +1627,7 @@ struct bpf_struct_ops {
> int (*update)(void *kdata, void *old_kdata);
> int (*validate)(void *kdata);
> struct btf *btf;
> + struct module *owner;
> const struct btf_type *type;
> const struct btf_type *value_type;
> const char *name;
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 7758f66ad734..b561245fe235 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -112,6 +112,7 @@ static const struct btf_type *module_type;
>
> static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
> struct btf *btf,
> + struct module *owner,
> struct bpf_verifier_log *log)
> {
> const struct btf_member *member;
> @@ -186,6 +187,7 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
> st_ops->name);
> } else {
> st_ops->btf = btf;
> + st_ops->owner = owner;
I suspect it will turn out to be just "st_ops->owner = st_ops->owner;" in a
latter patch. st_ops->owner should have already been initialized (with
THIS_MODULE?).
> st_ops->type_id = type_id;
> st_ops->type = t;
> st_ops->value_id = value_id;
> @@ -193,6 +195,7 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
> value_id);
> }
> }
> +
nit. extra newline.
> }
>
> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
> @@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>
> 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);
> + bpf_struct_ops_init_one(st_ops, btf, NULL, log);
> }
> }
>
> @@ -630,6 +633,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);
> + module_put(st_map->st_ops->owner);
> bpf_map_area_free(st_map);
> }
>
> @@ -676,9 +680,18 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
> if (!st_ops)
> return ERR_PTR(-ENOTSUPP);
>
> + /* If st_ops->owner is NULL, it means the struct_ops is
> + * statically defined in the kernel. We don't need to
> + * take a refcount on it.
> + */
> + if (st_ops->owner && !btf_try_get_module(st_ops->btf))
This just came to my mind. Is the module refcnt needed during map alloc/free or
it could be done during the reg/unreg instead?
> + return ERR_PTR(-EINVAL);
> +
> vt = st_ops->value_type;
> - if (attr->value_size != vt->size)
> + if (attr->value_size != vt->size) {
> + module_put(st_ops->owner);
> return ERR_PTR(-EINVAL);
> + }
>
> t = st_ops->type;
>
> @@ -689,8 +702,10 @@ 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)
> + if (!st_map) {
> + module_put(st_ops->owner);
> return ERR_PTR(-ENOMEM);
> + }
>
> st_map->st_ops = st_ops;
> map = &st_map->map;
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
2023-10-19 0:36 ` Martin KaFai Lau
@ 2023-10-19 16:29 ` Kui-Feng Lee
2023-10-20 5:07 ` Kui-Feng Lee
0 siblings, 1 reply; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-19 16:29 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 10/18/23 17:36, Martin KaFai Lau wrote:
> On 10/17/23 9:23 AM, 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.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>> include/linux/bpf.h | 1 +
>> kernel/bpf/bpf_struct_ops.c | 21 ++++++++++++++++++---
>> 2 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index e6a648af2daa..1e1647c8b0ce 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -1627,6 +1627,7 @@ struct bpf_struct_ops {
>> int (*update)(void *kdata, void *old_kdata);
>> int (*validate)(void *kdata);
>> struct btf *btf;
>> + struct module *owner;
>> const struct btf_type *type;
>> const struct btf_type *value_type;
>> const char *name;
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 7758f66ad734..b561245fe235 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -112,6 +112,7 @@ static const struct btf_type *module_type;
>> static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
>> struct btf *btf,
>> + struct module *owner,
>> struct bpf_verifier_log *log)
>> {
>> const struct btf_member *member;
>> @@ -186,6 +187,7 @@ static void bpf_struct_ops_init_one(struct
>> bpf_struct_ops *st_ops,
>> st_ops->name);
>> } else {
>> st_ops->btf = btf;
>> + st_ops->owner = owner;
>
> I suspect it will turn out to be just "st_ops->owner = st_ops->owner;"
> in a latter patch. st_ops->owner should have already been initialized
> (with THIS_MODULE?).
Yes, you are correct. It ends up st_ops->owner passing from the caller.
I will remove this line and the argument.
>
>> st_ops->type_id = type_id;
>> st_ops->type = t;
>> st_ops->value_id = value_id;
>> @@ -193,6 +195,7 @@ static void bpf_struct_ops_init_one(struct
>> bpf_struct_ops *st_ops,
>> value_id);
>> }
>> }
>> +
>
> nit. extra newline.
got it!
>
>> }
>> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>> @@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct
>> bpf_verifier_log *log)
>> 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);
>> + bpf_struct_ops_init_one(st_ops, btf, NULL, log);
>> }
>> }
>> @@ -630,6 +633,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);
>> + module_put(st_map->st_ops->owner);
>> bpf_map_area_free(st_map);
>> }
>> @@ -676,9 +680,18 @@ static struct bpf_map
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>> if (!st_ops)
>> return ERR_PTR(-ENOTSUPP);
>> + /* If st_ops->owner is NULL, it means the struct_ops is
>> + * statically defined in the kernel. We don't need to
>> + * take a refcount on it.
>> + */
>> + if (st_ops->owner && !btf_try_get_module(st_ops->btf))
>
> This just came to my mind. Is the module refcnt needed during map
> alloc/free or it could be done during the reg/unreg instead?
Sure, I can move it to reg/unreg.
>
>
>> + return ERR_PTR(-EINVAL);
>> +
>> vt = st_ops->value_type;
>> - if (attr->value_size != vt->size)
>> + if (attr->value_size != vt->size) {
>> + module_put(st_ops->owner);
>> return ERR_PTR(-EINVAL);
>> + }
>> t = st_ops->type;
>> @@ -689,8 +702,10 @@ 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)
>> + if (!st_map) {
>> + module_put(st_ops->owner);
>> return ERR_PTR(-ENOMEM);
>> + }
>> st_map->st_ops = st_ops;
>> map = &st_map->map;
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
2023-10-19 16:29 ` Kui-Feng Lee
@ 2023-10-20 5:07 ` Kui-Feng Lee
2023-10-20 21:37 ` Martin KaFai Lau
0 siblings, 1 reply; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-20 5:07 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 10/19/23 09:29, Kui-Feng Lee wrote:
>
>
> On 10/18/23 17:36, Martin KaFai Lau wrote:
>> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>
>>
>>> }
>>> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log
>>> *log)
>>> @@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct
>>> bpf_verifier_log *log)
>>> 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);
>>> + bpf_struct_ops_init_one(st_ops, btf, NULL, log);
>>> }
>>> }
>>> @@ -630,6 +633,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);
>>> + module_put(st_map->st_ops->owner);
>>> bpf_map_area_free(st_map);
>>> }
>>> @@ -676,9 +680,18 @@ static struct bpf_map
>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>> if (!st_ops)
>>> return ERR_PTR(-ENOTSUPP);
>>> + /* If st_ops->owner is NULL, it means the struct_ops is
>>> + * statically defined in the kernel. We don't need to
>>> + * take a refcount on it.
>>> + */
>>> + if (st_ops->owner && !btf_try_get_module(st_ops->btf))
>>
>> This just came to my mind. Is the module refcnt needed during map
>> alloc/free or it could be done during the reg/unreg instead?
>
>
> Sure, I can move it to reg/unreg.
Just found that we relies type information in st_ops to update element
and clean up maps.
We can not move get/put modules to reg/unreg except keeping a redundant
copy in
st_map or somewhere. It make the code much more complicated by
introducing get/put module here and there.
I prefer to keep as it is now. WDYT?
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
2023-10-20 5:07 ` Kui-Feng Lee
@ 2023-10-20 21:37 ` Martin KaFai Lau
2023-10-20 22:28 ` Kui-Feng Lee
0 siblings, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-20 21:37 UTC (permalink / raw)
To: Kui-Feng Lee, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 10/19/23 10:07 PM, Kui-Feng Lee wrote:
>
>
> On 10/19/23 09:29, Kui-Feng Lee wrote:
>>
>>
>> On 10/18/23 17:36, Martin KaFai Lau wrote:
>>> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>>
>>>
>>>> }
>>>> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>>>> @@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf, struct
>>>> bpf_verifier_log *log)
>>>> 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);
>>>> + bpf_struct_ops_init_one(st_ops, btf, NULL, log);
>>>> }
>>>> }
>>>> @@ -630,6 +633,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);
>>>> + module_put(st_map->st_ops->owner);
>>>> bpf_map_area_free(st_map);
>>>> }
>>>> @@ -676,9 +680,18 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union
>>>> bpf_attr *attr)
>>>> if (!st_ops)
>>>> return ERR_PTR(-ENOTSUPP);
>>>> + /* If st_ops->owner is NULL, it means the struct_ops is
>>>> + * statically defined in the kernel. We don't need to
>>>> + * take a refcount on it.
>>>> + */
>>>> + if (st_ops->owner && !btf_try_get_module(st_ops->btf))
While replying and looking at it again, I don't think the
btf_try_get_module(st_ops->btf) is safe. The module's owned st_ops itself could
have been gone with the module. The same goes with the "st_ops->owner" test, so
btf_is_module(btf) should be used instead.
I am risking to act like a broken clock to repeat this question, does it really
need to store btf back into the st_ops which may accidentally get into the above
btf_try_get_module(st_ops->btf) usage?
>>>
>>> This just came to my mind. Is the module refcnt needed during map alloc/free
>>> or it could be done during the reg/unreg instead?
>>
>>
>> Sure, I can move it to reg/unreg.
>
> Just found that we relies type information in st_ops to update element and clean
> up maps.
> We can not move get/put modules to reg/unreg except keeping a redundant copy in
> st_map or somewhere. It make the code much more complicated by
> introducing get/put module here and there.
>
> I prefer to keep as it is now. WDYT?
Yeah, sure. I was asking after seeing a longer wait time for the module to go
away in patch 11 selftest and requires an explicit waiting for the tasks_trace
period. Releasing the module refcnt earlier will help.
Regardless of the module refcnt hold/free location, I think storing the type*
and value* in the module's owned st_ops does not look correct now. It was fine
and convenient to piggy back them into bpf_struct_ops when everything was
built-in the kernel and no lifetime concern. It makes sense now to separate them
out from the module's owned st_ops. Something like:
struct btf_struct_ops_desc {
struct bpf_struct_ops *ops;
const struct btf_type *type;
const struct btf_type *value_type;
u32 type_id;
u32 value_id;
};
struct btf_struct_ops_tab {
u32 cnt;
u32 capacity;
struct btf_struct_ops_desc *st_ops_desc[];
};
wdyt?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map.
2023-10-20 21:37 ` Martin KaFai Lau
@ 2023-10-20 22:28 ` Kui-Feng Lee
0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-20 22:28 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 10/20/23 14:37, Martin KaFai Lau wrote:
> On 10/19/23 10:07 PM, Kui-Feng Lee wrote:
>>
>>
>> On 10/19/23 09:29, Kui-Feng Lee wrote:
>>>
>>>
>>> On 10/18/23 17:36, Martin KaFai Lau wrote:
>>>> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>>>
>>>>
>>>>> }
>>>>> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log
>>>>> *log)
>>>>> @@ -215,7 +218,7 @@ void bpf_struct_ops_init(struct btf *btf,
>>>>> struct bpf_verifier_log *log)
>>>>> 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);
>>>>> + bpf_struct_ops_init_one(st_ops, btf, NULL, log);
>>>>> }
>>>>> }
>>>>> @@ -630,6 +633,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);
>>>>> + module_put(st_map->st_ops->owner);
>>>>> bpf_map_area_free(st_map);
>>>>> }
>>>>> @@ -676,9 +680,18 @@ static struct bpf_map
>>>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>>> if (!st_ops)
>>>>> return ERR_PTR(-ENOTSUPP);
>>>>> + /* If st_ops->owner is NULL, it means the struct_ops is
>>>>> + * statically defined in the kernel. We don't need to
>>>>> + * take a refcount on it.
>>>>> + */
>>>>> + if (st_ops->owner && !btf_try_get_module(st_ops->btf))
>
> While replying and looking at it again, I don't think the
> btf_try_get_module(st_ops->btf) is safe. The module's owned st_ops
> itself could have been gone with the module. The same goes with the
> "st_ops->owner" test, so btf_is_module(btf) should be used instead.
I have change it locally. Here, it calls btf_try_get_module() after
calling btf_struct_ops_find_value(). The new code will call
btf_try_get_module() against the btf from attr->value_type_btf_obj_fd
before btf_struct_ops_find_value(). Just like I mentioned earlier to
ensure the callers of btf_struct_ops_find_value() and
btf_struct_ops_find() hold a refcount to the module.
>
> I am risking to act like a broken clock to repeat this question, does it
> really need to store btf back into the st_ops which may accidentally get
> into the above btf_try_get_module(st_ops->btf) usage?
>
>>>>
>>>> This just came to my mind. Is the module refcnt needed during map
>>>> alloc/free or it could be done during the reg/unreg instead?
>>>
>>>
>>> Sure, I can move it to reg/unreg.
>>
>> Just found that we relies type information in st_ops to update element
>> and clean up maps.
>> We can not move get/put modules to reg/unreg except keeping a
>> redundant copy in
>> st_map or somewhere. It make the code much more complicated by
>> introducing get/put module here and there.
>>
>> I prefer to keep as it is now. WDYT?
>
> Yeah, sure. I was asking after seeing a longer wait time for the module
> to go away in patch 11 selftest and requires an explicit waiting for the
> tasks_trace period. Releasing the module refcnt earlier will help.
>
> Regardless of the module refcnt hold/free location, I think storing the
> type* and value* in the module's owned st_ops does not look correct now.
> It was fine and convenient to piggy back them into bpf_struct_ops when
> everything was built-in the kernel and no lifetime concern. It makes
> sense now to separate them out from the module's owned st_ops. Something
> like:
>
> struct btf_struct_ops_desc {
> struct bpf_struct_ops *ops;
> const struct btf_type *type;
> const struct btf_type *value_type;
> u32 type_id;
> u32 value_id;
> };
>
> struct btf_struct_ops_tab {
> u32 cnt;
> u32 capacity;
> struct btf_struct_ops_desc *st_ops_desc[];
> };
>
> wdyt?
So, st_map should hold a pointer to a bpf_struct_ops_desc instead of
st_ops, right? It would work!
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH bpf-next v5 4/9] bpf: validate value_type
2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
` (2 preceding siblings ...)
2023-10-17 16:23 ` [PATCH bpf-next v5 3/9] bpf: hold module for bpf_struct_ops_map thinker.li
@ 2023-10-17 16:23 ` thinker.li
2023-10-17 16:23 ` [PATCH bpf-next v5 5/9] bpf: pass attached BTF to the bpf_struct_ops subsystem thinker.li
` (4 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 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>
---
kernel/bpf/bpf_struct_ops.c | 88 +++++++++++++++++++++++++++----------
1 file changed, 66 insertions(+), 22 deletions(-)
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index b561245fe235..69703584fa4a 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -20,9 +20,11 @@ enum bpf_struct_ops_state {
BPF_STRUCT_OPS_STATE_READY,
};
-#define BPF_STRUCT_OPS_COMMON_VALUE \
- refcount_t refcnt; \
- enum bpf_struct_ops_state state
+struct bpf_struct_ops_common_value {
+ refcount_t refcnt;
+ enum bpf_struct_ops_state state;
+};
+#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common
struct bpf_struct_ops_value {
BPF_STRUCT_OPS_COMMON_VALUE;
@@ -109,6 +111,38 @@ const struct bpf_prog_ops bpf_struct_ops_prog_ops = {
};
static const struct btf_type *module_type;
+static const struct btf_type *common_value_type;
+
+static bool is_valid_value_type(struct btf *btf, s32 value_id,
+ const struct btf_type *type,
+ const char *value_name)
+{
+ 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);
+ 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_init_one(struct bpf_struct_ops *st_ops,
struct btf *btf,
@@ -130,14 +164,6 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
}
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);
- return;
- }
-
type_id = btf_find_by_name_kind(btf, st_ops->name,
BTF_KIND_STRUCT);
if (type_id < 0) {
@@ -152,6 +178,16 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
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 btf_vmlinux\n",
+ value_name);
+ 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;
@@ -201,7 +237,7 @@ 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;
+ s32 module_id, common_value_id;
u32 i;
/* Ensure BTF type is emitted for "struct bpf_struct_ops_##_name" */
@@ -215,6 +251,14 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
return;
}
module_type = btf_type_by_id(btf, module_id);
+ common_value_id = btf_find_by_name_kind(btf,
+ "bpf_struct_ops_common_value",
+ BTF_KIND_STRUCT);
+ if (common_value_id < 0) {
+ pr_warn("Cannot find struct common_value in btf_vmlinux\n");
+ return;
+ }
+ common_value_type = btf_type_by_id(btf, common_value_id);
for (i = 0; i < ARRAY_SIZE(bpf_struct_ops); i++) {
st_ops = bpf_struct_ops[i];
@@ -278,7 +322,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;
@@ -289,7 +333,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
@@ -297,7 +341,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;
}
@@ -408,7 +452,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);
@@ -420,7 +464,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;
}
@@ -533,7 +577,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;
}
@@ -551,7 +595,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;
}
@@ -582,7 +626,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) {
@@ -676,7 +720,7 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
struct bpf_map *map;
int ret;
- st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf_vmlinux);
+ st_ops = bpf_struct_ops_find_value(btf_vmlinux, attr->btf_vmlinux_value_type_id);
if (!st_ops)
return ERR_PTR(-ENOTSUPP);
@@ -805,7 +849,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] 27+ messages in thread
* [PATCH bpf-next v5 5/9] bpf: pass attached BTF to the bpf_struct_ops subsystem
2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
` (3 preceding siblings ...)
2023-10-17 16:23 ` [PATCH bpf-next v5 4/9] bpf: validate value_type thinker.li
@ 2023-10-17 16:23 ` thinker.li
2023-10-17 16:23 ` [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration thinker.li
` (3 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 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>
Giving a BTF, the bpf_struct_ops knows the right place to look up type info
associated with a type ID. This enables a user space program to load a
struct_ops object linked to a struct_ops type defined by a module, by
providing the module BTF (fd).
The bpf_prog includes attach_btf in aux which is passed along with the
bpf_attr when loading the program. The purpose of attach_btf is to
determine the btf type of attach_btf_id. The attach_btf_id is then used to
identify the traced function for a trace program. In the case of struct_ops
programs, it is used to identify the struct_ops type of the struct_ops
object that a program is attached to.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/uapi/linux/bpf.h | 5 +++++
kernel/bpf/bpf_struct_ops.c | 34 +++++++++++++++++++++++++++-------
kernel/bpf/syscall.c | 2 +-
kernel/bpf/verifier.c | 4 +++-
tools/include/uapi/linux/bpf.h | 5 +++++
5 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 73b155e52204..b5ef22f65f35 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1390,6 +1390,11 @@ union bpf_attr {
* to using 5 hash functions).
*/
__u64 map_extra;
+
+ __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 69703584fa4a..60445ff32275 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -677,6 +677,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->st_ops->btf);
module_put(st_map->st_ops->owner);
bpf_map_area_free(st_map);
}
@@ -718,23 +719,36 @@ 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 = bpf_struct_ops_find_value(btf_vmlinux, attr->btf_vmlinux_value_type_id);
- if (!st_ops)
+ if (attr->value_type_btf_obj_fd) {
+ 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 = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
+ if (!st_ops) {
+ btf_put(btf);
return ERR_PTR(-ENOTSUPP);
+ }
/* If st_ops->owner is NULL, it means the struct_ops is
* statically defined in the kernel. We don't need to
* take a refcount on it.
*/
- if (st_ops->owner && !btf_try_get_module(st_ops->btf))
+ if (st_ops->owner && !btf_try_get_module(st_ops->btf)) {
+ btf_put(btf);
return ERR_PTR(-EINVAL);
+ }
vt = st_ops->value_type;
if (attr->value_size != vt->size) {
- module_put(st_ops->owner);
- return ERR_PTR(-EINVAL);
+ ret = -EINVAL;
+ goto errout;
}
t = st_ops->type;
@@ -747,8 +761,8 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
st_map = bpf_map_area_alloc(st_map_size, NUMA_NO_NODE);
if (!st_map) {
- module_put(st_ops->owner);
- return ERR_PTR(-ENOMEM);
+ ret = -ENOMEM;
+ goto errout;
}
st_map->st_ops = st_ops;
@@ -784,6 +798,12 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
bpf_map_init_from_attr(map, attr);
return map;
+
+errout:
+ btf_put(btf);
+ module_put(st_ops->owner);
+
+ 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 85c1d908f70f..5daf8a2c2bba 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1097,7 +1097,7 @@ static int map_check_btf(struct bpf_map *map, const struct btf *btf,
return ret;
}
-#define BPF_MAP_CREATE_LAST_FIELD map_extra
+#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 6564a03c425d..ce4df24eb03b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19623,6 +19623,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) {
@@ -19630,8 +19631,9 @@ 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 = bpf_struct_ops_find(btf_vmlinux, btf_id);
+ st_ops = bpf_struct_ops_find(btf, btf_id);
if (!st_ops) {
verbose(env, "attach_btf_id %u is not a supported struct\n",
btf_id);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 73b155e52204..b5ef22f65f35 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1390,6 +1390,11 @@ union bpf_attr {
* to using 5 hash functions).
*/
__u64 map_extra;
+
+ __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] 27+ messages in thread
* [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration
2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
` (4 preceding siblings ...)
2023-10-17 16:23 ` [PATCH bpf-next v5 5/9] bpf: pass attached BTF to the bpf_struct_ops subsystem thinker.li
@ 2023-10-17 16:23 ` thinker.li
2023-10-19 1:49 ` Martin KaFai Lau
2023-10-17 16:23 ` [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
` (2 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 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 pre-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 | 2 +
include/linux/btf.h | 29 +++++++
kernel/bpf/bpf_struct_ops.c | 124 +++++++++++++++---------------
kernel/bpf/bpf_struct_ops_types.h | 12 ---
kernel/bpf/btf.c | 2 +-
net/bpf/bpf_dummy_struct_ops.c | 14 +++-
net/ipv4/bpf_tcp_ca.c | 16 +++-
7 files changed, 119 insertions(+), 80 deletions(-)
delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 1e1647c8b0ce..b0f33147aa93 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3207,4 +3207,6 @@ 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);
+
#endif /* _LINUX_BPF_H */
diff --git a/include/linux/btf.h b/include/linux/btf.h
index aa2ba77648be..fdc83aa10462 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 type *)0); \
+ ((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 */
@@ -200,6 +202,7 @@ u32 btf_obj_id(const struct btf *btf);
bool btf_is_kernel(const struct btf *btf);
bool btf_is_module(const struct btf *btf);
struct module *btf_try_get_module(const struct btf *btf);
+struct btf *btf_get_module_btf(const struct module *module);
u32 btf_nr_types(const struct btf *btf);
bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
const struct btf_member *m,
@@ -577,4 +580,30 @@ int btf_add_struct_ops(struct bpf_struct_ops *st_ops);
const struct bpf_struct_ops **
btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
+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;
+};
+#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common
+
+/* 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) \
+extern struct bpf_struct_ops bpf_##_name; \
+ \
+struct bpf_struct_ops_##_name { \
+ BPF_STRUCT_OPS_COMMON_VALUE; \
+ struct _name data ____cacheline_aligned_in_smp; \
+}
+
#endif
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 60445ff32275..175068b083cb 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -13,19 +13,6 @@
#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,
-};
-
-struct bpf_struct_ops_common_value {
- refcount_t refcnt;
- enum bpf_struct_ops_state state;
-};
-#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common
-
struct bpf_struct_ops_value {
BPF_STRUCT_OPS_COMMON_VALUE;
char data[] ____cacheline_aligned_in_smp;
@@ -72,35 +59,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 { \
- BPF_STRUCT_OPS_COMMON_VALUE; \
- 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 * const bpf_struct_ops[] = {
-#define BPF_STRUCT_OPS_TYPE(_name) \
- [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name,
-#include "bpf_struct_ops_types.h"
-#undef BPF_STRUCT_OPS_TYPE
-};
-
const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
};
@@ -234,16 +192,51 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
}
+static int register_bpf_struct_ops_btf(struct bpf_struct_ops *st_ops,
+ struct btf *btf)
+{
+ struct bpf_verifier_log *log;
+ int err;
+
+ if (st_ops == NULL)
+ return -EINVAL;
+
+ log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
+ if (!log) {
+ err = -ENOMEM;
+ goto errout;
+ }
+
+ log->level = BPF_LOG_KERNEL;
+
+ bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log);
+
+ err = btf_add_struct_ops(st_ops);
+
+errout:
+ kfree(log);
+
+ return err;
+}
+
+int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)
+{
+ struct btf *btf;
+ int err;
+
+ btf = btf_get_module_btf(st_ops->owner);
+ if (!btf)
+ return -EINVAL;
+ err = register_bpf_struct_ops_btf(st_ops, btf);
+ btf_put(btf);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
+
void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
{
- struct bpf_struct_ops *st_ops;
s32 module_id, common_value_id;
- 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
module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
if (module_id < 0) {
@@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
return;
}
common_value_type = btf_type_by_id(btf, common_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, NULL, log);
- }
}
extern struct btf *btf_vmlinux;
@@ -271,32 +259,44 @@ extern struct btf *btf_vmlinux;
static const struct bpf_struct_ops *
bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
{
+ const struct bpf_struct_ops *st_ops = NULL;
+ const struct bpf_struct_ops **st_ops_list;
unsigned int i;
+ u32 cnt = 0;
if (!value_id || !btf_vmlinux)
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) {
+ st_ops = st_ops_list[i];
+ break;
+ }
}
- return NULL;
+ return st_ops;
}
const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf, u32 type_id)
{
+ const struct bpf_struct_ops *st_ops = NULL;
+ const struct bpf_struct_ops **st_ops_list;
unsigned int i;
+ u32 cnt;
if (!type_id || !btf_vmlinux)
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) {
+ st_ops = st_ops_list[i];
+ break;
+ }
}
- return NULL;
+ return st_ops;
}
static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void *key,
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 be5144dbb53d..990973d6057d 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7532,7 +7532,7 @@ struct module *btf_try_get_module(const struct btf *btf)
/* Returns struct btf corresponding to the struct module.
* This function can return NULL or ERR_PTR.
*/
-static struct btf *btf_get_module_btf(const struct module *module)
+struct btf *btf_get_module_btf(const struct module *module)
{
#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
struct btf_module *btf_mod, *tmp;
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index 5918d1b32e19..724bb7224079 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, ...);
@@ -216,11 +216,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,
@@ -228,4 +230,12 @@ 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)
+{
+ BTF_STRUCT_OPS_TYPE_EMIT(bpf_dummy_ops);
+ return register_bpf_struct_ops(&bpf_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 39dcccf0f174..20c401c73dfb 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),
@@ -271,7 +271,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,
@@ -281,10 +283,18 @@ 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;
+
+ BTF_STRUCT_OPS_TYPE_EMIT(tcp_congestion_ops);
+
+ 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);
+
+ return ret;
}
late_initcall(bpf_tcp_ca_kfunc_init);
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration
2023-10-17 16:23 ` [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration thinker.li
@ 2023-10-19 1:49 ` Martin KaFai Lau
2023-10-20 15:12 ` Kui-Feng Lee
0 siblings, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-19 1:49 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, netdev, bpf, ast, song, kernel-team, andrii, drosen
On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
> From: Kui-Feng Lee <thinker.li@gmail.com>
>
> Replace the static list of struct_ops types with pre-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 | 2 +
> include/linux/btf.h | 29 +++++++
> kernel/bpf/bpf_struct_ops.c | 124 +++++++++++++++---------------
> kernel/bpf/bpf_struct_ops_types.h | 12 ---
> kernel/bpf/btf.c | 2 +-
> net/bpf/bpf_dummy_struct_ops.c | 14 +++-
> net/ipv4/bpf_tcp_ca.c | 16 +++-
> 7 files changed, 119 insertions(+), 80 deletions(-)
> delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 1e1647c8b0ce..b0f33147aa93 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -3207,4 +3207,6 @@ 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);
> +
> #endif /* _LINUX_BPF_H */
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index aa2ba77648be..fdc83aa10462 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 type *)0); \
> + ((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 */
> @@ -200,6 +202,7 @@ u32 btf_obj_id(const struct btf *btf);
> bool btf_is_kernel(const struct btf *btf);
> bool btf_is_module(const struct btf *btf);
> struct module *btf_try_get_module(const struct btf *btf);
> +struct btf *btf_get_module_btf(const struct module *module);
> u32 btf_nr_types(const struct btf *btf);
> bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
> const struct btf_member *m,
> @@ -577,4 +580,30 @@ int btf_add_struct_ops(struct bpf_struct_ops *st_ops);
> const struct bpf_struct_ops **
> btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
>
> +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;
> +};
> +#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common
Since there is 'struct bpf_struct_ops_common_value' now, the
BPF_STRUCT_OPS_COMMON_VALUE macro is not as useful as before. Lets remove it.
> +
> +/* 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) \
> +extern struct bpf_struct_ops bpf_##_name; \
> + \
> +struct bpf_struct_ops_##_name { \
> + BPF_STRUCT_OPS_COMMON_VALUE; \
> + struct _name data ____cacheline_aligned_in_smp; \
> +}
I think the bpp_struct_ops_* should not be in btf.h. Probably move them to bpf.h
instead. or there is some other considerations I am missing?
> +
> #endif
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 60445ff32275..175068b083cb 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -13,19 +13,6 @@
> #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,
> -};
> -
> -struct bpf_struct_ops_common_value {
> - refcount_t refcnt;
> - enum bpf_struct_ops_state state;
> -};
> -#define BPF_STRUCT_OPS_COMMON_VALUE struct bpf_struct_ops_common_value common
> -
> struct bpf_struct_ops_value {
> BPF_STRUCT_OPS_COMMON_VALUE;
> char data[] ____cacheline_aligned_in_smp;
> @@ -72,35 +59,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 { \
> - BPF_STRUCT_OPS_COMMON_VALUE; \
> - 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 * const bpf_struct_ops[] = {
> -#define BPF_STRUCT_OPS_TYPE(_name) \
> - [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name,
> -#include "bpf_struct_ops_types.h"
> -#undef BPF_STRUCT_OPS_TYPE
> -};
> -
> const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
> };
>
> @@ -234,16 +192,51 @@ static void bpf_struct_ops_init_one(struct bpf_struct_ops *st_ops,
>
> }
>
> +static int register_bpf_struct_ops_btf(struct bpf_struct_ops *st_ops,
> + struct btf *btf)
Please combine this function into register_bpf_struct_ops(). They are both very
short.
> +{
> + struct bpf_verifier_log *log;
> + int err;
> +
> + if (st_ops == NULL)
> + return -EINVAL;
> +
> + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
> + if (!log) {
> + err = -ENOMEM;
> + goto errout;
> + }
> +
> + log->level = BPF_LOG_KERNEL;
> +
> + bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log);
> +
> + err = btf_add_struct_ops(st_ops);
> +
> +errout:
> + kfree(log);
> +
> + return err;
> +}
> +
> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)
Similar to the register kfunc counterpart, can this be moved to btf.c instead by
extern-ing bpf_struct_ops_init_one()? or there are some other structs/functions
need to extern?
> +{
> + struct btf *btf;
> + int err;
> +
> + btf = btf_get_module_btf(st_ops->owner);
> + if (!btf)
> + return -EINVAL;
> + err = register_bpf_struct_ops_btf(st_ops, btf);
> + btf_put(btf);
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
> +
> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
The bpf_struct_ops_init() is pretty much only finding the btf "module_id" and
"common_value_id". Lets use the BTF_ID_LIST to do it instead. Then the newly
added bpf_struct_ops_init_one() could use a proper name bpf_struct_ops_init()
instead of having the special "_one" suffix.
> {
> - struct bpf_struct_ops *st_ops;
> s32 module_id, common_value_id;
> - 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
>
> module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
> if (module_id < 0) {
> @@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
> return;
> }
> common_value_type = btf_type_by_id(btf, common_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, NULL, log);
> - }
> }
>
> extern struct btf *btf_vmlinux;
> @@ -271,32 +259,44 @@ extern struct btf *btf_vmlinux;
> static const struct bpf_struct_ops *
> bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
> {
> + const struct bpf_struct_ops *st_ops = NULL;
> + const struct bpf_struct_ops **st_ops_list;
> unsigned int i;
> + u32 cnt = 0;
>
> if (!value_id || !btf_vmlinux)
The "!btf_vmlinux" should have been changed to "!btf" in the earlier patch
(patch 2?),
and is this null check still needed now?
> 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) {
> + st_ops = st_ops_list[i];
nit. Like the change in the earlier patch that is being replaced here,
directly "return st_ops_list[i];".
> + break;
> + }
> }
>
> - return NULL;
> + return st_ops;
> }
>
> const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf, u32 type_id)
> {
> + const struct bpf_struct_ops *st_ops = NULL;
> + const struct bpf_struct_ops **st_ops_list;
> unsigned int i;
> + u32 cnt;
>
> if (!type_id || !btf_vmlinux)
> 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) {
> + st_ops = st_ops_list[i];
Same.
> + break;
> + }
> }
>
> - return NULL;
> + return st_ops;
> }
>
> static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void *key,
> 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
Seeing this gone is satisfying.
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index be5144dbb53d..990973d6057d 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -7532,7 +7532,7 @@ struct module *btf_try_get_module(const struct btf *btf)
> /* Returns struct btf corresponding to the struct module.
> * This function can return NULL or ERR_PTR.
> */
> -static struct btf *btf_get_module_btf(const struct module *module)
> +struct btf *btf_get_module_btf(const struct module *module)
> {
> #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> struct btf_module *btf_mod, *tmp;
> diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
> index 5918d1b32e19..724bb7224079 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;
Is it still needed ?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration
2023-10-19 1:49 ` Martin KaFai Lau
@ 2023-10-20 15:12 ` Kui-Feng Lee
2023-10-20 17:53 ` Kui-Feng Lee
0 siblings, 1 reply; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-20 15:12 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, netdev, bpf, ast, song, kernel-team, andrii, drosen
On 10/18/23 18:49, Martin KaFai Lau wrote:
> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>
>> Replace the static list of struct_ops types with pre-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 | 2 +
>> include/linux/btf.h | 29 +++++++
>> kernel/bpf/bpf_struct_ops.c | 124 +++++++++++++++---------------
>> kernel/bpf/bpf_struct_ops_types.h | 12 ---
>> kernel/bpf/btf.c | 2 +-
>> net/bpf/bpf_dummy_struct_ops.c | 14 +++-
>> net/ipv4/bpf_tcp_ca.c | 16 +++-
>> 7 files changed, 119 insertions(+), 80 deletions(-)
>> delete mode 100644 kernel/bpf/bpf_struct_ops_types.h
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 1e1647c8b0ce..b0f33147aa93 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -3207,4 +3207,6 @@ 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);
>> +
>> #endif /* _LINUX_BPF_H */
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index aa2ba77648be..fdc83aa10462 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 type *)0); \
>> + ((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 */
>> @@ -200,6 +202,7 @@ u32 btf_obj_id(const struct btf *btf);
>> bool btf_is_kernel(const struct btf *btf);
>> bool btf_is_module(const struct btf *btf);
>> struct module *btf_try_get_module(const struct btf *btf);
>> +struct btf *btf_get_module_btf(const struct module *module);
>> u32 btf_nr_types(const struct btf *btf);
>> bool btf_member_is_reg_int(const struct btf *btf, const struct
>> btf_type *s,
>> const struct btf_member *m,
>> @@ -577,4 +580,30 @@ int btf_add_struct_ops(struct bpf_struct_ops
>> *st_ops);
>> const struct bpf_struct_ops **
>> btf_get_struct_ops(struct btf *btf, u32 *ret_cnt);
>> +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;
>> +};
>> +#define BPF_STRUCT_OPS_COMMON_VALUE struct
>> bpf_struct_ops_common_value common
>
> Since there is 'struct bpf_struct_ops_common_value' now, the
> BPF_STRUCT_OPS_COMMON_VALUE macro is not as useful as before. Lets
> remove it.
Agree
>
>> +
>> +/* 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) \
>> +extern struct bpf_struct_ops bpf_##_name; \
>> + \
>> +struct bpf_struct_ops_##_name { \
>> + BPF_STRUCT_OPS_COMMON_VALUE; \
>> + struct _name data ____cacheline_aligned_in_smp; \
>> +}
>
> I think the bpp_struct_ops_* should not be in btf.h. Probably move them
> to bpf.h instead. or there is some other considerations I am missing?
Yes, I think bpf.h is the right place.
>
>> +
>> #endif
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 60445ff32275..175068b083cb 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -13,19 +13,6 @@
>> #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,
>> -};
>> -
>> -struct bpf_struct_ops_common_value {
>> - refcount_t refcnt;
>> - enum bpf_struct_ops_state state;
>> -};
>> -#define BPF_STRUCT_OPS_COMMON_VALUE struct
>> bpf_struct_ops_common_value common
>> -
>> struct bpf_struct_ops_value {
>> BPF_STRUCT_OPS_COMMON_VALUE;
>> char data[] ____cacheline_aligned_in_smp;
>> @@ -72,35 +59,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 { \
>> - BPF_STRUCT_OPS_COMMON_VALUE; \
>> - 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 * const bpf_struct_ops[] = {
>> -#define BPF_STRUCT_OPS_TYPE(_name) \
>> - [BPF_STRUCT_OPS_TYPE_##_name] = &bpf_##_name,
>> -#include "bpf_struct_ops_types.h"
>> -#undef BPF_STRUCT_OPS_TYPE
>> -};
>> -
>> const struct bpf_verifier_ops bpf_struct_ops_verifier_ops = {
>> };
>> @@ -234,16 +192,51 @@ static void bpf_struct_ops_init_one(struct
>> bpf_struct_ops *st_ops,
>> }
>> +static int register_bpf_struct_ops_btf(struct bpf_struct_ops *st_ops,
>> + struct btf *btf)
>
> Please combine this function into register_bpf_struct_ops(). They are
> both very short.
>
Got it!
>> +{
>> + struct bpf_verifier_log *log;
>> + int err;
>> +
>> + if (st_ops == NULL)
>> + return -EINVAL;
>> +
>> + log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
>> + if (!log) {
>> + err = -ENOMEM;
>> + goto errout;
>> + }
>> +
>> + log->level = BPF_LOG_KERNEL;
>> +
>> + bpf_struct_ops_init_one(st_ops, btf, st_ops->owner, log);
>> +
>> + err = btf_add_struct_ops(st_ops);
>> +
>> +errout:
>> + kfree(log);
>> +
>> + return err;
>> +}
>> +
>> +int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)
>
> Similar to the register kfunc counterpart, can this be moved to btf.c
> instead by extern-ing bpf_struct_ops_init_one()? or there are some other
> structs/functions need to extern?
It is wierd to move a function of bpf_struct_ops to btf.
But, kfunc already did that, I don't mind to follow it.
>
>> +{
>> + struct btf *btf;
>> + int err;
>> +
>> + btf = btf_get_module_btf(st_ops->owner);
>> + if (!btf)
>> + return -EINVAL;
>> + err = register_bpf_struct_ops_btf(st_ops, btf);
>> + btf_put(btf);
>> +
>> + return err;
>> +}
>> +EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
>> +
>> void bpf_struct_ops_init(struct btf *btf, struct bpf_verifier_log *log)
>
> The bpf_struct_ops_init() is pretty much only finding the btf
> "module_id" and "common_value_id". Lets use the BTF_ID_LIST to do it
> instead. Then the newly added bpf_struct_ops_init_one() could use a
> proper name bpf_struct_ops_init() instead of having the special "_one"
> suffix.
Got it!
>
>> {
>> - struct bpf_struct_ops *st_ops;
>> s32 module_id, common_value_id;
>> - 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
>> module_id = btf_find_by_name_kind(btf, "module", BTF_KIND_STRUCT);
>> if (module_id < 0) {
>> @@ -259,11 +252,6 @@ void bpf_struct_ops_init(struct btf *btf, struct
>> bpf_verifier_log *log)
>> return;
>> }
>> common_value_type = btf_type_by_id(btf, common_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, NULL, log);
>> - }
>> }
>> extern struct btf *btf_vmlinux;
>> @@ -271,32 +259,44 @@ extern struct btf *btf_vmlinux;
>> static const struct bpf_struct_ops *
>> bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
>> {
>> + const struct bpf_struct_ops *st_ops = NULL;
>> + const struct bpf_struct_ops **st_ops_list;
>> unsigned int i;
>> + u32 cnt = 0;
>> if (!value_id || !btf_vmlinux)
>
> The "!btf_vmlinux" should have been changed to "!btf" in the earlier
> patch (patch 2?),
This is not btf. It mean to check if btf_vmlinux is initialized.
It is not necessary anymore.
For checking btf, the following btf_get_struct_ops() will keep cnt zero
if btf is NULL, so it is unnecessary as well.
>
> and is this null check still needed now?
>
>> 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) {
>> + st_ops = st_ops_list[i];
>
> nit. Like the change in the earlier patch that is being replaced here,
> directly "return st_ops_list[i];".
Got it!
>
>> + break;
>> + }
>> }
>> - return NULL;
>> + return st_ops;
>> }
>> const struct bpf_struct_ops *bpf_struct_ops_find(struct btf *btf,
>> u32 type_id)
>> {
>> + const struct bpf_struct_ops *st_ops = NULL;
>> + const struct bpf_struct_ops **st_ops_list;
>> unsigned int i;
>> + u32 cnt;
>> if (!type_id || !btf_vmlinux)
>> 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) {
>> + st_ops = st_ops_list[i];
>
> Same.
Ack!
>
>> + break;
>> + }
>> }
>> - return NULL;
>> + return st_ops;
>> }
>> static int bpf_struct_ops_map_get_next_key(struct bpf_map *map, void
>> *key,
>> 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
>
> Seeing this gone is satisfying
>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index be5144dbb53d..990973d6057d 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -7532,7 +7532,7 @@ struct module *btf_try_get_module(const struct
>> btf *btf)
>> /* Returns struct btf corresponding to the struct module.
>> * This function can return NULL or ERR_PTR.
>> */
>> -static struct btf *btf_get_module_btf(const struct module *module)
>> +struct btf *btf_get_module_btf(const struct module *module)
>> {
>> #ifdef CONFIG_DEBUG_INFO_BTF_MODULES
>> struct btf_module *btf_mod, *tmp;
>> diff --git a/net/bpf/bpf_dummy_struct_ops.c
>> b/net/bpf/bpf_dummy_struct_ops.c
>> index 5918d1b32e19..724bb7224079 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;
>
> Is it still needed ?
Yes, it will be used by bpf_struct_ops_test_run().
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration
2023-10-20 15:12 ` Kui-Feng Lee
@ 2023-10-20 17:53 ` Kui-Feng Lee
0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-20 17:53 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, netdev, bpf, ast, song, kernel-team, andrii, drosen
On 10/20/23 08:12, Kui-Feng Lee wrote:
>
>
> On 10/18/23 18:49, Martin KaFai Lau wrote:
>> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>>> From: Kui-Feng Lee <thinker.li@gmail.com>
>>> static const struct bpf_struct_ops *
>>> bpf_struct_ops_find_value(struct btf *btf, u32 value_id)
>>> {
>>> + const struct bpf_struct_ops *st_ops = NULL;
>>> + const struct bpf_struct_ops **st_ops_list;
>>> unsigned int i;
>>> + u32 cnt = 0;
>>> if (!value_id || !btf_vmlinux)
>>
>> The "!btf_vmlinux" should have been changed to "!btf" in the earlier
>> patch (patch 2?),
>
> This is not btf. It mean to check if btf_vmlinux is initialized.
> It is not necessary anymore.
> For checking btf, the following btf_get_struct_ops() will keep cnt zero
> if btf is NULL, so it is unnecessary as well.
Forget my previous comment. I think you are right!
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs.
2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
` (5 preceding siblings ...)
2023-10-17 16:23 ` [PATCH bpf-next v5 6/9] bpf, net: switch to dynamic registration thinker.li
@ 2023-10-17 16:23 ` thinker.li
2023-10-17 21:49 ` Andrii Nakryiko
2023-10-19 2:43 ` Martin KaFai Lau
2023-10-17 16:23 ` [PATCH bpf-next v5 8/9] bpf: export btf_ctx_access to modules thinker.li
2023-10-17 16:23 ` [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
8 siblings, 2 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 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, mod_btf is added to bpf_map to keep a
reference to the module BTF. The FD of the module BTF is passed to the
kernel as mod_btf_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>
---
tools/lib/bpf/bpf.c | 4 ++-
tools/lib/bpf/bpf.h | 5 +++-
tools/lib/bpf/libbpf.c | 68 +++++++++++++++++++++++++++---------------
3 files changed, 51 insertions(+), 26 deletions(-)
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index b0f1913763a3..af46488e4ea9 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_extra);
+ 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 74c2887cfd24..1733cdc21241 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -51,8 +51,11 @@ struct bpf_map_create_opts {
__u32 numa_node;
__u32 map_ifindex;
+
+ __u32 value_type_btf_obj_fd;
+ size_t:0;
};
-#define bpf_map_create_opts__last_field map_ifindex
+#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 3a6108e3238b..d8a60fb52f5c 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -519,6 +519,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;
@@ -893,6 +894,8 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
return 0;
}
+static int load_module_btfs(struct bpf_object *obj);
+
static const struct btf_member *
find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
{
@@ -922,22 +925,29 @@ find_member_by_name(const struct btf *btf, const struct btf_type *t,
return NULL;
}
+static int find_module_btf_id(struct bpf_object *obj, const char *kern_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_module_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);
@@ -991,14 +1001,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;
@@ -1006,16 +1018,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;
@@ -1091,6 +1106,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;
@@ -1133,8 +1150,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;
}
@@ -5193,8 +5209,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);
@@ -7700,9 +7718,9 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
return libbpf_kallsyms_parse(kallsyms_cb, obj);
}
-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)
+static int find_module_btf_id(struct bpf_object *obj, const char *kern_name,
+ __u16 kind, struct btf **res_btf,
+ struct module_btf **res_mod_btf)
{
struct module_btf *mod_btf;
struct btf *btf;
@@ -7710,7 +7728,7 @@ static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
btf = obj->btf_vmlinux;
mod_btf = NULL;
- id = btf__find_by_name_kind(btf, ksym_name, kind);
+ id = btf__find_by_name_kind(btf, kern_name, kind);
if (id == -ENOENT) {
err = load_module_btfs(obj);
@@ -7721,7 +7739,7 @@ static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
/* we assume module_btf's BTF FD is always >0 */
mod_btf = &obj->btf_modules[i];
btf = mod_btf->btf;
- id = btf__find_by_name_kind_own(btf, ksym_name, kind);
+ id = btf__find_by_name_kind_own(btf, kern_name, kind);
if (id != -ENOENT)
break;
}
@@ -7744,7 +7762,7 @@ static int bpf_object__resolve_ksym_var_btf_id(struct bpf_object *obj,
struct btf *btf = NULL;
int id, err;
- id = find_ksym_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
+ id = find_module_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
if (id < 0) {
if (id == -ESRCH && ext->is_weak)
return 0;
@@ -7798,8 +7816,8 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
local_func_proto_id = ext->ksym.type_id;
- kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
- &mod_btf);
+ kfunc_id = find_module_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
+ &mod_btf);
if (kfunc_id < 0) {
if (kfunc_id == -ESRCH && ext->is_weak)
return 0;
@@ -9464,9 +9482,9 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
return err;
}
-static int find_kernel_btf_id(struct bpf_object *obj, const char *attach_name,
- enum bpf_attach_type attach_type,
- int *btf_obj_fd, int *btf_type_id)
+static int find_kernel_attach_btf_id(struct bpf_object *obj, const char *attach_name,
+ enum bpf_attach_type attach_type,
+ int *btf_obj_fd, int *btf_type_id)
{
int ret, i;
@@ -9531,7 +9549,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_attach_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",
@@ -12945,9 +12965,9 @@ int bpf_program__set_attach_target(struct bpf_program *prog,
err = bpf_object__load_vmlinux_btf(prog->obj, true);
if (err)
return libbpf_err(err);
- err = find_kernel_btf_id(prog->obj, attach_func_name,
- prog->expected_attach_type,
- &btf_obj_fd, &btf_id);
+ err = find_kernel_attach_btf_id(prog->obj, attach_func_name,
+ prog->expected_attach_type,
+ &btf_obj_fd, &btf_id);
if (err)
return libbpf_err(err);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs.
2023-10-17 16:23 ` [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
@ 2023-10-17 21:49 ` Andrii Nakryiko
2023-10-18 2:25 ` Kui-Feng Lee
2023-10-19 2:43 ` Martin KaFai Lau
1 sibling, 1 reply; 27+ messages in thread
From: Andrii Nakryiko @ 2023-10-17 21:49 UTC (permalink / raw)
To: thinker.li
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, drosen,
sinquersw, kuifeng
On Tue, Oct 17, 2023 at 9:23 AM <thinker.li@gmail.com> wrote:
>
> 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, mod_btf is added to bpf_map to keep a
> reference to the module BTF. The FD of the module BTF is passed to the
> kernel as mod_btf_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>
> ---
> tools/lib/bpf/bpf.c | 4 ++-
> tools/lib/bpf/bpf.h | 5 +++-
> tools/lib/bpf/libbpf.c | 68 +++++++++++++++++++++++++++---------------
> 3 files changed, 51 insertions(+), 26 deletions(-)
>
I have a few nits, please accommodate them, and with that please add
my ack on libbpf side of things
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index b0f1913763a3..af46488e4ea9 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_extra);
> + 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 74c2887cfd24..1733cdc21241 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -51,8 +51,11 @@ struct bpf_map_create_opts {
>
> __u32 numa_node;
> __u32 map_ifindex;
> +
> + __u32 value_type_btf_obj_fd;
> + size_t:0;
> };
> -#define bpf_map_create_opts__last_field map_ifindex
> +#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 3a6108e3238b..d8a60fb52f5c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -519,6 +519,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;
> @@ -893,6 +894,8 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
> return 0;
> }
>
> +static int load_module_btfs(struct bpf_object *obj);
> +
you don't need this forward declaration, do you?
> static const struct btf_member *
> find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
> {
> @@ -922,22 +925,29 @@ find_member_by_name(const struct btf *btf, const struct btf_type *t,
> return NULL;
> }
>
[...]
> if (obj->btf && btf__fd(obj->btf) >= 0) {
> create_attr.btf_fd = btf__fd(obj->btf);
> @@ -7700,9 +7718,9 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
> return libbpf_kallsyms_parse(kallsyms_cb, obj);
> }
>
> -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)
> +static int find_module_btf_id(struct bpf_object *obj, const char *kern_name,
> + __u16 kind, struct btf **res_btf,
> + struct module_btf **res_mod_btf)
I actually find "find_module" terminology confusing, because it might
not be in the module after all, right? I think "find_ksym_btf_id" is a
totally appropriate name, and it's just that in some cases that kernel
symbol (ksym) will be found in the kernel module instead of in vmlinux
image itself. Still, it's a kernel. Let's keep the name?
> {
> struct module_btf *mod_btf;
> struct btf *btf;
[...]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs.
2023-10-17 21:49 ` Andrii Nakryiko
@ 2023-10-18 2:25 ` Kui-Feng Lee
0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-18 2:25 UTC (permalink / raw)
To: Andrii Nakryiko, thinker.li
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, drosen, kuifeng
On 10/17/23 14:49, Andrii Nakryiko wrote:
> On Tue, Oct 17, 2023 at 9:23 AM <thinker.li@gmail.com> wrote:
>>
>> 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, mod_btf is added to bpf_map to keep a
>> reference to the module BTF. The FD of the module BTF is passed to the
>> kernel as mod_btf_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>
>> ---
>> tools/lib/bpf/bpf.c | 4 ++-
>> tools/lib/bpf/bpf.h | 5 +++-
>> tools/lib/bpf/libbpf.c | 68 +++++++++++++++++++++++++++---------------
>> 3 files changed, 51 insertions(+), 26 deletions(-)
>>
>
> I have a few nits, please accommodate them, and with that please add
> my ack on libbpf side of things
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
Thanks for reviewing!
>
>> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
>> index b0f1913763a3..af46488e4ea9 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_extra);
>> + 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 74c2887cfd24..1733cdc21241 100644
>> --- a/tools/lib/bpf/bpf.h
>> +++ b/tools/lib/bpf/bpf.h
>> @@ -51,8 +51,11 @@ struct bpf_map_create_opts {
>>
>> __u32 numa_node;
>> __u32 map_ifindex;
>> +
>> + __u32 value_type_btf_obj_fd;
>> + size_t:0;
>> };
>> -#define bpf_map_create_opts__last_field map_ifindex
>> +#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 3a6108e3238b..d8a60fb52f5c 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -519,6 +519,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;
>> @@ -893,6 +894,8 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
>> return 0;
>> }
>>
>> +static int load_module_btfs(struct bpf_object *obj);
>> +
>
> you don't need this forward declaration, do you?
I will remove it.
>
>> static const struct btf_member *
>> find_member_by_offset(const struct btf_type *t, __u32 bit_offset)
>> {
>> @@ -922,22 +925,29 @@ find_member_by_name(const struct btf *btf, const struct btf_type *t,
>> return NULL;
>> }
>>
>
> [...]
>
>> if (obj->btf && btf__fd(obj->btf) >= 0) {
>> create_attr.btf_fd = btf__fd(obj->btf);
>> @@ -7700,9 +7718,9 @@ static int bpf_object__read_kallsyms_file(struct bpf_object *obj)
>> return libbpf_kallsyms_parse(kallsyms_cb, obj);
>> }
>>
>> -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)
>> +static int find_module_btf_id(struct bpf_object *obj, const char *kern_name,
>> + __u16 kind, struct btf **res_btf,
>> + struct module_btf **res_mod_btf)
>
> I actually find "find_module" terminology confusing, because it might
> not be in the module after all, right? I think "find_ksym_btf_id" is a
> totally appropriate name, and it's just that in some cases that kernel
> symbol (ksym) will be found in the kernel module instead of in vmlinux
> image itself. Still, it's a kernel. Let's keep the name?
Agree!
>
>> {
>> struct module_btf *mod_btf;
>> struct btf *btf;
>
> [...]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs.
2023-10-17 16:23 ` [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
2023-10-17 21:49 ` Andrii Nakryiko
@ 2023-10-19 2:43 ` Martin KaFai Lau
2023-10-19 16:31 ` Kui-Feng Lee
1 sibling, 1 reply; 27+ messages in thread
From: Martin KaFai Lau @ 2023-10-19 2:43 UTC (permalink / raw)
To: thinker.li
Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
> -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)
> +static int find_module_btf_id(struct bpf_object *obj, const char *kern_name,
> + __u16 kind, struct btf **res_btf,
> + struct module_btf **res_mod_btf)
> {
> struct module_btf *mod_btf;
> struct btf *btf;
> @@ -7710,7 +7728,7 @@ static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
>
> btf = obj->btf_vmlinux;
> mod_btf = NULL;
> - id = btf__find_by_name_kind(btf, ksym_name, kind);
> + id = btf__find_by_name_kind(btf, kern_name, kind);
>
> if (id == -ENOENT) {
> err = load_module_btfs(obj);
> @@ -7721,7 +7739,7 @@ static int find_ksym_btf_id(struct bpf_object *obj, const char *ksym_name,
> /* we assume module_btf's BTF FD is always >0 */
> mod_btf = &obj->btf_modules[i];
> btf = mod_btf->btf;
> - id = btf__find_by_name_kind_own(btf, ksym_name, kind);
> + id = btf__find_by_name_kind_own(btf, kern_name, kind);
> if (id != -ENOENT)
> break;
> }
> @@ -7744,7 +7762,7 @@ static int bpf_object__resolve_ksym_var_btf_id(struct bpf_object *obj,
> struct btf *btf = NULL;
> int id, err;
>
> - id = find_ksym_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
> + id = find_module_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
> if (id < 0) {
> if (id == -ESRCH && ext->is_weak)
> return 0;
> @@ -7798,8 +7816,8 @@ static int bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
>
> local_func_proto_id = ext->ksym.type_id;
>
> - kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
> - &mod_btf);
> + kfunc_id = find_module_btf_id(obj, ext->essent_name ?: ext->name, BTF_KIND_FUNC, &kern_btf,
> + &mod_btf);
> if (kfunc_id < 0) {
> if (kfunc_id == -ESRCH && ext->is_weak)
> return 0;
> @@ -9464,9 +9482,9 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> return err;
> }
>
> -static int find_kernel_btf_id(struct bpf_object *obj, const char *attach_name,
> - enum bpf_attach_type attach_type,
> - int *btf_obj_fd, int *btf_type_id)
> +static int find_kernel_attach_btf_id(struct bpf_object *obj, const char *attach_name,
> + enum bpf_attach_type attach_type,
> + int *btf_obj_fd, int *btf_type_id)
> {
> int ret, i;
>
> @@ -9531,7 +9549,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_attach_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",
> @@ -12945,9 +12965,9 @@ int bpf_program__set_attach_target(struct bpf_program *prog,
> err = bpf_object__load_vmlinux_btf(prog->obj, true);
> if (err)
> return libbpf_err(err);
> - err = find_kernel_btf_id(prog->obj, attach_func_name,
> - prog->expected_attach_type,
> - &btf_obj_fd, &btf_id);
> + err = find_kernel_attach_btf_id(prog->obj, attach_func_name,
> + prog->expected_attach_type,
> + &btf_obj_fd, &btf_id);
Please avoid mixing this level of name changes with the main changes. It is
quite confusing for the reviewer and it is not mentioned in the commit message
either.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs.
2023-10-19 2:43 ` Martin KaFai Lau
@ 2023-10-19 16:31 ` Kui-Feng Lee
0 siblings, 0 replies; 27+ messages in thread
From: Kui-Feng Lee @ 2023-10-19 16:31 UTC (permalink / raw)
To: Martin KaFai Lau, thinker.li
Cc: kuifeng, bpf, ast, song, kernel-team, andrii, drosen
On 10/18/23 19:43, Martin KaFai Lau wrote:
> On 10/17/23 9:23 AM, thinker.li@gmail.com wrote:
>> -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)
>> +static int find_module_btf_id(struct bpf_object *obj, const char
>> *kern_name,
>> + __u16 kind, struct btf **res_btf,
>> + struct module_btf **res_mod_btf)
>> {
>> struct module_btf *mod_btf;
>> struct btf *btf;
>> @@ -7710,7 +7728,7 @@ static int find_ksym_btf_id(struct bpf_object
>> *obj, const char *ksym_name,
>> btf = obj->btf_vmlinux;
>> mod_btf = NULL;
>> - id = btf__find_by_name_kind(btf, ksym_name, kind);
>> + id = btf__find_by_name_kind(btf, kern_name, kind);
>> if (id == -ENOENT) {
>> err = load_module_btfs(obj);
>> @@ -7721,7 +7739,7 @@ static int find_ksym_btf_id(struct bpf_object
>> *obj, const char *ksym_name,
>> /* we assume module_btf's BTF FD is always >0 */
>> mod_btf = &obj->btf_modules[i];
>> btf = mod_btf->btf;
>> - id = btf__find_by_name_kind_own(btf, ksym_name, kind);
>> + id = btf__find_by_name_kind_own(btf, kern_name, kind);
>> if (id != -ENOENT)
>> break;
>> }
>> @@ -7744,7 +7762,7 @@ static int
>> bpf_object__resolve_ksym_var_btf_id(struct bpf_object *obj,
>> struct btf *btf = NULL;
>> int id, err;
>> - id = find_ksym_btf_id(obj, ext->name, BTF_KIND_VAR, &btf, &mod_btf);
>> + id = find_module_btf_id(obj, ext->name, BTF_KIND_VAR, &btf,
>> &mod_btf);
>> if (id < 0) {
>> if (id == -ESRCH && ext->is_weak)
>> return 0;
>> @@ -7798,8 +7816,8 @@ static int
>> bpf_object__resolve_ksym_func_btf_id(struct bpf_object *obj,
>> local_func_proto_id = ext->ksym.type_id;
>> - kfunc_id = find_ksym_btf_id(obj, ext->essent_name ?: ext->name,
>> BTF_KIND_FUNC, &kern_btf,
>> - &mod_btf);
>> + kfunc_id = find_module_btf_id(obj, ext->essent_name ?: ext->name,
>> BTF_KIND_FUNC, &kern_btf,
>> + &mod_btf);
>> if (kfunc_id < 0) {
>> if (kfunc_id == -ESRCH && ext->is_weak)
>> return 0;
>> @@ -9464,9 +9482,9 @@ static int libbpf_find_prog_btf_id(const char
>> *name, __u32 attach_prog_fd)
>> return err;
>> }
>> -static int find_kernel_btf_id(struct bpf_object *obj, const char
>> *attach_name,
>> - enum bpf_attach_type attach_type,
>> - int *btf_obj_fd, int *btf_type_id)
>> +static int find_kernel_attach_btf_id(struct bpf_object *obj, const
>> char *attach_name,
>> + enum bpf_attach_type attach_type,
>> + int *btf_obj_fd, int *btf_type_id)
>> {
>> int ret, i;
>> @@ -9531,7 +9549,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_attach_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",
>> @@ -12945,9 +12965,9 @@ int bpf_program__set_attach_target(struct
>> bpf_program *prog,
>> err = bpf_object__load_vmlinux_btf(prog->obj, true);
>> if (err)
>> return libbpf_err(err);
>> - err = find_kernel_btf_id(prog->obj, attach_func_name,
>> - prog->expected_attach_type,
>> - &btf_obj_fd, &btf_id);
>> + err = find_kernel_attach_btf_id(prog->obj, attach_func_name,
>> + prog->expected_attach_type,
>> + &btf_obj_fd, &btf_id);
>
> Please avoid mixing this level of name changes with the main changes. It
> is quite confusing for the reviewer and it is not mentioned in the
> commit message either.
Got it! Sorry confusing.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH bpf-next v5 8/9] bpf: export btf_ctx_access to modules.
2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
` (6 preceding siblings ...)
2023-10-17 16:23 ` [PATCH bpf-next v5 7/9] libbpf: Find correct module BTFs for struct_ops maps and progs thinker.li
@ 2023-10-17 16:23 ` thinker.li
2023-10-17 16:23 ` [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
8 siblings, 0 replies; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 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 990973d6057d..dc6d8fe9b63d 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6141,6 +6141,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] 27+ messages in thread
* [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops().
2023-10-17 16:22 [PATCH bpf-next v5 0/9] Registrating struct_ops types from modules thinker.li
` (7 preceding siblings ...)
2023-10-17 16:23 ` [PATCH bpf-next v5 8/9] bpf: export btf_ctx_access to modules thinker.li
@ 2023-10-17 16:23 ` thinker.li
2023-10-17 18:03 ` kernel test robot
8 siblings, 1 reply; 27+ messages in thread
From: thinker.li @ 2023-10-17 16:23 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>
---
tools/testing/selftests/bpf/Makefile | 2 +
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 59 +++++++++++++++++++
.../selftests/bpf/bpf_testmod/bpf_testmod.h | 5 ++
.../bpf/prog_tests/test_struct_ops_module.c | 38 ++++++++++++
.../selftests/bpf/progs/struct_ops_module.c | 30 ++++++++++
tools/testing/selftests/bpf/testing_helpers.c | 35 +++++++++++
6 files changed, 169 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
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index caede9b574cb..dd7ff14e1fdf 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -706,6 +706,8 @@ $(OUTPUT)/uprobe_multi: uprobe_multi.c
$(call msg,BINARY,,$@)
$(Q)$(CC) $(CFLAGS) $(LDFLAGS) $^ $(LDLIBS) -o $@
+$(OUTPUT)/testing_helpers.o: $(OUTPUT)/rcu_tasks_trace_gp.skel.h
+
EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR) \
prog_tests/tests.h map_tests/tests.h verifier/tests.h \
feature bpftool \
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index cefc5dd72573..f1a20669d884 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>
@@ -517,11 +518,66 @@ 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)
+#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
+
+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;
+
+ BTF_STRUCT_OPS_TYPE_EMIT(bpf_testmod_ops);
+ 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,
+};
+
+#endif /* CONFIG_DEBUG_INFO_BTF_MODULES */
+
extern int bpf_fentry_test1(int a);
static int bpf_testmod_init(void)
@@ -532,6 +588,9 @@ 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);
+#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
+ ret = ret ?: register_bpf_struct_ops(&bpf_bpf_testmod_ops);
+#endif
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..7261fc6c377a
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <time.h>
+
+#include "rcu_tasks_trace_gp.skel.h"
+#include "struct_ops_module.skel.h"
+
+static void test_regular_load(void)
+{
+ struct struct_ops_module *skel;
+ struct bpf_link *link;
+ DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts);
+ int err;
+
+ 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"))
+ return;
+
+ link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+ ASSERT_OK_PTR(link, "attach_test_mod_1");
+
+ ASSERT_EQ(skel->bss->test_2_result, 7, "test_2_result");
+
+ bpf_link__destroy(link);
+
+ struct_ops_module__destroy(skel);
+}
+
+void serial_test_struct_ops_module(void)
+{
+ if (test__start_subtest("regular_load"))
+ test_regular_load();
+}
+
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/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 8d994884c7b4..05870cd62458 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -10,6 +10,7 @@
#include "test_progs.h"
#include "testing_helpers.h"
#include <linux/membarrier.h>
+#include "rcu_tasks_trace_gp.skel.h"
int parse_num_list(const char *s, bool **num_set, int *num_set_len)
{
@@ -380,10 +381,44 @@ int load_bpf_testmod(bool verbose)
return 0;
}
+/* This function will trigger call_rcu_tasks_trace() in the kernel */
+static int kern_sync_rcu_tasks_trace(void)
+{
+ struct rcu_tasks_trace_gp *rcu;
+ time_t start;
+ long gp_seq;
+ LIBBPF_OPTS(bpf_test_run_opts, opts);
+
+ rcu = rcu_tasks_trace_gp__open_and_load();
+ if (IS_ERR(rcu))
+ return -EFAULT;
+ if (rcu_tasks_trace_gp__attach(rcu))
+ return -EFAULT;
+
+ gp_seq = READ_ONCE(rcu->bss->gp_seq);
+
+ if (bpf_prog_test_run_opts(bpf_program__fd(rcu->progs.do_call_rcu_tasks_trace),
+ &opts))
+ return -EFAULT;
+ if (opts.retval != 0)
+ return -EFAULT;
+
+ start = time(NULL);
+ while ((start + 2) > time(NULL) &&
+ gp_seq == READ_ONCE(rcu->bss->gp_seq))
+ sched_yield();
+
+ rcu_tasks_trace_gp__destroy(rcu);
+
+ return 0;
+}
+
/*
* Trigger synchronize_rcu() in kernel.
*/
int kern_sync_rcu(void)
{
+ if (kern_sync_rcu_tasks_trace())
+ return -EFAULT;
return syscall(__NR_membarrier, MEMBARRIER_CMD_SHARED, 0, 0);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops().
2023-10-17 16:23 ` [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops() thinker.li
@ 2023-10-17 18:03 ` kernel test robot
0 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2023-10-17 18:03 UTC (permalink / raw)
To: thinker.li, bpf, ast, martin.lau, song, kernel-team, andrii, drosen
Cc: oe-kbuild-all
Hi,
kernel test robot noticed the following build warnings:
[auto build test WARNING on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/thinker-li-gmail-com/bpf-refactory-struct_ops-type-initialization-to-a-function/20231018-002613
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20231017162306.176586-10-thinker.li%40gmail.com
patch subject: [PATCH bpf-next v5 9/9] selftests/bpf: test case for register_bpf_struct_ops().
reproduce: (https://download.01.org/0day-ci/archive/20231018/202310180128.xAuYnY3h-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310180128.xAuYnY3h-lkp@intel.com/
# many are suggestions rather than must-fix
WARNING:LINE_SPACING: Missing a blank line after declarations
#239: FILE: tools/testing/selftests/bpf/testing_helpers.c:390:
+ long gp_seq;
+ LIBBPF_OPTS(bpf_test_run_opts, opts);
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 27+ messages in thread