| Message ID | 20230913061449.1918219-2-thinker.li@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Registrating struct_ops types from modules | expand |
On 9/12/23 11:14 PM, thinker.li@gmail.com wrote: > From: Kui-Feng Lee <thinker.li@gmail.com> > > Move most of code to bpf_struct_ops_init_one() that can be use to > initialize new struct_ops types registered dynamically. While in RFC, still better to have SOB so that it won't be overlooked in the future. > --- > 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 fdc3e8705a3c..1662875e0ebe 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); It needs to do some sanity checks on the value_type since this won't be statically enforced by bpf_struct_ops.c. [ ... ] > +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 Can this static way of defining struct_ops be removed? bpf_tcp_ca should be able to use the register_bpf_struct_ops() introduced in patch 2. For the future subsystem supporting struct_ops, the subsystem could be compiled as a kernel module or as a built-in. register_bpf_struct_ops() should work for both.
On 9/15/23 15:43, Martin KaFai Lau wrote: > On 9/12/23 11:14 PM, thinker.li@gmail.com wrote: >> From: Kui-Feng Lee <thinker.li@gmail.com> >> >> Move most of code to bpf_struct_ops_init_one() that can be use to >> initialize new struct_ops types registered dynamically. > > While in RFC, still better to have SOB so that it won't be overlooked in > the future. > >> --- >> 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 fdc3e8705a3c..1662875e0ebe 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); > > It needs to do some sanity checks on the value_type since this won't be > statically enforced by bpf_struct_ops.c. Do you mean to check if a value_type has refcnt, state and data field? > > [ ... ] > >> +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 > > Can this static way of defining struct_ops be removed? bpf_tcp_ca should > be able to use the register_bpf_struct_ops() introduced in patch 2. It sounds good for me. > > For the future subsystem supporting struct_ops, the subsystem could be > compiled as a kernel module or as a built-in. register_bpf_struct_ops() > should work for both. >
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c index fdc3e8705a3c..1662875e0ebe 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); } }
From: Kui-Feng Lee <thinker.li@gmail.com> Move most of code to bpf_struct_ops_init_one() that can be use to initialize new struct_ops types registered dynamically. --- kernel/bpf/bpf_struct_ops.c | 157 +++++++++++++++++++----------------- 1 file changed, 83 insertions(+), 74 deletions(-)