| Message ID | 20230920155923.151136-9-thinker.li@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | Registrating struct_ops types from modules | expand |
On Wed, Sep 20, 2023 at 9:00 AM <thinker.li@gmail.com> wrote: > > From: Kui-Feng Lee <thinker.li@gmail.com> > > The type info of a struct_ops type may be in a module. So, we need to know > which module BTF to look for type information. The later patches will make > libbpf to attach module BTFs to programs. This patch passes attached BTF > from syscall to bpf_struct_ops subsystem to make sure attached BTF is > available when the bpf_struct_ops subsystem is ready to use it. > > bpf_prog has attach_btf in aux from attach_btf_obj_fd, that is pass along > with the bpf_attr loading the program. attach_btf is used to find the btf > type of attach_btf_id. attach_btf_id is used to identify the traced > function for a trace program. For struct_ops programs, it is used to > identify the struct_ops type of the struct_ops object a program attached > to. > > Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> > --- > include/uapi/linux/bpf.h | 4 ++++ > kernel/bpf/bpf_struct_ops.c | 12 +++++++++++- > kernel/bpf/syscall.c | 2 +- > kernel/bpf/verifier.c | 4 +++- > tools/include/uapi/linux/bpf.h | 4 ++++ > 5 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 73b155e52204..178d6fa45fa0 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -1390,6 +1390,10 @@ union bpf_attr { > * to using 5 hash functions). > */ > __u64 map_extra; > + > + __u32 mod_btf_fd; /* fd pointing to a BTF type data > + * for btf_vmlinux_value_type_id. > + */ we have attach_btf_obj_fd for BPF_PROG_LOAD command, so I guess consistent naming would be "<something>_btf_obj_fd" where <something> would make it more-or-less clear that this is BTF 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 8b5c859377e9..d5600d9ad302 100644 > --- a/kernel/bpf/bpf_struct_ops.c > +++ b/kernel/bpf/bpf_struct_ops.c > @@ -765,9 +765,19 @@ 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(attr->btf_vmlinux_value_type_id, btf_vmlinux); > + /* XXX: We need a module name or ID to find a BTF type. */ > + /* XXX: should use btf from attr->btf_fd */ Do we need these XXX: comments? I think you had some more in previous patches > + if (attr->mod_btf_fd) { > + btf = btf_get_by_fd(attr->mod_btf_fd); > + if (IS_ERR(btf)) > + return ERR_PTR(PTR_ERR(btf)); > + } else { > + btf = btf_vmlinux; > + } > + st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf); > if (!st_ops) > return ERR_PTR(-ENOTSUPP); should we make sure that module's BTF is put properly on error? > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 85c1d908f70f..fed3870fec7a 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 mod_btf_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 99b45501951c..11f85dbc911b 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_id, btf_vmlinux); > + st_ops = bpf_struct_ops_find(btf_id, btf); > 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..178d6fa45fa0 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -1390,6 +1390,10 @@ union bpf_attr { > * to using 5 hash functions). > */ > __u64 map_extra; > + > + __u32 mod_btf_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 >
On 9/25/23 15:58, Andrii Nakryiko wrote: > On Wed, Sep 20, 2023 at 9:00 AM <thinker.li@gmail.com> wrote: >> >> From: Kui-Feng Lee <thinker.li@gmail.com> >> >> The type info of a struct_ops type may be in a module. So, we need to know >> which module BTF to look for type information. The later patches will make >> libbpf to attach module BTFs to programs. This patch passes attached BTF >> from syscall to bpf_struct_ops subsystem to make sure attached BTF is >> available when the bpf_struct_ops subsystem is ready to use it. >> >> bpf_prog has attach_btf in aux from attach_btf_obj_fd, that is pass along >> with the bpf_attr loading the program. attach_btf is used to find the btf >> type of attach_btf_id. attach_btf_id is used to identify the traced >> function for a trace program. For struct_ops programs, it is used to >> identify the struct_ops type of the struct_ops object a program attached >> to. >> >> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> >> --- >> include/uapi/linux/bpf.h | 4 ++++ >> kernel/bpf/bpf_struct_ops.c | 12 +++++++++++- >> kernel/bpf/syscall.c | 2 +- >> kernel/bpf/verifier.c | 4 +++- >> tools/include/uapi/linux/bpf.h | 4 ++++ >> 5 files changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >> index 73b155e52204..178d6fa45fa0 100644 >> --- a/include/uapi/linux/bpf.h >> +++ b/include/uapi/linux/bpf.h >> @@ -1390,6 +1390,10 @@ union bpf_attr { >> * to using 5 hash functions). >> */ >> __u64 map_extra; >> + >> + __u32 mod_btf_fd; /* fd pointing to a BTF type data >> + * for btf_vmlinux_value_type_id. >> + */ > > we have attach_btf_obj_fd for BPF_PROG_LOAD command, so I guess > consistent naming would be "<something>_btf_obj_fd" where <something> > would make it more-or-less clear that this is BTF for > btf_vmlinux_value_type_id? Got it! I will rename it to value_type_btf_obj_fd. > >> }; >> >> 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 8b5c859377e9..d5600d9ad302 100644 >> --- a/kernel/bpf/bpf_struct_ops.c >> +++ b/kernel/bpf/bpf_struct_ops.c >> @@ -765,9 +765,19 @@ 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(attr->btf_vmlinux_value_type_id, btf_vmlinux); >> + /* XXX: We need a module name or ID to find a BTF type. */ >> + /* XXX: should use btf from attr->btf_fd */ > > Do we need these XXX: comments? I think you had some more in previous patches Will be removed. > >> + if (attr->mod_btf_fd) { >> + btf = btf_get_by_fd(attr->mod_btf_fd); >> + if (IS_ERR(btf)) >> + return ERR_PTR(PTR_ERR(btf)); >> + } else { >> + btf = btf_vmlinux; >> + } >> + st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf); >> if (!st_ops) >> return ERR_PTR(-ENOTSUPP); > > should we make sure that module's BTF is put properly on error? Yes, this issue has been addressed locally. > >> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c >> index 85c1d908f70f..fed3870fec7a 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 mod_btf_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 99b45501951c..11f85dbc911b 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_id, btf_vmlinux); >> + st_ops = bpf_struct_ops_find(btf_id, btf); >> 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..178d6fa45fa0 100644 >> --- a/tools/include/uapi/linux/bpf.h >> +++ b/tools/include/uapi/linux/bpf.h >> @@ -1390,6 +1390,10 @@ union bpf_attr { >> * to using 5 hash functions). >> */ >> __u64 map_extra; >> + >> + __u32 mod_btf_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 >>
On 9/20/23 8:59 AM, thinker.li@gmail.com wrote: > From: Kui-Feng Lee <thinker.li@gmail.com> > > The type info of a struct_ops type may be in a module. So, we need to know > which module BTF to look for type information. The later patches will make > libbpf to attach module BTFs to programs. This patch passes attached BTF > from syscall to bpf_struct_ops subsystem to make sure attached BTF is > available when the bpf_struct_ops subsystem is ready to use it. > > bpf_prog has attach_btf in aux from attach_btf_obj_fd, that is pass along > with the bpf_attr loading the program. attach_btf is used to find the btf > type of attach_btf_id. attach_btf_id is used to identify the traced > function for a trace program. For struct_ops programs, it is used to > identify the struct_ops type of the struct_ops object a program attached > to. > > Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> > --- > include/uapi/linux/bpf.h | 4 ++++ > kernel/bpf/bpf_struct_ops.c | 12 +++++++++++- > kernel/bpf/syscall.c | 2 +- > kernel/bpf/verifier.c | 4 +++- > tools/include/uapi/linux/bpf.h | 4 ++++ > 5 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 73b155e52204..178d6fa45fa0 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -1390,6 +1390,10 @@ union bpf_attr { > * to using 5 hash functions). > */ > __u64 map_extra; > + > + __u32 mod_btf_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 8b5c859377e9..d5600d9ad302 100644 > --- a/kernel/bpf/bpf_struct_ops.c > +++ b/kernel/bpf/bpf_struct_ops.c > @@ -765,9 +765,19 @@ 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(attr->btf_vmlinux_value_type_id, btf_vmlinux); > + /* XXX: We need a module name or ID to find a BTF type. */ > + /* XXX: should use btf from attr->btf_fd */ > + if (attr->mod_btf_fd) { > + btf = btf_get_by_fd(attr->mod_btf_fd); The btf is leaked in all cases because it is not stored (and owned) in st_map during map_alloc. This circle back to the earlier comment in patch 4 about where btf is stored. > + if (IS_ERR(btf)) > + return ERR_PTR(PTR_ERR(btf)); > + } else { > + btf = btf_vmlinux; > + } > + st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf); > if (!st_ops) > return ERR_PTR(-ENOTSUPP); >
On 9/25/23 17:24, Martin KaFai Lau wrote: > On 9/20/23 8:59 AM, thinker.li@gmail.com wrote: >> From: Kui-Feng Lee <thinker.li@gmail.com> >> >> The type info of a struct_ops type may be in a module. So, we need to >> know >> which module BTF to look for type information. The later patches will >> make >> libbpf to attach module BTFs to programs. This patch passes attached BTF >> from syscall to bpf_struct_ops subsystem to make sure attached BTF is >> available when the bpf_struct_ops subsystem is ready to use it. >> >> bpf_prog has attach_btf in aux from attach_btf_obj_fd, that is pass along >> with the bpf_attr loading the program. attach_btf is used to find the btf >> type of attach_btf_id. attach_btf_id is used to identify the traced >> function for a trace program. For struct_ops programs, it is used to >> identify the struct_ops type of the struct_ops object a program attached >> to. >> >> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com> >> --- >> include/uapi/linux/bpf.h | 4 ++++ >> kernel/bpf/bpf_struct_ops.c | 12 +++++++++++- >> kernel/bpf/syscall.c | 2 +- >> kernel/bpf/verifier.c | 4 +++- >> tools/include/uapi/linux/bpf.h | 4 ++++ >> 5 files changed, 23 insertions(+), 3 deletions(-) >> >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >> index 73b155e52204..178d6fa45fa0 100644 >> --- a/include/uapi/linux/bpf.h >> +++ b/include/uapi/linux/bpf.h >> @@ -1390,6 +1390,10 @@ union bpf_attr { >> * to using 5 hash functions). >> */ >> __u64 map_extra; >> + >> + __u32 mod_btf_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 8b5c859377e9..d5600d9ad302 100644 >> --- a/kernel/bpf/bpf_struct_ops.c >> +++ b/kernel/bpf/bpf_struct_ops.c >> @@ -765,9 +765,19 @@ 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(attr->btf_vmlinux_value_type_id, btf_vmlinux); >> + /* XXX: We need a module name or ID to find a BTF type. */ >> + /* XXX: should use btf from attr->btf_fd */ >> + if (attr->mod_btf_fd) { >> + btf = btf_get_by_fd(attr->mod_btf_fd); > > The btf is leaked in all cases because it is not stored (and owned) in > st_map during map_alloc. This circle back to the earlier comment in > patch 4 about where btf is stored. This has been fixed locally. Basically, map will hold the module instead of btf. > >> + if (IS_ERR(btf)) >> + return ERR_PTR(PTR_ERR(btf)); >> + } else { >> + btf = btf_vmlinux; >> + } >> + st_ops = >> bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf); >> if (!st_ops) >> return ERR_PTR(-ENOTSUPP); >
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 73b155e52204..178d6fa45fa0 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -1390,6 +1390,10 @@ union bpf_attr { * to using 5 hash functions). */ __u64 map_extra; + + __u32 mod_btf_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 8b5c859377e9..d5600d9ad302 100644 --- a/kernel/bpf/bpf_struct_ops.c +++ b/kernel/bpf/bpf_struct_ops.c @@ -765,9 +765,19 @@ 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(attr->btf_vmlinux_value_type_id, btf_vmlinux); + /* XXX: We need a module name or ID to find a BTF type. */ + /* XXX: should use btf from attr->btf_fd */ + if (attr->mod_btf_fd) { + btf = btf_get_by_fd(attr->mod_btf_fd); + if (IS_ERR(btf)) + return ERR_PTR(PTR_ERR(btf)); + } else { + btf = btf_vmlinux; + } + st_ops = bpf_struct_ops_find_value(attr->btf_vmlinux_value_type_id, btf); if (!st_ops) return ERR_PTR(-ENOTSUPP); diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 85c1d908f70f..fed3870fec7a 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 mod_btf_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 99b45501951c..11f85dbc911b 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_id, btf_vmlinux); + st_ops = bpf_struct_ops_find(btf_id, btf); 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..178d6fa45fa0 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -1390,6 +1390,10 @@ union bpf_attr { * to using 5 hash functions). */ __u64 map_extra; + + __u32 mod_btf_fd; /* fd pointing to a BTF type data + * for btf_vmlinux_value_type_id. + */ }; struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */